Re: [RFC PATCH 01/15] remote.c: don't dereference NULL in freeing loop

From: René Scharfe <l.s.r@web.de>
Date: Jun 3, 2022, 21:07 UTC
Message-ID: <0c296c0e-bbd4-57a6-1cab-3b98c3bc8927@web.de>
In-Reply-To: <RFC-patch-01.15-b3a678d934a-20220603T183608Z-avarab@gmail.com>

Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:

Show 8 quoted lines

> Fix a bug in fd3cb0501e1 (remote: move static variables into
> per-repository struct, 2021-11-17) where we'd free(remote->pushurl[i])
> after having NULL'd out remote->pushurl. itself.
>
> While we're at it let's get rid of the redundant braces per the
> CodingGuidelines, which also serves to show in the diff context that
> we were doing a FREE_AND_NULL(remote->pushurl) afterwards too, let's
> keep that one.

The extended context is helping, but the brace removal makes this change harder to read.

Show 19 quoted lines

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  remote.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 930fdc9c2f6..61240562df1 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -144,14 +144,10 @@ static void remote_clear(struct remote *remote)
>  	free((char *)remote->name);
>  	free((char *)remote->foreign_vcs);
>
> -	for (i = 0; i < remote->url_nr; i++) {
> +	for (i = 0; i < remote->url_nr; i++)
>  		free((char *)remote->url[i]);
> -	}
> -	FREE_AND_NULL(remote->pushurl);

So you remove the redundant release of the pushurl array, but the url array, which should be freed here after its elements are released, still leaks. The duplicate FREE_AND_NULL perhaps resulted from a copy&paste error.

Show 6 quoted lines

> -
> -	for (i = 0; i < remote->pushurl_nr; i++) {
> +	for (i = 0; i < remote->pushurl_nr; i++)
>  		free((char *)remote->pushurl[i]);
> -	}
>  	FREE_AND_NULL(remote->pushurl);

Why set pushurl to NULL after release? This results in an invalid state unless pushurl_nr und pushurl_alloc are reset to zero. Same goes for the url array above -- either a simple free(3) call suffices or url_nr and url_alloc need to be cleared as well.

>  	free((char *)remote->receivepack);
>  	free((char *)remote->uploadpack);