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

From: Patrick Steinhardt <ps@pks.im>
Date: Nov 25, 2024, 12:38 UTC
Message-ID: <Z0RvxAkmfZhgo_LJ@pks.im>
In-Reply-To: <20241125112746.GA1069812@coredump.intra.peff.net>

On Mon, Nov 25, 2024 at 06:27:46AM -0500, Jeff King wrote:

Show 16 quoted lines

> 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]

Show 17 quoted lines

> 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