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. 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? 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. :) -Peff
