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

From: Jeff King <peff@peff.net>
Date: Nov 25, 2024, 16:35 UTC
Message-ID: <20241125163543.GA13033@coredump.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:

Show 21 quoted lines

> 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