Re: [PATCH 0/1] quote: quote space

From: Jeff King <peff@peff.net>
Date: Mar 27, 2024, 09:17 UTC
Message-ID: <20240327091742.GA847537@coredump.intra.peff.net>
In-Reply-To: <xmqqfrwlltjn.fsf@gitster.g>

On Tue, Mar 19, 2024 at 03:56:44PM -0700, Junio C Hamano wrote:

Show 9 quoted lines

> Unfortunately, this loop can terminate prematurely when a crafted
> directory name ended with a SP.  The next pathname component after
> that SP (i.e. the beginning of the possible postimage filename) will
> be a slash, and instead of rejecting that position as the valid
> separation point between pre- and post-image filenames and keep
> looping, we stopped processing right there.
> 
> The fix is simple.  Instead of stopping and giving up, keep going on
> when we see such a condition.

That makes sense, but leaves me with only one question...

Show 14 quoted lines

> @@ -1292,8 +1292,15 @@ static char *git_header_name(int p_value,
>  				return NULL; /* no postimage name */
>  			second = skip_tree_prefix(p_value, name + len + 1,
>  						  line_len - (len + 1));
> +			/*
> +			 * If we are at the SP at the end of a directory,
> +			 * skip_tree_prefix() may return NULL as that makes
> +			 * it appears as if we have an absolute path.
> +			 * Keep going to find another SP.
> +			 */
>  			if (!second)
> -				return NULL;
> +				continue;
> +

If we saw a NULL from skip_tree_prefix() because it really was an absolute path, is continuing the right thing? Or put another way: will we continue to correctly reject such an absolute path, and not accidentally find a pair of names?

I think it may be OK because true absolute paths imply that the first entry would start with "/", and we would already have bailed earlier in the function. So:

  diff --git /foo /bar

will already be rejected at the start of "/foo". And in broken input like:

  diff --git a/foo /bar

we must assume that the start of "/bar" is a possible name, which is what your patch is fixing. And in broken mixed input like that, we would fail to find a valid split point, and correctly return NULL.

I guess these happen in practice with "/dev/null" as the left-hand side. But there we'd never need the names from this line, since we'd have a separate "deleted file mode ..." header line.

-Peff