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

[Junio C Hamano <gitster@pobox.com>]

Brandon Williams <bmwill@google.com> writes:

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

Could you add a line after this example, that says something like
"which matches what execvp() would have done with a request to
execute git-hello with such a $PATH."

That is because it can be argued that bin1/git-hello should be found
and get complaint "not an executable file", or that bin1/git-hello
should be skipped but bin2/git-hello should be found and get
complaint "not an executable file", both to help the user diagnose
and fix the broken $PATH (or director contents).  It is the easiest
to justify why we chose this other definition to skip both git-hello
in bin1 and bin2 if that is an established existing practice---we
can say "sure, what you propose also may make sense, but we match
what execvp(3) does".

The patch text looks good.

Thanks.