Re: [PATCH] http: Add Accept-Language header if possible

> On Tue, Jan 27, 2015 at 3:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Yi EungJun <semtlenori@gmail.com> writes:
>>
>>> +
>>> +             sprintf(q_format, ";q=0.%%0%dd", decimal_places);
>>> +
>>> +             strbuf_addstr(buf, "Accept-Language: ");
>>> +
>>> +             for(i = 0; i < num_langs; i++) {
>>> +                     if (i > 0)
>>> +                             strbuf_addstr(buf, ", ");
>>> +
>>> +                     strbuf_addstr(buf, strbuf_detach(&language_tags[i], NULL));
>>
>> This is not wrong per-se, but it looks somewhat convoluted to me.
>> ...
>
> Actually, this is wrong, isn't it?
>
> strbuf_detach() removes the language_tags[i].buf from the strbuf,
> and the caller now owns that piece of memory. Then strbuf_addstr()
> appends a copy of that string to buf, and the piece of memory
> that was originally held by language_tags[i].buf is now lost forever.
>
> This is leaking.
>
>>> +     /* free language tags */
>>> +     for(i = 0; i < num_langs; i++) {
>>> +             strbuf_release(&language_tags[i]);
>>> +     }
>
> ... because this loop does not free memory for earlier parts of language_tags[].
>
>> I am wondering if using strbuf for each of the language_tags[] is
>> even necessary.  How about doing it this way instead?
>
> And I think my counter-proposal does not leak (as it does not us strbuf for
> language_tags[] anymore).
>
>>
>>  http.c | 22 +++++++++-------------
>>  1 file changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/http.c b/http.c
>> index 6111c6a..db591b3 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -1027,7 +1027,7 @@ static void write_accept_language(struct strbuf *buf)
>>         const int MAX_DECIMAL_PLACES = 3;
>>         const int MAX_LANGUAGE_TAGS = 1000;
>>         const int MAX_ACCEPT_LANGUAGE_HEADER_SIZE = 4000;
>> -       struct strbuf *language_tags = NULL;
>> +       char **language_tags = NULL;
>>         int num_langs = 0;
>>         const char *s = get_preferred_languages();
>>         int i;
>> @@ -1053,9 +1053,7 @@ static void write_accept_language(struct strbuf *buf)
>>                 if (tag.len) {
>>                         num_langs++;
>>                         REALLOC_ARRAY(language_tags, num_langs);
>> -                       strbuf_init(&language_tags[num_langs - 1], 0);
>> -                       strbuf_swap(&tag, &language_tags[num_langs - 1]);
>> -
>> +                       language_tags[num_langs - 1] = strbuf_detach(&tag, NULL);
>>                         if (num_langs >= MAX_LANGUAGE_TAGS - 1) /* -1 for '*' */
>>                                 break;
>>                 }
>> @@ -1070,13 +1068,12 @@ static void write_accept_language(struct strbuf *buf)
>>
>>                 /* add '*' */
>>                 REALLOC_ARRAY(language_tags, num_langs + 1);
>> -               strbuf_init(&language_tags[num_langs], 0);
>> -               strbuf_addstr(&language_tags[num_langs++], "*");
>> +               language_tags[num_langs++] = "*"; /* it's OK; this won't be freed */
>>
>>                 /* compute decimal_places */
>>                 for (max_q = 1, decimal_places = 0;
>> -                               max_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;
>> -                               decimal_places++, max_q *= 10)
>> +                    max_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;
>> +                    decimal_places++, max_q *= 10)
>>                         ;
>>
>>                 sprintf(q_format, ";q=0.%%0%dd", decimal_places);
>> @@ -1087,7 +1084,7 @@ static void write_accept_language(struct strbuf *buf)
>>                         if (i > 0)
>>                                 strbuf_addstr(buf, ", ");
>>
>> -                       strbuf_addstr(buf, strbuf_detach(&language_tags[i], NULL));
>> +                       strbuf_addstr(buf, language_tags[i]);
>>
>>                         if (i > 0)
>>                                 strbuf_addf(buf, q_format, max_q - i);
>> @@ -1101,10 +1098,9 @@ static void write_accept_language(struct strbuf *buf)
>>                 }
>>         }
>>
>> -       /* free language tags */
>> -       for(i = 0; i < num_langs; i++) {
>> -               strbuf_release(&language_tags[i]);
>> -       }
>> +       /* free language tags -- last one is a static '*' */
>> +       for(i = 0; i < num_langs - 1; i++)
>> +               free(language_tags[i]);
>>         free(language_tags);
>>  }
>>