Re: [PATCH v6 12/11] run-command: don't try to execute directories

[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.  For example:
>
> Lets suppose a user has PATH=~/bin (where 'bin' is a directory) and they
> happen to have another directory inside 'bin' named 'git-remote-blah'.
> Then git tries to execute the directory:
>
> 	$ git ls-remote blah://blah
> 	fatal: cannot exec 'git-remote-blah': 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 'stat()' and check that the path is to a regular file.  Now
> run-command won't try to execute the directory 'git-remote-blah':
>
> 	$ git ls-remote blah://blah
> 	fatal: Unable to find remote helper for 'blah'
>
> Signed-off-by: Brandon Williams <bmwill@google.com>

For the interested, the context in which this was reported was trying
to execute a directory named 'ssh'.  Thanks for a quick fix.

Technically this bug has existed since

	commit 38f865c27d1f2560afb48efd2b7b105c1278c4b5
	Author: Jeff King <peff@peff.net>
	Date:   Fri Mar 30 03:52:18 2012 -0400

	   run-command: treat inaccessible directories as ENOENT

Until we switched from using execvp to execve, the symptom was very
subtle: it only affected the error message when a program could not be
found, instead of affecting functionality more substantially.

[...]
> --- a/run-command.c
> +++ b/run-command.c
> @@ -127,6 +127,7 @@ static char *locate_in_PATH(const char *file)
>  
>  	while (1) {
>  		const char *end = strchrnul(p, ':');
> +		struct stat st;
>  
>  		strbuf_reset(&buf);
>  
> @@ -137,7 +138,7 @@ static char *locate_in_PATH(const char *file)
>  		}
>  		strbuf_addstr(&buf, file);
>  
> -		if (!access(buf.buf, F_OK))
> +		if (!stat(buf.buf, &st) && S_ISREG(st.st_mode))
>  			return strbuf_detach(&buf, NULL);

Should this share code with help.c's is_executable()?

I suppose not, since that would have trouble finding scripts without
the executable bit set.

I was momentarily nervous about what happens if this gets run on
Windows. This is just looking for a file's existence, not
executability, so it should be fine.

> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -37,6 +37,13 @@ test_expect_success !MINGW 'run_command can run a script without a #! line' '
>  	test_cmp empty err
>  '
>  
> +test_expect_success 'run_command should not try to execute a directory' '
> +	test_when_finished "rm -rf bin/blah" &&
> +	mkdir -p bin/blah &&
> +	PATH=bin:$PATH test_must_fail test-run-command run-command blah 2>err &&

Two nits:

- this environment variable setting leaks past the test_must_fail
  invocation in some shells.  When running external comments, they
  update the environment after forking, but when running shell
  functions, they update the environment first and never set it back.

  A search with "git grep -e '=.* test_must_fail'" finds no other
  instances of this pattern, so apparently we've done a good job of
  being careful about that. *surprised*  t/check-non-portable-shell.pl
  doesn't check for this.  Perhaps it should.

  Standard workarounds:

  	(
		PATH=... &&
		export PATH &&
		test_must_fail ...
	)

  or

	test_must_fail env PATH=... ...

- using a relative path (other than '.') in $PATH feels unusual.  We
  can mimic a typical user setup more closely by using "$PWD/bin". 

> +	test_i18ngrep "No such file or directory" err

This string comes from libc.  Is there some other way to test for
what this patch does?

E.g. how about something like the following?

Thanks,
Jonathan

diff --git i/t/t0061-run-command.sh w/t/t0061-run-command.sh
index 30c4ad75ff..68cd0a8072 100755
--- i/t/t0061-run-command.sh
+++ w/t/t0061-run-command.sh
@@ -38,10 +38,16 @@ test_expect_success !MINGW 'run_command can run a script without a #! line' '
 '
 
 test_expect_success 'run_command should not try to execute a directory' '
-	test_when_finished "rm -rf bin/blah" &&
-	mkdir -p bin/blah &&
-	PATH=bin:$PATH test_must_fail test-run-command run-command blah 2>err &&
-	test_i18ngrep "No such file or directory" err
+	test_when_finished "rm -rf bin1 bin2" &&
+	mkdir -p bin1/blah &&
+	mkdir bin2 &&
+	cat hello-script >bin2/blah &&
+	chmod +x bin2/blah &&
+	PATH=$PWD/bin1:$PWD/bin2:$PATH \
+	test-run-command run-command blah >actual 2>err &&
+
+	test_cmp hello-script actual &&
+	test_cmp empty err
 '
 
 test_expect_success POSIXPERM 'run_command reports EACCES' '