* [PATCH] bisect: address Coverity warning about potential double free
@ 2024-11-25 15:56 Patrick Steinhardt
2024-11-25 16:35 ` Jeff King
0 siblings, 1 reply; 3+ messages in thread
From: Patrick Steinhardt @ 2024-11-25 15:56 UTC (permalink / raw)
To: git; +Cc: Jeff King
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>
---
Hi,
this addresses the issue reported by Peff in [1]. The patch is based on
top of 6ea2d9d271 (Sync with Git 2.47.1, 2024-11-25) with
ps/leakfixes-part-10 at fc1ddf42af (t: remove TEST_PASSES_SANITIZE_LEAK
annotations, 2024-11-20) merged into it.
Thanks!
Patrick
[1]: <20241125131722.GA1613472@coredump•intra.peff.net>
---
bisect.c | 2 --
t/t6002-rev-list-bisect.sh | 5 +++++
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/bisect.c b/bisect.c
index f6fa5c235ffb351011ed5e81771fbcdad9ca0917..d71c4e4b44b40706b8182bc8821bf711b5794376 100644
--- a/bisect.c
+++ b/bisect.c
@@ -442,8 +442,6 @@ void find_bisection(struct commit_list **commit_list, int *reaches,
best->next = NULL;
}
*reaches = weight(best);
- } else {
- free_commit_list(*commit_list);
}
*commit_list = best;
diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
index b95a0212adff71632d0b91cf96432b276c86a44c..daa009c9a1b4b67d510df74f1d5d5cc2b1a904cd 100755
--- a/t/t6002-rev-list-bisect.sh
+++ b/t/t6002-rev-list-bisect.sh
@@ -308,4 +308,9 @@ test_expect_success '--bisect-all --first-parent' '
test_cmp expect actual
'
+test_expect_success '--bisect without any revisions' '
+ git rev-list --bisect HEAD..HEAD >out &&
+ test_must_be_empty out
+'
+
test_done
---
base-commit: c5c2f8884377a610fe2752658af3b06f790502b5
change-id: 20241125-pks-leak-fixes-address-double-free-e7fee5a4a300
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] bisect: address Coverity warning about potential double free
2024-11-25 15:56 [PATCH] bisect: address Coverity warning about potential double free Patrick Steinhardt
@ 2024-11-25 16:35 ` Jeff King
2024-11-26 2:29 ` Junio C Hamano
0 siblings, 1 reply; 3+ messages in thread
From: Jeff King @ 2024-11-25 16:35 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
On Mon, Nov 25, 2024 at 04:56:25PM +0100, Patrick Steinhardt wrote:
> 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.
Nicely explained.
> 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.
Seems reasonable. That test will end up passing an empty list into
find_bisection(), because everything is UNINTERESTING, and the revision
machinery's limit_list() removes the UNINTERESTING elements from the
revs->commits list.
I wondered if a topology more like this:
one
/
base--two
could produce something interesting from "rev-list --bisect ^one two".
But no, we still end up removing all of the uninteresting commits before
we hit find_bisection(). And anyway, "two" is obviously going to be the
output, so we know "best" won't be NULL and it won't hit your new code.
So I agree there doesn't seem to be a way to trigger the new code that
isn't just a noop.
Thanks for fixing this!
-Peff
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] bisect: address Coverity warning about potential double free
2024-11-25 16:35 ` Jeff King
@ 2024-11-26 2:29 ` Junio C Hamano
0 siblings, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2024-11-26 2:29 UTC (permalink / raw)
To: Jeff King; +Cc: Patrick Steinhardt, git
Jeff King <peff@peff•net> writes:
>> 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.
>
> Nicely explained.
>
>> 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.
>
> Seems reasonable. That test will end up passing an empty list into
> find_bisection(), because everything is UNINTERESTING, and the revision
> machinery's limit_list() removes the UNINTERESTING elements from the
> revs->commits list.
>
> I wondered if a topology more like this:
>
> one
> /
> base--two
>
> could produce something interesting from "rev-list --bisect ^one two".
> But no, we still end up removing all of the uninteresting commits before
> we hit find_bisection(). And anyway, "two" is obviously going to be the
> output, so we know "best" won't be NULL and it won't hit your new code.
>
> So I agree there doesn't seem to be a way to trigger the new code that
> isn't just a noop.
>
> Thanks for fixing this!
Thanks, both of you. Will queue and merge to 'next'.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-11-26 2:29 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-25 15:56 [PATCH] bisect: address Coverity warning about potential double free Patrick Steinhardt
2024-11-25 16:35 ` Jeff King
2024-11-26 2:29 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox