Re: [PATCH 13/20] sideband: fix leaks when configuring sideband colors

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 20, 2024, 23:52 UTC
Message-ID: <xmqqv7zu3g6c.fsf@gitster.g>
In-Reply-To: <5d09959b6426e53a68e1bce547f9507bdf21bcde.1724159575.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 20 quoted lines

> We read a bunch of configs in `use_sideband_colors()` to configure the
> colors that Git should use. We never free the strings read from the
> config though, causing memory leaks. Fix those.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  sideband.c                          | 8 +++++---
>  t/t5409-colorize-remote-messages.sh | 1 +
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/sideband.c b/sideband.c
> index 5d8907151fe..deb6ec0a8b7 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -30,7 +30,7 @@ static int use_sideband_colors(void)
>  
>  	const char *key = "color.remote";
>  	struct strbuf sb = STRBUF_INIT;
> -	char *value;
> +	char *value = NULL;

Hmph, this is a bit unfortunate. If there is no configuration variable, on the first call to this function, we come to the end of if/else cascade and we need to free.

Another possibility to convey the intention better may be to assign NULL to value after the "we already know the value, so return early" took place.

Show 5 quoted lines

> @@ -43,15 +43,17 @@ static int use_sideband_colors(void)
>  	} else {
>  		use_sideband_colors_cached = GIT_COLOR_AUTO;
>  	}
> +	FREE_AND_NULL(value);

This can be a simple "free(value)"; I do not see the need to clear the variable at this point.

Show 9 quoted lines

>  	for (i = 0; i < ARRAY_SIZE(keywords); i++) {
>  		strbuf_reset(&sb);
>  		strbuf_addf(&sb, "%s.%s", key, keywords[i].keyword);
>  		if (git_config_get_string(sb.buf, &value))
>  			continue;
> -		if (color_parse(value, keywords[i].color))
> -			continue;
> +		color_parse(value, keywords[i].color);
> +		FREE_AND_NULL(value);

Likewise. I do not see the need to clear. We only come here after we got something from gti_config_get_string(). That value may or may not fail color_parse(), but when we reach this point, value always has something we need to free, not some leftover value from the previous iteration.

The patch is _not_ wrong per-se, though.

Thanks.