On Fri, Apr 1, 2016 at 1:14 PM, Jeff King <peff@xxxxxxxx> wrote:
> On Fri, Apr 01, 2016 at 10:03:25AM -0700, Junio C Hamano wrote:
>> 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.
Nicer commit message.
> 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:
>
>> + if (!value)
>> + return config_error_nonbool(key);
>
> 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);
> }
Yep, I had noted the bit about having to die() manually when reviewing
the earlier patch, but it slipped from memory when composing the reply
to the current version of the patch.
> 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.
Which again suggests a new function which does this work but doesn't
copy the string (despite the already quite large set of similar
functions).
--
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