{"thread":{"id":"31713","subject":"[PATCH] run-command: don't try to execute directories","startedAt":"2012-10-02T14:46:33Z","lastAt":"2012-10-02T21:26:45Z","messageCount":5,"participants":["Carlos Martín Nieto","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"200322","messageId":"1349189193-25497-1-git-send-email-cmn@elego.de","threadId":"31713","inReplyTo":null,"subject":"[PATCH] run-command: don't try to execute directories","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-10-02T14:46:33Z","receivedAt":"2012-10-02T14:46:33Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"When looking through $PATH to try to find an external command,\nlocate_in_PATH doesn't check that it's trying to execute a file. Add a\ncheck to make sure we won't try to execute a directory.\n\nThis also stops us from looking further and maybe finding that the\nuser meant an alias, as in the case where the user has\n/home/user/bin/git-foo/git-foo.pl and an alias\n\n [alias] foo = !/home/user/bin/git-foo/git-foo.pl\n\nRunning 'git foo' will currently will try to execute ~/bin/git-foo and\nfail because you can't execute a directory. By making sure we don't do\nthat, we realise that it's an alias and do the right thing\n\nSigned-off-by: Carlos Martín Nieto \n\n---\n\nThis comes from a case in #git. Not sure if this is worth it, or the\nbetter solution is just to say no to dirs in $PATH.\n\nAfter writing all of that, I thought to check the shell, and indeed\n\n % git-foo\n zsh: permission denied: git-foo\n\nso if the shell doesn't do it, the benefits probably don't outweigh\nhaving a dozen stat instead of access calls. strace reveals that zsh\ndoes what git currently does. bash uses stat and says 'command not\nfound'.\n\nSending in case someone finds it useful or interesting. Feel free to\nignore it or make fun of it if you want.\n\n run-command.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 1101ef7..97e6960 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -85,6 +85,7 @@ static char *locate_in_PATH(const char *file)\n {\n \tconst char *p = getenv(\"PATH\");\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct stat st;\n \n \tif (!p || !*p)\n \t\treturn NULL;\n@@ -101,8 +102,9 @@ static char *locate_in_PATH(const char *file)\n \t\t}\n \t\tstrbuf_addstr(&buf, file);\n \n-\t\tif (!access(buf.buf, F_OK))\n+\t\tif (!stat(buf.buf, &st) && !S_ISDIR(st.st_mode)) {\n \t\t\treturn strbuf_detach(&buf, NULL);\n+\t\t}\n \n \t\tif (!*end)\n \t\t\tbreak;\n-- \n1.8.0.rc0.175.g59a8d0e\n"},{"id":"200350","messageId":"7vvces93qj.fsf@alter.siamese.dyndns.org","threadId":"31713","inReplyTo":"1349189193-25497-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH] run-command: don't try to execute directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-02T17:35:16Z","receivedAt":"2012-10-02T17:35:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> When looking through $PATH to try to find an external command,\n> locate_in_PATH doesn't check that it's trying to execute a file. Add a\n> check to make sure we won't try to execute a directory.\n>\n> This also stops us from looking further and maybe finding that the\n> user meant an alias, as in the case where the user has\n> /home/user/bin/git-foo/git-foo.pl and an alias\n>\n> [alias] foo = !/home/user/bin/git-foo/git-foo.pl\n>\n> Running 'git foo' will currently will try to execute ~/bin/git-foo and\n> fail because you can't execute a directory. By making sure we don't do\n> that, we realise that it's an alias and do the right thing\n>\n> Signed-off-by: Carlos Martín Nieto \n>\n> ---\n>\n> This comes from a case in #git. Not sure if this is worth it, or the\n> better solution is just to say no to dirs in $PATH.\n>\n> After writing all of that, I thought to check the shell, and indeed\n>\n> % git-foo\n> zsh: permission denied: git-foo\n>\n> so if the shell doesn't do it, the benefits probably don't outweigh\n> having a dozen stat instead of access calls. strace reveals that zsh\n> does what git currently does. bash uses stat and says 'command not\n> found'.\n\nHrm, I do not use zsh but it does not seem to reproduce for me.\n\n\t$ mkdir -p /var/tmp/xx/git\n $ zsh\n % PATH=/var/tmp/xx:$PATH\n % type git\n git is /home/junio/bin/git\n % git version\n git version 1.8.0.rc0.45.g7ce8dc5\n\t% zsh --version\n\tzsh 4.3.10 (x86_64-unknown-linux-gnu)\n\n> @@ -101,8 +102,9 @@ static char *locate_in_PATH(const char *file)\n> \t\t}\n> \t\tstrbuf_addstr(&buf, file);\n> \n> -\t\tif (!access(buf.buf, F_OK))\n> +\t\tif (!stat(buf.buf, &st) && !S_ISDIR(st.st_mode)) {\n> \t\t\treturn strbuf_detach(&buf, NULL);\n> +\t\t}\n\nSo we used to say \"if it exists and accessible, return that\". Now\nwe say \"if it exists and is not a directory, return that\".\n\nI have to wonder what would happen if it exists as a non-directory\nbut we cannot access it. Is that a regression?\n\n\n> \t\tif (!*end)\n> \t\t\tbreak;\n"},{"id":"200363","messageId":"87bogkisas.fsf@centaur.cmartin.tk","threadId":"31713","inReplyTo":"7vvces93qj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] run-command: don't try to execute directories","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-10-02T19:32:11Z","receivedAt":"2012-10-02T19:32:11Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"Junio C Hamano writes:\n\n> Carlos Martín Nieto writes:\n>\n>> When looking through $PATH to try to find an external command,\n>> locate_in_PATH doesn't check that it's trying to execute a file. Add a\n>> check to make sure we won't try to execute a directory.\n>>\n>> This also stops us from looking further and maybe finding that the\n>> user meant an alias, as in the case where the user has\n>> /home/user/bin/git-foo/git-foo.pl and an alias\n>>\n>> [alias] foo = !/home/user/bin/git-foo/git-foo.pl\n>>\n>> Running 'git foo' will currently will try to execute ~/bin/git-foo and\n>> fail because you can't execute a directory. By making sure we don't do\n>> that, we realise that it's an alias and do the right thing\n>>\n>> Signed-off-by: Carlos Martín Nieto \n>>\n>> ---\n>>\n>> This comes from a case in #git. Not sure if this is worth it, or the\n>> better solution is just to say no to dirs in $PATH.\n>>\n>> After writing all of that, I thought to check the shell, and indeed\n>>\n>> % git-foo\n>> zsh: permission denied: git-foo\n>>\n>> so if the shell doesn't do it, the benefits probably don't outweigh\n>> having a dozen stat instead of access calls. strace reveals that zsh\n>> does what git currently does. bash uses stat and says 'command not\n>> found'.\n>\n> Hrm, I do not use zsh but it does not seem to reproduce for me.\n>\n> \t$ mkdir -p /var/tmp/xx/git\n> $ zsh\n> % PATH=/var/tmp/xx:$PATH\n> % type git\n> git is /home/junio/bin/git\n> % git version\n> git version 1.8.0.rc0.45.g7ce8dc5\n> \t% zsh --version\n> \tzsh 4.3.10 (x86_64-unknown-linux-gnu)\n\nzsh has some quite aggressive PATH caching. I did this with git-foo in\nthe path so it didn't already know what to look for. I can reproduce\nwhat you saw, but also consider this:\n\n % /var/tmp/xx/git\n zsh: permission denied: /var/tmp/xx/git\n % zsh --version\n zsh 4.3.17 (x86_64-unknown-linux-gnu)\n\nIf you change your test to use git-foo instead of just git, you should\nsee what I wrote in the message.\n\nbash rightfully complains that it's a stupid thing to do.\n\n $ /var/tmp/xx/git\n bash: /var/tmp/xx/git: Is a directory\n\n>\n>> @@ -101,8 +102,9 @@ static char *locate_in_PATH(const char *file)\n>> \t\t}\n>> \t\tstrbuf_addstr(&buf, file);\n>> \n>> -\t\tif (!access(buf.buf, F_OK))\n>> +\t\tif (!stat(buf.buf, &st) && !S_ISDIR(st.st_mode)) {\n>> \t\t\treturn strbuf_detach(&buf, NULL);\n>> +\t\t}\n>\n> So we used to say \"if it exists and accessible, return that\". Now\n> we say \"if it exists and is not a directory, return that\".\n>\n> I have to wonder what would happen if it exists as a non-directory\n> but we cannot access it. Is that a regression?\n\nI guess it would be, yeah. Would this be related to tha situation where\nthe user isn't allowed to access something in their PATH?\n\nHow about something like this instead? We keep the access check and only\ndo the stat call when we have found something we want to look at.\n\n cmn\n\n---8<---\n\ndiff --git a/run-command.c b/run-command.c\nindex 1101ef7..fb8a93c 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -85,6 +85,7 @@ static char *locate_in_PATH(const char *file)\n {\n const char *p = getenv(\"PATH\");\n struct strbuf buf = STRBUF_INIT;\n+ struct stat st;\n \n if (!p || !*p)\n return NULL;\n@@ -101,7 +102,8 @@ static char *locate_in_PATH(const char *file)\n }\n strbuf_addstr(&buf, file);\n \n- if (!access(buf.buf, F_OK))\n+ if (!access(buf.buf, F_OK) &&\n+ !stat(buf.buf, &st) && !S_ISDIR(st.st_mode))\n return strbuf_detach(&buf, NULL);\n \n if (!*end)\n"},{"id":"200367","messageId":"7vd3107igx.fsf@alter.siamese.dyndns.org","threadId":"31713","inReplyTo":"87bogkisas.fsf@centaur.cmartin.tk","subject":"Re: [PATCH] run-command: don't try to execute directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-02T19:59:58Z","receivedAt":"2012-10-02T19:59:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"cmn@elego.de (Carlos Martín Nieto) writes:\n\n> How about something like this instead? We keep the access check and only\n> do the stat call when we have found something we want to look at.\n\nSounds safer.\n\nLooking at the way the stat call is indented twice, I suspect that\nthe variable can be defined inner scope, not at the top-level of the\nfunction?\n\n>\n> ---8<---\n>\n> diff --git a/run-command.c b/run-command.c\n> index 1101ef7..fb8a93c 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -85,6 +85,7 @@ static char *locate_in_PATH(const char *file)\n> {\n> const char *p = getenv(\"PATH\");\n> struct strbuf buf = STRBUF_INIT;\n> + struct stat st;\n> \n> if (!p || !*p)\n> return NULL;\n> @@ -101,7 +102,8 @@ static char *locate_in_PATH(const char *file)\n> }\n> strbuf_addstr(&buf, file);\n> \n> - if (!access(buf.buf, F_OK))\n> + if (!access(buf.buf, F_OK) &&\n> + !stat(buf.buf, &st) && !S_ISDIR(st.st_mode))\n> return strbuf_detach(&buf, NULL);\n> \n> if (!*end)\n"},{"id":"200370","messageId":"20121002212645.GA26789@sigill.intra.peff.net","threadId":"31713","inReplyTo":"87bogkisas.fsf@centaur.cmartin.tk","subject":"Re: [PATCH] run-command: don't try to execute directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-02T21:26:45Z","receivedAt":"2012-10-02T21:26:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 02, 2012 at 09:32:11PM +0200, Carlos Martín Nieto wrote:\n\n> >> @@ -101,8 +102,9 @@ static char *locate_in_PATH(const char *file)\n> >> \t\t}\n> >> \t\tstrbuf_addstr(&buf, file);\n> >> \n> >> -\t\tif (!access(buf.buf, F_OK))\n> >> +\t\tif (!stat(buf.buf, &st) && !S_ISDIR(st.st_mode)) {\n> >> \t\t\treturn strbuf_detach(&buf, NULL);\n> >> +\t\t}\n> >\n> > So we used to say \"if it exists and accessible, return that\". Now\n> > we say \"if it exists and is not a directory, return that\".\n> >\n> > I have to wonder what would happen if it exists as a non-directory\n> > but we cannot access it. Is that a regression?\n> \n> I guess it would be, yeah. Would this be related to tha situation where\n> the user isn't allowed to access something in their PATH?\n\nThis code path is related to correcting EACCES errors into ENOENT. But\nit does not bother checking permissions itself. We know there is some\npermission problem, because execvp told us EACCES, so we are only\nchecking whether such a file actually exists at all in the PATH. And\nthat is why we are using F_OK with access, and not X_OK.\n\nSo any reason for which stat() would fail would presumably cause\naccess(F_OK) to fail, too (mostly things like leading directories not\nbeing readable), and I think converting the access into a stat is OK.\n\nAdding the !ISDIR on top of it makes sense if you want to consider the\ndirectory in your PATH to be a harmless thing to be ignored. However, I\nam not sure that is a good idea. The intent of ignoring the original\nEACCES is that it could be caused by totally uninteresting crap, like an\ninaccessible directory in your PATH.\n\nWhereas in this case, the error really is that we found \"git-foo\", but\nit is somehow broken. And it almost certainly is a configuration error\non the part of the user (why would they put a git-foo directory in their\nPATH? Presumably they meant to put its contents into the PATH).\n\nSo I think your implementation is fine, but I'm a little dubious of the\nvalue of ignoring such an error.\n\n-Peff\n"}]}