Re: [PATCH] run-command: don't try to execute directories

[cmn@elego.de (Carlos Martín Nieto)]

Junio C Hamano <gitster@pobox.com> writes:

> Carlos Martín Nieto <cmn@elego.de> writes:
>
>> When looking through $PATH to try to find an external command,
>> locate_in_PATH doesn't check that it's trying to execute a file. Add a
>> check to make sure we won't try to execute a directory.
>>
>> This also stops us from looking further and maybe finding that the
>> user meant an alias, as in the case where the user has
>> /home/user/bin/git-foo/git-foo.pl and an alias
>>
>>     [alias] foo = !/home/user/bin/git-foo/git-foo.pl
>>
>> Running 'git foo' will currently will try to execute ~/bin/git-foo and
>> fail because you can't execute a directory. By making sure we don't do
>> that, we realise that it's an alias and do the right thing
>>
>> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
>>
>> ---
>>
>> This comes from a case in #git. Not sure if this is worth it, or the
>> better solution is just to say no to dirs in $PATH.
>>
>> After writing all of that, I thought to check the shell, and indeed
>>
>>     % git-foo
>>     zsh: permission denied: git-foo
>>
>> so if the shell doesn't do it, the benefits probably don't outweigh
>> having a dozen stat instead of access calls. strace reveals that zsh
>> does what git currently does. bash uses stat and says 'command not
>> found'.
>
> Hrm, I do not use zsh but it does not seem to reproduce for me.
>
> 	$ mkdir -p /var/tmp/xx/git
>         $ zsh
>         % PATH=/var/tmp/xx:$PATH
>         % type git
>         git is /home/junio/bin/git
>         % git version
>         git version 1.8.0.rc0.45.g7ce8dc5
> 	% zsh --version
> 	zsh 4.3.10 (x86_64-unknown-linux-gnu)

zsh has some quite aggressive PATH caching. I did this with git-foo in
the path so it didn't already know what to look for. I can reproduce
what you saw, but also consider this:

    % /var/tmp/xx/git
    zsh: permission denied: /var/tmp/xx/git
    % zsh --version
    zsh 4.3.17 (x86_64-unknown-linux-gnu)

If you change your test to use git-foo instead of just git, you should
see what I wrote in the message.

bash rightfully complains that it's a stupid thing to do.

    $ /var/tmp/xx/git
    bash: /var/tmp/xx/git: Is a directory

>
>> @@ -101,8 +102,9 @@ static char *locate_in_PATH(const char *file)
>>  		}
>>  		strbuf_addstr(&buf, file);
>>  
>> -		if (!access(buf.buf, F_OK))
>> +		if (!stat(buf.buf, &st) && !S_ISDIR(st.st_mode)) {
>>  			return strbuf_detach(&buf, NULL);
>> +		}
>
> So we used to say "if it exists and accessible, return that".  Now
> we say "if it exists and is not a directory, return that".
>
> I have to wonder what would happen if it exists as a non-directory
> but we cannot access it.  Is that a regression?

I guess it would be, yeah. Would this be related to tha situation where
the user isn't allowed to access something in their PATH?

How about something like this instead? We keep the access check and only
do the stat call when we have found something we want to look at.

   cmn

---8<---

diff --git a/run-command.c b/run-command.c
index 1101ef7..fb8a93c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -85,6 +85,7 @@ static char *locate_in_PATH(const char *file)
 {
        const char *p = getenv("PATH");
        struct strbuf buf = STRBUF_INIT;
+       struct stat st;
 
        if (!p || !*p)
                return NULL;
@@ -101,7 +102,8 @@ static char *locate_in_PATH(const char *file)
                }
                strbuf_addstr(&buf, file);
 
-               if (!access(buf.buf, F_OK))
+               if (!access(buf.buf, F_OK) &&
+                   !stat(buf.buf, &st) && !S_ISDIR(st.st_mode))
                        return strbuf_detach(&buf, NULL);
 
                if (!*end)