# [PATCH] bisect: address Coverity warning about potential double free 3 messages from 2024-11-25 to 2024-11-26. Participants: Patrick Steinhardt, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/62544 ## Patrick Steinhardt, 2024-11-25 15:56 Subject: [PATCH] bisect: address Coverity warning about potential double free Message-ID: <20241125-pks-leak-fixes-address-double-free-v1-1-d33fd8ebf55b@pks.im> URL: https://gitlist.dev/e/20241125-pks-leak-fixes-address-double-free-v1-1-d33fd8ebf55b%40pks.im ``` 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 Signed-off-by: Patrick Steinhardt --- 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 ``` ## Jeff King, 2024-11-25 16:35 Subject: Re: [PATCH] bisect: address Coverity warning about potential double free Message-ID: <20241125163543.GA13033@coredump.intra.peff.net> URL: https://gitlist.dev/e/20241125163543.GA13033%40coredump.intra.peff.net In-Reply-To: <20241125-pks-leak-fixes-address-double-free-v1-1-d33fd8ebf55b@pks.im> ``` 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 ``` ## Junio C Hamano, 2024-11-26 02:29 Subject: Re: [PATCH] bisect: address Coverity warning about potential double free Message-ID: URL: https://gitlist.dev/e/xmqqplmi3eyo.fsf%40gitster.g In-Reply-To: <20241125163543.GA13033@coredump.intra.peff.net> ``` Jeff King 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'. ```