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

From: Toon Claes <toon@iotcl.com>
Date: Nov 20, 2024, 10:32 UTC
Message-ID: <875xoitcu8.fsf@iotcl.com>
In-Reply-To: <20241111-b4-pks-leak-fixes-pt10-v2-7-6154bf91f0b0@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 22 quoted lines

> There are various cases where we leak commit list items because we
> evict items from the list, but don't free them. Plug those.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  bisect.c                    | 35 +++++++++++++++++++++++++++--------
>  t/t6030-bisect-porcelain.sh |  1 +
>  2 files changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index 12efcff2e3c1836ab6e63d36e4d42269fbcaeaab..0e804086cbf6a02d42bda98b62fb86daf099b82d 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -440,11 +440,19 @@ void find_bisection(struct commit_list **commit_list, int *reaches,
>  			free_commit_list(list->next);
>  			best = list;
>  			best->next = NULL;
> +		} else {
> +			for (p = list; p != best; p = next) {
> +				next = p->next;
> +				free(p);
> +			}

This makes the code do:

    if (!something) {
        // ...
    } else {
        // ...
    }

I find it odd reading code like that. Would you mind swapping them around? Or is there a reason we type it like this, because I see this also being done like this around line 393?

More on the functional side of this code, I'd like to understand better what's happening here. It's clear to me when `best` is NULL we want to free the whole commit_list. And when `best` is set with FIND_BISECTION_ALL flag set, we want to free the commit_list up to `best`. But when the FIND_BISECTION_ALL flag is not set, we want to do the opposite, free from `best->next` till the end? But what has this to do with FIND_BISECTION_ALL?

-- Toon