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
