On Fri, Apr 01, 2016 at 10:03:25AM -0700, Junio C Hamano wrote:
> -- >8 --
> From: Stefan Beller <sbeller@xxxxxxxxxx>
> 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:
> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html