Re: [PATCH] parse-options-cb.c: use string_list_append_nodup in OPT_STRING_LIST()

[Duy Nguyen <pclouds@gmail.com>]

On Mon, Jun 13, 2016 at 5:03 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Jun 10, 2016 at 06:57:26PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> If the given string list has strdup_strings set (*), the string will be
>> duplicated again. Pointless and leak memory. Ignore that flag.
>>
>> (*) only interpret-trailers.c does it at the moment
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  parse-options-cb.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/parse-options-cb.c b/parse-options-cb.c
>> index 239898d..8a1b6e6 100644
>> --- a/parse-options-cb.c
>> +++ b/parse-options-cb.c
>> @@ -144,7 +144,7 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
>>       if (!arg)
>>               return -1;
>>
>> -     string_list_append(v, xstrdup(arg));
>> +     string_list_append_nodup(v, xstrdup(arg));
>
> Hmm. So I agree this is an improvement, in the sense that we are
> double-allocating when v->strdup_strings is set.  But I think there's a
> deeper issue here. Why are we always allocating in the first place?
>
> If the memory we are getting in "arg" is not stable, then we _do_ need
> to make a copy of it. But in that case, we want "strdup_strings" to be
> set; without it any time we later run string_list_clear(), we leak the
> allocated memory, because the struct has no idea that it is the owner of
> the memory (and we do call string_list_clear() when we see "--no-foo").
>
> If the memory _is_ stable, then we are fine to add a direct reference to
> it, and can lose the extra xstrdup() here. Only the caller knows for
> sure, so we should be respecting their value of strdup_strings (so lose
> the xstrdup, but keep calling string_list_append()).
>
> In practice, I suspect the memory _is_ stable, because we are generally
> parsing command-line arguments. But it does not hurt to stay on the
> conservative side, and always make a copy (in case we are parsing
> something besides the global argv array) . Apparently I am the original
> author of this code, in c8ba163 (parse-options: add OPT_STRING_LIST
> helper, 2011-06-09), but there's no mention of this point there, in the
> list archives, or in my brain.
>
> So if we are doing the conservative thing, then I think the resulting
> code should either look like:
>
>   if (!v->strdup_strings)
>         die("BUG: OPT_STRING_LIST should always use strdup_strings");
>   string_list_append(v, arg);

I agree with the analysis. But this die() would hit all callers
(except interpret-trailers) because they all initialize with _NODUP
and setting strdup_strings may require auditing all access to the
string list in question, e.g. to change string_list_append(v,
xstrdup(xxx)) to string_list_append(xxx). it may cause side effects if
we are not careful.

So far all callers are in builtin/, I think it will not take much time
to verify that they all call parse_options() with global argv, then we
can just lose extra xstrdup() and stick to string_list_append().
OPTION_STRING already assumes that argument strings are stable because
they are passed back as-is. Can we go with an easier route, adding a
comment on top of parse_options() stating that argv[] pointers may be
passed back as-is and it's up to the caller to xstrdup() appropriately
before argv[] memory is freed?

>
> or:
>
>   /* silently enable for convenience */
>   v->strdup_strings = 1;
>   string_list_append(v, arg);
>
> Of the two, I like the top one as it is less magical, but it would
> require adjusting the initialization of the string-list for most of the
> callers.
>
> -Peff



-- 
Duy