]> git.ipfire.org Git - thirdparty/git.git/commit
bisect: address Coverity warning about potential double free
authorPatrick Steinhardt <ps@pks.im>
Mon, 25 Nov 2024 15:56:25 +0000 (16:56 +0100)
committerJunio C Hamano <gitster@pobox.com>
Tue, 26 Nov 2024 01:22:24 +0000 (10:22 +0900)
commit5f9f7fafb7e74ef2965766345f45851732315b00
treef8a46571300f69d4c18dc7ba64f03c4c8e9269d8
parentc6c977e82b94a8266a1f24bed6fcddb15bd01d1c
bisect: address Coverity warning about potential double free

Coverity has started to warn about a potential double-free in
`find_bisection()`. This warning is triggered because we may modify the
list head of the passed-in `commit_list` in case it is an UNINTERESTING
commit, but still call `free_commit_list()` on the original variable
that points to the now-freed head in case where `do_find_bisection()`
returns a `NULL` pointer.

As far as I can see, this double free cannot happen in practice, as
`do_find_bisection()` only returns a `NULL` pointer when it was passed a
`NULL` input. So in order to trigger the double free we would have to
call `find_bisection()` with a commit list that only consists of
UNINTERESTING commits, but I have not been able to construct a case
where that happens.

Drop the `else` branch entirely as it seems to be a no-op anyway.
Another option might be to instead call `free_commit_list()` on `list`,
which is the modified version of `commit_list` and thus wouldn't cause a
double free. But as mentioned, I couldn't come up with any case where a
passed-in non-NULL list becomes empty, so this shouldn't be necessary.
And if it ever does become necessary we'd notice anyway via the leak
sanitizer.

Interestingly enough we did not have a single test exercising this
branch: all tests pass just fine even when replacing it with a call to
`BUG()`. Add a test that exercises it.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
bisect.c
t/t6002-rev-list-bisect.sh