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

["René Scharfe" <l.s.r@web.de>]

Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:
> 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.

>
> 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.

> -
> -	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);