On Tue, Jan 27, 2015 at 3:34 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Yi EungJun <semtlenori@xxxxxxxxx> 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);
> }
>
--
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