[PATCH] bisect: address Coverity warning about potential double free

From: Patrick Steinhardt <ps@pks.im>
Date: Nov 25, 2024, 15:56 UTC
Message-ID: <20241125-pks-leak-fixes-address-double-free-v1-1-d33fd8ebf55b@pks.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 <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