git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v6 0/1] http: Add Accept-Language header if possible

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Jan 19, 2015, 20:21 UTC
Message-ID
<CAPig+cSrX=mpFWznRtYgQOsi7YU7ewEo6VqMmkq9OSiveG961Q@mail.gmail.com>
In-Reply-To
<1421583995-3663-1-git-send-email-eungjun.yi@navercorp.com>
On Sunday, January 18, 2015, Yi EungJun <semtlenori@gmail.com> wrote:
Show 34 quoted lines
> Add an Accept-Language header which indicates the user's preferred
> languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.
>
> Examples:
>   LANGUAGE= -> ""
>   LANGUAGE=ko:en -> "Accept-Language: ko, en;q=0.9, *;q=0.1"
>   LANGUAGE=ko LANG=en_US.UTF-8 -> "Accept-Language: ko, *;q=0.1"
>   LANGUAGE= LANG=en_US.UTF-8 -> "Accept-Language: en-US, *;q=0.1"
>
> This gives git servers a chance to display remote error messages in
> the user's preferred language.
>
> Limit the number of languages to 1,000 because q-value must not be
> smaller than 0.001, and limit the length of Accept-Language header to
> 4,000 bytes for some HTTP servers which cannot accept such long header.
>
> Signed-off-by: Yi EungJun <eungjun.yi@navercorp.com>
> ---
> diff --git a/http.c b/http.c
> index 040f362..349b033 100644
> --- a/http.c
> +++ b/http.c
> @@ -986,6 +993,145 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,
>                 strbuf_addstr(charset, "ISO-8859-1");
>  }
>
> +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;
> +       int num_langs;
Mental note: 'num_langs' is not initialized.
Show 13 quoted lines
> +       const char *s = get_preferred_languages();
> +
> +       /* Don't add Accept-Language header if no language is preferred. */
> +       if (!s)
> +               return;
> +
> +       /*
> +        * Split the colon-separated string of preferred languages into
> +        * language_tags array.
> +        */
> +       do {
> +               /* increase language_tags array to add new language tag */
> +               REALLOC_ARRAY(language_tags, num_langs + 1);
'num_langs' is used here uninitialized.
Show 11 quoted lines
> +               strbuf_init(&language_tags[num_langs], 0);
> +
> +               /* collect language tag */
> +               for (; *s && (isalnum(*s) || *s == '_'); s++)
> +                       strbuf_addch(&language_tags[num_langs], *s == '_' ? '-' : *s);
> +
> +               /* skip .codeset, @modifier and any other unnecessary parts */
> +               while (*s && *s != ':')
> +                       s++;
> +
> +               if (language_tags[num_langs].len > 0) {
Mental note: An empty ("") language tag is never allowed in language_tags[].
 > +                       num_langs++;

This is a little bit ugly. At the top of the loop, you allocate space in the array for a strbuf and initialize it. However, if the language tag is empty (""), then 'num_langs' is never incremented, so the next time through the loop, strbuf_init() is invoked on the same block of memory (assuming the realloc was a no-op since the allocation size did not change), overwriting whatever was there and possibly leaking memory. In this particular case, by examining the parser code closely, we can see that nothing was added to the strbuf, so nothing is being leaked the next time around, given the current implementation of strbuf.

However, this is potentially fragile. A change to the implementation of strbuf in the future (for instance, if strbuf_init() allocates memory immediately) could result in a leak here. Moreover, this no-leak situation only holds true if no text at all has been added to the strbuf after strbuf_init(). If someone changes the parser in the future to operate a bit differently so that some text is added and then removed from the strbuf, even though the end result still has length is 0, then it will start leaking.

One way to make this more robust would be to have a separate strbuf for collecting the language tag. When you encounter a non-empty tag, only then grow the array and initialize the new strbuf in the array. Finally, use strbuf_swap() to swap the collected language tag into the new array position. Something like this:

    struct strbuf tag = STRBUF_INIT;
    do {
         for (; *s && (isalnum(*s) || *s == '_'); s++)
             strbuf_addch(&tag, *s == '_' ? '-' : *s);
        [...]
        if (tag.len) {
            num_langs++;
            REALLOC_ARRAY(language_tags, num_langs);
            strbuf_init(&language_tags[num_langs], 0);
            strbuf_swap(&tag, &language_tags[num_langs]);
            if (num_langs >= ...)
                break;
        }
    while (...);
    strbuf_release(&tag);
Show 9 quoted lines
> +                       if (num_langs >= MAX_LANGUAGE_TAGS - 1) /* -1 for '*' */
> +                               break;
> +               }
> +       } while (*s++);
> +
> +       /* write Accept-Language header into buf */
> +       if (num_langs >= 1) {
> +               int i;
> +               int last_buf_len;
Mental note: 'last_buf_len' is not initialized.
Show 22 quoted lines
> +               int max_q;
> +               int decimal_places;
> +               char q_format[32];
> +
> +               /* add '*' */
> +               REALLOC_ARRAY(language_tags, num_langs + 1);
> +               strbuf_init(&language_tags[num_langs], 0);
> +               strbuf_addstr(&language_tags[num_langs++], "*");
> +
> +               /* compute decimal_places */
> +               for (max_q = 1, decimal_places = 0;
> +                               max_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;
> +                               decimal_places++, max_q *= 10)
> +                       ;
> +
> +               sprintf(q_format, ";q=0.%%0%dd", decimal_places);
> +
> +               strbuf_addstr(buf, "Accept-Language: ");
> +
> +               for(i = 0; i < num_langs; i++) {
> +                       if (language_tags[i].len == 0)
> +                               continue;

The parsing code does not allow empty tags ("") in language_tags[], so this conditional is useless, isn't it?

Show 5 quoted lines
> +
> +                       if (i > 0)
> +                               strbuf_addstr(buf, ", ");
> +
> +                       strbuf_addstr(buf, strbuf_detach(&language_tags[i], NULL));

This leaks the string detached from 'language_tag[i]' since strbuf_addstr() does not take ownership of it.

Show 5 quoted lines
> +                       if (i > 0)
> +                               strbuf_addf(buf, q_format, max_q - i);
> +
> +                       if (buf->len > MAX_ACCEPT_LANGUAGE_HEADER_SIZE) {
> +                               strbuf_remove(buf, last_buf_len, buf->len - last_buf_len);

'last_buf_len' is (potentially) used here uninitialized the first time through loop.

Show 8 quoted lines
> +                               break;
> +                       }
> +
> +                       last_buf_len = buf->len;
> +               }
> +       }
> +
> +       free(language_tags);

This _seems_ to be okay since strbuf_detach() was invoked for each strbuf in language_tags[], however, those strings were in fact leaked (as noted above), so it's not actually correct.

Show 6 quoted lines
> +}
> +
> +/*
> + * Get an Accept-Language header which indicates user's preferred languages.
> + *
> + * This function always return non-NULL string as strbuf_detach() does.

Repeating from [1]: It's not good form to describe the published API in terms of an implementation detail (strbuf_detach). Also, it would be more idiomatic in C to return NULL rather than empty string.

[1]: http://article.gmane.org/gmane.comp.version-control.git/261810/
Show 20 quoted lines
> + *
> + * Examples:
> + *   LANGUAGE= -> ""
> + *   LANGUAGE=ko:en -> "Accept-Language: ko, en; q=0.9, *; q=0.1"
> + *   LANGUAGE=ko_KR.UTF-8:sr@latin -> "Accept-Language: ko-KR, sr; q=0.9, *; q=0.1"
> + *   LANGUAGE=ko LANG=en_US.UTF-8 -> "Accept-Language: ko, *; q=0.1"
> + *   LANGUAGE= LANG=en_US.UTF-8 -> "Accept-Language: en-US, *; q=0.1"
> + *   LANGUAGE= LANG=C -> ""
> + */
> +static const char *get_accept_language(void)
> +{
> +       if (!cached_accept_language) {
> +               struct strbuf buf = STRBUF_INIT;
> +               write_accept_language(&buf);
> +               cached_accept_language = strbuf_detach(&buf, NULL);
> +       }
> +
> +       return cached_accept_language;
> +}
> +
Previous: Torsten BögershausenNext: Junio C Hamano
Message 20 of 46 in “http: Add Accept-Language header if possible”
  1. 0/1 http: Add Accept-Language header if possibleYi EungJun, Jul 19, 2014
  2. 1/1 http: Add Accept-Language header if possibleYi EungJun, Jul 19, 2014
  3. Junio C HamanoJul 21, 2014
  4. Yi, EungJunAug 3, 2014
  5. 0/1 http: Add Accept-Language header if possibleYi EungJun, Dec 2, 2014
  6. 1/1 http: Add Accept-Language header if possibleYi EungJun, Dec 2, 2014
  7. Junio C HamanoDec 3, 2014
  8. Eric SunshineDec 3, 2014
  9. Junio C HamanoDec 3, 2014
  10. Michael BlumeDec 3, 2014
  11. Michael BlumeDec 3, 2014
  12. 0/1 http: Add Accept-Language header if possibleYi EungJun, Dec 22, 2014
  13. 1/1 http: Add Accept-Language header if possibleYi EungJun, Dec 22, 2014
  14. Junio C HamanoDec 22, 2014
  15. Eric SunshineDec 24, 2014
  16. Junio C HamanoDec 29, 2014
  17. 0/1 http: Add Accept-Language header if possibleYi EungJun, Jan 18, 2015
  18. 1/1 http: Add Accept-Language header if possibleYi EungJun, Jan 18, 2015
  19. Torsten BögershausenJan 18, 2015
  20. Eric SunshineJan 19, 2015
  21. Junio C HamanoJan 22, 2015
  22. 0/1 http: Add Accept-Language header if possibleYi EungJun, Jan 27, 2015
  23. http: Add Accept-Language header if possibleYi EungJun, Jan 27, 2015
  24. Junio C HamanoJan 27, 2015
  25. Junio C HamanoJan 28, 2015
  26. Yi, EungJunJan 28, 2015
  27. 0/1 http: Add Accept-Language header if possibleYi EungJun, Jan 28, 2015
  28. 1/1 http: Add Accept-Language header if possibleYi EungJun, Jan 28, 2015
  29. Junio C HamanoFeb 25, 2015
  30. Jeff KingFeb 26, 2015
  31. Jeff KingFeb 26, 2015
  32. Junio C HamanoFeb 26, 2015
  33. Jeff KingFeb 26, 2015
  34. Junio C HamanoFeb 26, 2015
  35. Stefan BellerFeb 26, 2015
  36. Jeff KingFeb 26, 2015
  37. Jeff KingFeb 26, 2015
  38. Stefan BellerFeb 26, 2015
  39. Jeff KingFeb 26, 2015
  40. Jeff KingFeb 26, 2015
  41. Junio C HamanoFeb 26, 2015
  42. Junio C HamanoFeb 26, 2015
  43. Junio C HamanoJan 29, 2015
  44. Yi, EungJunJan 30, 2015
  45. http: Include locale.h when using setlocale()Ævar Arnfjörð Bjarmason, Mar 6, 2015
  46. Junio C HamanoMar 6, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.