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

From: Jeff King <peff@peff.net>
Date: Nov 25, 2024, 11:27 UTC
Message-ID: <20241125112746.GA1069812@coredump.intra.peff.net>
In-Reply-To: <20241120-b4-pks-leak-fixes-pt10-v3-7-d67f08f45c74@pks.im>

On Wed, Nov 20, 2024 at 02:39:36PM +0100, Patrick Steinhardt wrote:

Show 11 quoted lines

> 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