Re: [PATCH v2 13/21] config: plug various memory leaks

[Jeff King <peff@peff.net>]

On Fri, May 24, 2024 at 12:04:12PM +0200, Patrick Steinhardt wrote:

> diff --git a/alias.c b/alias.c
> index 269892c356..4daafd9bda 100644
> --- a/alias.c
> +++ b/alias.c
> @@ -21,9 +21,11 @@ static int config_alias_cb(const char *key, const char *value,
>  		return 0;
>  
>  	if (data->alias) {
> -		if (!strcasecmp(p, data->alias))
> +		if (!strcasecmp(p, data->alias)) {
> +			FREE_AND_NULL(data->v);
>  			return git_config_string(&data->v,
>  						 key, value);
> +		}
>  	} else if (data->list) {
>  		string_list_append(data->list, p);
>  	}

IMHO this should be done automatically by git_config_string(). The
current design is an accident waiting to happen, and in the long run
every call is going to need this FREE_AND_NULL(). By doing it in the
function the calling code is shorter, and there's no way we'll forget.

I posted a series along those lines a month or so ago:

  https://lore.kernel.org/git/20240407005656.GA436890@coredump.intra.peff.net/

The catch is that you can't do this:

  const char *foo = "bar";
  git_config_string(&foo, ...);

So I introduced a new function that took a non-const pointer with the
new behavior, with the idea that we'd eventually migrate everything
over. It looks like you may have already done that migration earlier in
your series, since the move to "char *" in the previous patch was OK.

  Though as a side note, sadly:

    char *foo = "bar";

  does not produce an error or even a warning without -Wwrite-strings. I
  think in the long run we should enable that, but there's a little
  cleanup required to do so.

The main reason I didn't follow up on that earlier series is that there
was some discussion about maybe moving this stuff over to strbufs (after
teaching it to handle literal initializers). But if you've managed to
remove all of the cases that needed that, I think just sticking with
"char *" is fine.

The other issue raised in that thread is that many of these config
variables are also passed to parse-options, which treats them as const
strings (and we get no compiler support because it goes through a void
pointer). So they may leak if we overwrite them, or in the unusual case
that we load config after parsing options, we may try to free a non-heap
string. The one we discussed was log's signature_file, and it looks like
you split that to use two variables, which works. Junio suggested an
OPT_FILENAME_DUP() option, which I'm also OK with. The main challenge to
me is being sure we found all such spots (and not accidentally
introducing new ones). But I don't have a good solution there.

> @@ -1566,7 +1569,7 @@ static int git_default_core_config(const char *var, const char *value,
>  
>  	if (!strcmp(var, "core.checkroundtripencoding")) {
>  		FREE_AND_NULL(check_roundtrip_encoding);
> -		return git_config_string((const char **) &check_roundtrip_encoding, var, value);
> +		return git_config_string(&check_roundtrip_encoding, var, value);
>  	}

This should have lost its cast in the previous commit, no? Applying up
to patch 12 and building with DEVELOPER=1 gets a warning.

-Peff