Re: [PATCH v3 07/27] bisect: fix various cases where we leak commit list items

[Patrick Steinhardt <ps@pks.im>]

On Mon, Nov 25, 2024 at 08:17:22AM -0500, Jeff King wrote:
> On Mon, Nov 25, 2024 at 01:38:28PM +0100, Patrick Steinhardt wrote:
> 
> > > 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.
> 
> Hrm, I thought you were fixing a case that was triggered by the
> leak-checker here. But I guess there were several hunks in the patch, so
> maybe you added this one based on inspection of the code and it was
> never important.

I remember that it took me quite a while until I was able to root cause
all the leaks in this subsystem, so some of the changes are of "while at
it" spirit.

> Just skimming over do_find_bisection(), it will always return something
> unless it is fed a NULL list in the first place. So a NULL "best"
> implies a NULL "list". Which implies there is nothing to free, because
> every item from commit_list was either UNINTERESTING and freed earlier,
> or made it into "list".
> 
> So could the "else" added by your patch just go away entirely?

Seems like it, yeah.

> That would also explain why you couldn't trigger this in practice; one
> imagines that the bisect code may avoid getting this far in the first
> place with an empty list. But you can do:
> 
>   git rev-list --bisect  ;# no revisions!
> 
> to get there. I wondered if:
> 
>   git rev-list --bisect ^HEAD
> 
> might give us the double-free, but that ends up with an empty
> commit-list in the first place.
> 
> > 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?
> 
> I'm not that familiar with it either. But it does look like the intent
> was that commit_list would get cannibalized into "list" (freeing
> anything that didn't make it), and then we'd work with "list" from
> there. And when we _do_ have anything in the list, then we either return
> it (if FIND_BISECTION_ALL is set) or free the non-best entries. But all
> of that is handled in the "if (best)" block.
> 
> So I think the code was non-leaky before your patch, and you'd just want
> to revert that hunk.
> 
> I'd be just as happy for you to send it. :)

Okay, I'll drop that hunk.

Patrick