Re: [PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks

From: Jeff King <peff@peff.net>
Date: Nov 8, 2023, 17:21 UTC
Message-ID: <20231108172125.GD1028115@coredump.intra.peff.net>
In-Reply-To: <361f1bd9c88e3e6b7b135ba67b39d3bf4d70e322.1699455383.git.ps@pks.im>

On Wed, Nov 08, 2023 at 03:57:27PM +0100, Patrick Steinhardt wrote:

Show 16 quoted lines

> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
> index c8e6c0733f4..3b64022dc57 100755
> --- a/t/t9164-git-svn-dcommit-concurrent.sh
> +++ b/t/t9164-git-svn-dcommit-concurrent.sh
> @@ -47,9 +47,15 @@ setup_hook()
>  	echo "cnt=$skip_revs" > "$hook_type-counter"
>  	rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
>  	hook="$rawsvnrepo/hooks/$hook_type"
> -	cat > "$hook" <<- 'EOF1'
> +	cat >"$hook" <<-EOF
>  		#!/bin/sh
>  		set -e
> +
> +		PATH=\"$PATH\"
> +		export PATH
> +	EOF

The quoting here is weird. The original escaped the double-quotes because it was echo-ing and interpolated string:

> -		echo "PATH=\"$PATH\"; export PATH" >>"$hook"

But the here-doc interpolation does not treat double-quotes as special, and you end up with actual double-quote characters at the beginning and end of your $PATH variable, wrecking those entries (but I guess it works some of the time depending on whether those parts of your $PATH are important).

It also interpolates PATH while making the here-doc. That's what you want to convince the hook's inner shell to use the same PATH as the outer shell, but it also means that the inner shell will parse and evaluate it. I don't think anyone is going to inject '"; rm -rf /' into their own test runs, but it does mean we'd do the wrong thing if their $PATH contains double-quotes or backslashes.

Certainly the original suffered from the same issue, so it may not be worth worrying about. But the more robust way to do it is:

  # export ORIG_PATH, which presumably is not cleared the same way PATH
  # is, so that the hook can access it
  ORIG_PATH=$PATH &&
  export ORIG_PATH &&

  cat >"$hook" <<EOF
  # pull the original path from the caller
  PATH=$ORIG_PATH
  export PATH

  ...do other stuff...
  EOF

That's assuming that environment variables make it intact to the hook at all (it is not clear to me why the original $PATH doesn't). If they don't, you'd probably have to stash it in a file like:

  echo "$PATH" >orig-path
  cat >"$hook" <<EOF
  PATH="$(cat orig-path)"
  export PATH
  EOF

-Peff