Patrick Steinhardt <ps@xxxxxx> writes:
> 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@xxxxxx>
> ---
> 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.
> @@ -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.
> 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.
