Re: [PATCHv4 1/4] notes: don't leak memory in git_config_get_notes_strategy

From: Jeff King <peff@peff.net>
Date: Apr 1, 2016, 17:14 UTC
Message-ID: <20160401171424.GA20345@sigill.intra.peff.net>
In-Reply-To: <xmqq4mbljm76.fsf@gitster.mtv.corp.google.com>

On Fri, Apr 01, 2016 at 10:03:25AM -0700, Junio C Hamano wrote:

Show 17 quoted lines

> -- >8 --
> From: Stefan Beller <sbeller@google.com>
> Date: Thu, 31 Mar 2016 11:04:03 -0700
> Subject: [PATCH] notes: don't leak memory in git_config_get_notes_strategy
> 
> This function asks for the value of a configuration and
> after using the value does not have to retain ownership
> of the value.  git_config_get_string_const() however is
> a function to get a copy of the value, but we forget to
> free it before we return.
> 
> Because we only need to peek the value without retaining
> a pointer to it, use git_config_get_value() to peek into
> the value cached in the config API layer.
> 
> As git_config_get_value() does not insist the value to be
> a string, we'd need to do the "nonbool" check ourselves.

Unfortunately, I don't think this is quite right. In the original, we relied on git_config_get_string_const to notice a non-string value, at which point it would die:

  $ git -c notes.mergeStrategy notes merge whatever
  error: Missing value for 'notes.mergeStrategy'
  fatal: unable to parse 'notes.mergeStrategy' from command-line config

But in your patch:

Show 11 quoted lines

> @@ -743,8 +743,10 @@ static int git_config_get_notes_strategy(const char *key,
>  {
>  	const char *value;
>  
> -	if (git_config_get_string_const(key, &value))
> +	if (git_config_get_value(key, &value))
>  		return 1;
> +	if (!value)
> +		return config_error_nonbool(key);
>  	if (parse_notes_merge_strategy(value, strategy))
>  		git_die_config(key, "unknown notes merge strategy %s", value);

We just return an error from git_config_get_notes_strategy(). If this were a callback to git_config(), that would be fine (as we would auto-die then in the caller), but it's not. It is called directly for a specific key. One of the callers treats a non-zero return as "we don't have that variable", and the other ignores the return value completely.

So I think you'd want something more like:

  if (!value) {
	config_error_nonbool(key);
	git_die_config(key);
  }

That keeps the original message intact (though it is a bit verbose in the first place).

This is why I wondered if the minor "do not allocate" tweak was worth the trouble, when git_config_get_string() just handles this for us.

-Peff