{"thread":{"id":"62544","subject":"[PATCH] bisect: address Coverity warning about potential double free","startedAt":"2024-11-25T15:56:46Z","lastAt":"2024-11-26T02:29:22Z","messageCount":3,"participants":["Patrick Steinhardt","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"508102","messageId":"20241125-pks-leak-fixes-address-double-free-v1-1-d33fd8ebf55b@pks.im","threadId":"62544","inReplyTo":null,"subject":"[PATCH] bisect: address Coverity warning about potential double free","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-25T15:56:25Z","receivedAt":"2024-11-25T15:56:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Coverity has started to warn about a potential double-free in\n`find_bisection()`. This warning is triggered because we may modify the\nlist head of the passed-in `commit_list` in case it is an UNINTERESTING\ncommit, but still call `free_commit_list()` on the original variable\nthat points to the now-freed head in case where `do_find_bisection()`\nreturns a `NULL` pointer.\n\nAs far as I can see, this double free cannot happen in practice, as\n`do_find_bisection()` only returns a `NULL` pointer when it was passed a\n`NULL` input. So in order to trigger the double free we would have to\ncall `find_bisection()` with a commit list that only consists of\nUNINTERESTING commits, but I have not been able to construct a case\nwhere that happens.\n\nDrop the `else` branch entirely as it seems to be a no-op anyway.\nAnother option might be to instead call `free_commit_list()` on `list`,\nwhich is the modified version of `commit_list` and thus wouldn't cause a\ndouble free. But as mentioned, I couldn't come up with any case where a\npassed-in non-NULL list becomes empty, so this shouldn't be necessary.\nAnd if it ever does become necessary we'd notice anyway via the leak\nsanitizer.\n\nInterestingly enough we did not have a single test exercising this\nbranch: all tests pass just fine even when replacing it with a call to\n`BUG()`. Add a test that exercises it.\n\nReported-by: Jeff King <peff@peff.net>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nHi,\n\nthis addresses the issue reported by Peff in [1]. The patch is based on\ntop of 6ea2d9d271 (Sync with Git 2.47.1, 2024-11-25) with\nps/leakfixes-part-10 at fc1ddf42af (t: remove TEST_PASSES_SANITIZE_LEAK\nannotations, 2024-11-20) merged into it.\n\nThanks!\n\nPatrick\n\n[1]: <20241125131722.GA1613472@coredump.intra.peff.net>\n---\n bisect.c                   | 2 --\n t/t6002-rev-list-bisect.sh | 5 +++++\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex f6fa5c235ffb351011ed5e81771fbcdad9ca0917..d71c4e4b44b40706b8182bc8821bf711b5794376 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -442,8 +442,6 @@ void find_bisection(struct commit_list **commit_list, int *reaches,\n \t\t\tbest->next = NULL;\n \t\t}\n \t\t*reaches = weight(best);\n-\t} else {\n-\t\tfree_commit_list(*commit_list);\n \t}\n \t*commit_list = best;\n \ndiff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh\nindex b95a0212adff71632d0b91cf96432b276c86a44c..daa009c9a1b4b67d510df74f1d5d5cc2b1a904cd 100755\n--- a/t/t6002-rev-list-bisect.sh\n+++ b/t/t6002-rev-list-bisect.sh\n@@ -308,4 +308,9 @@ test_expect_success '--bisect-all --first-parent' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--bisect without any revisions' '\n+\tgit rev-list --bisect HEAD..HEAD >out &&\n+\ttest_must_be_empty out\n+'\n+\n test_done\n\n---\nbase-commit: c5c2f8884377a610fe2752658af3b06f790502b5\nchange-id: 20241125-pks-leak-fixes-address-double-free-e7fee5a4a300\n\n"},{"id":"508103","messageId":"20241125163543.GA13033@coredump.intra.peff.net","threadId":"62544","inReplyTo":"20241125-pks-leak-fixes-address-double-free-v1-1-d33fd8ebf55b@pks.im","subject":"Re: [PATCH] bisect: address Coverity warning about potential double free","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-25T16:35:43Z","receivedAt":"2024-11-25T16:35:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 25, 2024 at 04:56:25PM +0100, Patrick Steinhardt wrote:\n\n> Coverity has started to warn about a potential double-free in\n> `find_bisection()`. This warning is triggered because we may modify the\n> list head of the passed-in `commit_list` in case it is an UNINTERESTING\n> commit, but still call `free_commit_list()` on the original variable\n> that points to the now-freed head in case where `do_find_bisection()`\n> returns a `NULL` pointer.\n> \n> As far as I can see, this double free cannot happen in practice, as\n> `do_find_bisection()` only returns a `NULL` pointer when it was passed a\n> `NULL` input. So in order to trigger the double free we would have to\n> call `find_bisection()` with a commit list that only consists of\n> UNINTERESTING commits, but I have not been able to construct a case\n> where that happens.\n> \n> Drop the `else` branch entirely as it seems to be a no-op anyway.\n> Another option might be to instead call `free_commit_list()` on `list`,\n> which is the modified version of `commit_list` and thus wouldn't cause a\n> double free. But as mentioned, I couldn't come up with any case where a\n> passed-in non-NULL list becomes empty, so this shouldn't be necessary.\n> And if it ever does become necessary we'd notice anyway via the leak\n> sanitizer.\n\nNicely explained.\n\n> Interestingly enough we did not have a single test exercising this\n> branch: all tests pass just fine even when replacing it with a call to\n> `BUG()`. Add a test that exercises it.\n\nSeems reasonable. That test will end up passing an empty list into\nfind_bisection(), because everything is UNINTERESTING, and the revision\nmachinery's limit_list() removes the UNINTERESTING elements from the\nrevs->commits list.\n\nI wondered if a topology more like this:\n\n      one\n      /\n  base--two\n\ncould produce something interesting from \"rev-list --bisect ^one two\".\nBut no, we still end up removing all of the uninteresting commits before\nwe hit find_bisection(). And anyway, \"two\" is obviously going to be the\noutput, so we know \"best\" won't be NULL and it won't hit your new code.\n\nSo I agree there doesn't seem to be a way to trigger the new code that\nisn't just a noop.\n\nThanks for fixing this!\n\n-Peff\n"},{"id":"508132","messageId":"xmqqplmi3eyo.fsf@gitster.g","threadId":"62544","inReplyTo":"20241125163543.GA13033@coredump.intra.peff.net","subject":"Re: [PATCH] bisect: address Coverity warning about potential double free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-26T02:29:19Z","receivedAt":"2024-11-26T02:29:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> Drop the `else` branch entirely as it seems to be a no-op anyway.\n>> Another option might be to instead call `free_commit_list()` on `list`,\n>> which is the modified version of `commit_list` and thus wouldn't cause a\n>> double free. But as mentioned, I couldn't come up with any case where a\n>> passed-in non-NULL list becomes empty, so this shouldn't be necessary.\n>> And if it ever does become necessary we'd notice anyway via the leak\n>> sanitizer.\n>\n> Nicely explained.\n>\n>> Interestingly enough we did not have a single test exercising this\n>> branch: all tests pass just fine even when replacing it with a call to\n>> `BUG()`. Add a test that exercises it.\n>\n> Seems reasonable. That test will end up passing an empty list into\n> find_bisection(), because everything is UNINTERESTING, and the revision\n> machinery's limit_list() removes the UNINTERESTING elements from the\n> revs->commits list.\n>\n> I wondered if a topology more like this:\n>\n>       one\n>       /\n>   base--two\n>\n> could produce something interesting from \"rev-list --bisect ^one two\".\n> But no, we still end up removing all of the uninteresting commits before\n> we hit find_bisection(). And anyway, \"two\" is obviously going to be the\n> output, so we know \"best\" won't be NULL and it won't hit your new code.\n>\n> So I agree there doesn't seem to be a way to trigger the new code that\n> isn't just a noop.\n>\n> Thanks for fixing this!\n\nThanks, both of you.  Will queue and merge to 'next'.\n"}]}