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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 26, 2017, 01:44 UTC
Message-ID: <xmqqzif4t01f.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170425234700.194277-2-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 26 quoted lines

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