Re: [PATCH v5 1/1] worktree add: sanitize worktree names

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 11, 2019, 13:05 UTC
Message-ID: <nycvar.QRO.7.76.6.1903111401220.41@tvgsbejvaqbjf.bet>
In-Reply-To: <20190308092834.12549-2-pclouds@gmail.com>

Hi Duy,

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

Show 15 quoted lines

> 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...

Show 62 quoted lines

>  
>  	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