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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 25, 2017, 02:51 UTC
Message-ID: <20170425025140.GH28740@aiede.svl.corp.google.com>
In-Reply-To: <xmqqr30hxn72.fsf@gitster.mtv.corp.google.com>

Junio C Hamano wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:

Show 9 quoted lines

>> 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.
>
> Hmph, what if you had bin/ssh/ directory and bin2/ssh executable and
> had bin:bin2 listed in this order in your $PATH?  Without this change
> you'll get an error and that's the end of it.  With this change,
> you'd be able to execute bin2/ssh executable, no?  So I am not sure
> if I agree with the "this is just an error message subtlety".

I think you misunderstood what I meant. execvp() does not have this bug. In current master, run_command() (within function sane_execvp()) double-checks execvp()'s work when it sees EACCES to decide whether to convert it into a more user-friendly ENOENT. Because of this bug, if you have a bin/ssh/ directory and no bin2/ssh executable, instead of reporting this condition as a user-friendly ENOENT, it would leave it as EACCES.

> What does execvp() do when bin/ssh/ directory, bin2/ssh
> non-executable regular file, and bin3/ssh executable file exist and
> you have bin:bin2:bin3 on your $PATH?  That is what locate_in_PATH()
> should emulate, I would think.

Good catch.

 $ mkdir -p $HOME/bin1/greet
 $ mkdir $HOME/bin2
 $ printf '%s\n' 'echo bin2' >$HOME/bin2/greet
 $ mkdir $HOME/bin3
 $ printf '%s\n' '#!/bin/sh' 'echo bin3' >$HOME/bin3/greet
 $ chmod +x $HOME/bin3/greet
 $ PATH=$HOME/bin1:$HOME/bin2:$HOME/bin3:$PATH perl -e 'exec("greet")'
 bin3

It needs to skip over non-executable files.

I think this means we'd want to reuse something like is_executable from help.c.

[...]

Show 7 quoted lines

>>> +		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 confused myself about the script special-case: they are supposed to have the executable bit set, too --- the special-casing is just about lacking #!/bin/sh at the top (and hence not being directly executable with execve).

Show 6 quoted lines

>> 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.
>
> When we are looking for "ssh" with locate_in_PATH(), shouldn't we
> look for "ssh.exe" on Windows, though?

Fortunately this is in a #if !defined(GIT_WINDOWS_NATIVE) block. It's probably worth adding a comment so people know not to rely on it matching Windows path search behavior.

Thanks for looking it over, Jonathan