Hi Duy,

On Fri, 8 Mar 2019, Nguyễn Thái Ngọc Duy wrote:

> diff --git a/refs.c b/refs.c
> index 142888a40a..e9f83018f0 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -72,30 +72,57 @@ static unsigned char refname_disposition[256] = {
>   * - it ends with ".lock", or
>   * - it contains a "@{" portion
>   */
> -static int check_refname_component(const char *refname, int *flags)
> +static int check_refname_component(const char *refname, int *flags,
> +                                struct strbuf *sanitized)
>  {
>       const char *cp;
>       char last = '\0';
> +     size_t component_start;

This variable is uninitialized. It is then...

> +
> +     if (sanitized)
> +             component_start = sanitized->len;

... initialized only when `sanitized` is not `NULL`, and subsequently...

>  
>       for (cp = refname; ; cp++) {
>               int ch = *cp & 255;
>               unsigned char disp = refname_disposition[ch];
> +
> +             if (sanitized && disp != 1)
> +                     strbuf_addch(sanitized, ch);
> +
>               switch (disp) {
>               case 1:
>                       goto out;
>               case 2:
> -                     if (last == '.')
> -                             return -1; /* Refname contains "..". */
> +                     if (last == '.') { /* Refname contains "..". */
> +                             if (sanitized)
> +                                     sanitized->len--; /* collapse ".." to 
> single "." */
> +                             else
> +                                     return -1;
> +                     }
>                       break;
>               case 3:
> -                     if (last == '@')
> -                             return -1; /* Refname contains "@{". */
> +                     if (last == '@') { /* Refname contains "@{". */
> +                             if (sanitized)
> +                                     sanitized->buf[sanitized->len-1] = '-';
> +                             else
> +                                     return -1;
> +                     }
>                       break;
>               case 4:
> -                     return -1;
> +                     /* forbidden char */
> +                     if (sanitized)
> +                             sanitized->buf[sanitized->len-1] = '-';
> +                     else
> +                             return -1;
> +                     break;
>               case 5:
> -                     if (!(*flags & REFNAME_REFSPEC_PATTERN))
> -                             return -1; /* refspec can't be a pattern */
> +                     if (!(*flags & REFNAME_REFSPEC_PATTERN)) {
> +                             /* refspec can't be a pattern */
> +                             if (sanitized)
> +                                     sanitized->buf[sanitized->len-1] = '-';
> +                             else
> +                                     return -1;
> +                     }
>  
>                       /*
>                        * Unset the pattern flag so that we only accept
> @@ -109,26 +136,48 @@ static int check_refname_component(const char *refname, 
> int *flags)
>  out:
>       if (cp == refname)
>               return 0; /* Component has zero length. */
> -     if (refname[0] == '.')
> -             return -1; /* Component starts with '.'. */
> +
> +     if (refname[0] == '.') { /* Component starts with '.'. */
> +             if (sanitized)
> +                     sanitized->buf[component_start] = '-';

... used a loooooooong time after that, also only if `sanitized` is not
`NULL`.

Apparently for some GCC versions, this is too cute, and it complains that
this variable might be used uninitialized:
https://dev.azure.com/gitgitgadget/git/_build/results?buildId=4352&view=logs

And quite honestly, even for mere humans it is not all *that* clear that
`sanitized` cannot be changed from `NULL` to non-`NULL` in the code in
between, *in particular* because the changes extend over two hunks, the
code between is not shown.

I would strongly advise against trying to be so cute, and just initialize
the variable already. Over-optimization in such instances makes the code a
lot harder to reason about.

Ciao,
Johannes

Reply via email to