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
