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.
Looking at the parts of the function related to commit_list:
void find_bisection(struct commit_list **commit_list, ...)
{
...
/*
* Count the number of total and tree-changing items on the
* list, while reversing the list.
*/
for (nr = on_list = 0, last = NULL, p = *commit_list;
p;
p = next) {
unsigned commit_flags = p->item->object.flags;
next = p->next;
if (commit_flags & UNINTERESTING) {
free(p);
continue;
}
p->next = last;
last = p;
if (!(commit_flags & TREESAME))
nr++;
on_list++;
}
...
best = do_find_bisection(list, nr, weights, bisect_flags);
if (best) {
if (!(bisect_flags & FIND_BISECTION_ALL)) {
list->item = best->item;
free_commit_list(list->next);
best = list;
best->next = NULL;
}
*reaches = weight(best);
} else {
free_commit_list(*commit_list);
}
...
}
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)?
-Peff
