On Mon, Nov 25, 2024 at 06:27:46AM -0500, Jeff King wrote:
> On Wed, Nov 20, 2024 at 02:39:36PM +0100, Patrick Steinhardt wrote:
>
> > diff --git a/bisect.c b/bisect.c
> > index 12efcff2e3c1836ab6e63d36e4d42269fbcaeaab..f6fa5c235ffb351011ed5e81771fbcdad9ca0917 100644
> > --- a/bisect.c
> > +++ b/bisect.c
> > @@ -442,9 +442,12 @@ void find_bisection(struct commit_list **commit_list, int *reaches,
> > best->next = NULL;
> > }
> > *reaches = weight(best);
> > + } else {
> > + free_commit_list(*commit_list);
> > }
>
> Coverity complains about this hunk being a potential double-free, and
> I'm not sure it's wrong.
Sigh. The whole bisection code has been a bit of a nightmare to
make sense of for me.
[snip]
> We iterate over commit_list using "p". If the entry is UNINTERESTING, we
> free that node immediately and skip it. That's OK for a node in the
> middle of the list, since after we reverse the list by modifying the
> next pointers, nobody will refer to it anymore.
>
> But we never updated commit_list. What if the first entry in the list is
> UNINTERESTING? We'll have freed it, but *commit_list will still point to
> it, and your free_commit_list() will be a double-free.
>
> And for that matter, I am confused about what should be in commit_list
> after the reverse anyway. Even if we didn't free that first entry, it
> will now be the final entry in the reversed list. So wouldn't
> *commit_list always be pointing to a single node?
>
> Should the code be freeing "list" and not "*commit_list"? Should the
> reversal be assigning "*commit_list = last" (in which case do we still
> need "list" at all)?
Mh. By now I wonder whether this code can be hit in the first place. Is
there ever a case where `do_find_bisection()` returns `NULL`? Replacing
the whole branch with `BUG()` doesn't make even a single test case fail.
Anyway. I'm not familiar enough with the code in question to tell, and
it's clear that `*commit_list = best;` will leak `*commit_list` if it is
not free'd beforehand. So I think freeing `list` is the right thing to
do. Do you want to send a follow-up patch or shall I do this?
Patrick
