Re: [PATCH v8 2/2] run-command: restrict PATH search to executable files

[Jonathan Nieder <jrnieder@gmail.com>]

Brandon Williams wrote:

> In some situations run-command will incorrectly try (and fail) to
> execute a directory instead of an executable file.  This was observed by
> having a directory called "ssh" in $PATH before the real ssh and trying
> to use ssh protoccol, reslting in the following:
>
> 	$ git ls-remote ssh://url
> 	fatal: cannot exec 'ssh': Permission denied
>
> It ends up being worse and run-command will even try to execute a
> non-executable file if it preceeds the executable version of a file on
> the PATH.  For example, if PATH=~/bin1:~/bin2:~/bin3 and there exists a
> directory 'git-hello' in 'bin1', a non-executable file 'git-hello' in
> bin2 and an executable file 'git-hello' (which prints "Hello World!") in
> bin3 the following will occur:
>
> 	$ git hello
> 	fatal: cannot exec 'git-hello': Permission denied
>
> This is due to only checking 'access()' when locating an executable in
> PATH, which doesn't distinguish between files and directories.  Instead
> use 'is_executable()' which check that the path is to a regular,
> executable file.  Now run-command won't try to execute the directory or
> non-executable file 'git-hello':
>
> 	$ git hello
> 	Hello World!
>
> Reported-by: Brian Hatfield <bhatfield@google.com>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  run-command.c          | 19 ++++++++++++++++++-
>  t/t0061-run-command.sh | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 1 deletion(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.  Patch left unsnipped for reference.

> diff --git a/run-command.c b/run-command.c
> index 2ffbd7e67..9e36151bf 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -159,6 +159,23 @@ int is_executable(const char *name)
>  	return st.st_mode & S_IXUSR;
>  }
>  
> +/*
> + * Search $PATH for a command.  This emulates the path search that
> + * execvp would perform, without actually executing the command so it
> + * can be used before fork() to prepare to run a command using
> + * execve() or after execvp() to diagnose why it failed.
> + *
> + * The caller should ensure that file contains no directory
> + * separators.
> + *
> + * Returns the path to the command, as found in $PATH or NULL if the
> + * command could not be found.  The caller inherits ownership of the memory
> + * used to store the resultant path.
> + *
> + * This should not be used on Windows, where the $PATH search rules
> + * are more complicated (e.g., a search for "foo" should find
> + * "foo.exe").
> + */
>  static char *locate_in_PATH(const char *file)
>  {
>  	const char *p = getenv("PATH");
> @@ -179,7 +196,7 @@ static char *locate_in_PATH(const char *file)
>  		}
>  		strbuf_addstr(&buf, file);
>  
> -		if (!access(buf.buf, F_OK))
> +		if (is_executable(buf.buf))
>  			return strbuf_detach(&buf, NULL);
>  
>  		if (!*end)
> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index 98c09dd98..e4739170a 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -37,6 +37,36 @@ test_expect_success !MINGW 'run_command can run a script without a #! line' '
>  	test_cmp empty err
>  '
>  
> +test_expect_success 'run_command does not try to execute a directory' '
> +	test_when_finished "rm -rf bin1 bin2" &&
> +	mkdir -p bin1/greet bin2 &&
> +	write_script bin2/greet <<-\EOF &&
> +	cat bin2/greet
> +	EOF
> +
> +	PATH=$PWD/bin1:$PWD/bin2:$PATH \
> +		test-run-command run-command greet >actual 2>err &&
> +	test_cmp bin2/greet actual &&
> +	test_cmp empty err
> +'
> +
> +test_expect_success POSIXPERM 'run_command passes over non-executable file' '
> +	test_when_finished "rm -rf bin1 bin2" &&
> +	mkdir -p bin1 bin2 &&
> +	write_script bin1/greet <<-\EOF &&
> +	cat bin1/greet
> +	EOF
> +	chmod -x bin1/greet &&
> +	write_script bin2/greet <<-\EOF &&
> +	cat bin2/greet
> +	EOF
> +
> +	PATH=$PWD/bin1:$PWD/bin2:$PATH \
> +		test-run-command run-command greet >actual 2>err &&
> +	test_cmp bin2/greet actual &&
> +	test_cmp empty err
> +'
> +
>  test_expect_success POSIXPERM 'run_command reports EACCES' '
>  	cat hello-script >hello.sh &&
>  	chmod -x hello.sh &&