{"thread":{"id":"30109","subject":"[PATCH] run-command: treat inaccessible directories as ENOENT","startedAt":"2012-03-30T07:52:18Z","lastAt":"2012-03-30T20:22:39Z","messageCount":5,"participants":["Jeff King","Frans Klaver","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"188136","messageId":"20120330075217.GA8384@sigill.intra.peff.net","threadId":"30109","inReplyTo":null,"subject":"[PATCH] run-command: treat inaccessible directories as ENOENT","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-30T07:52:18Z","receivedAt":"2012-03-30T07:52:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When execvp reports EACCES, it can be one of two things:\n\n 1. We found a file to execute, but did not have\n permissions to do so.\n\n 2. We did not have permissions to look in some directory\n in the $PATH.\n\nIn the former case, we want to consider this a\npermissions problem and report it to the user as such (since\ngetting this for something like \"git foo\" is likely a\nconfiguration error).\n\nIn the latter case, there is a good chance that the\ninaccessible directory does not contain anything of\ninterest. Reporting \"permission denied\" is confusing to the\nuser (and prevents our usual \"did you mean...?\" lookup). It\nalso prevents git from trying alias lookup, since we do so\nonly when an external command does not exist (not when it\nexists but has an error).\n\nThis patch detects EACCES from execvp, checks whether we are\nin case (2), and if so converts errno to ENOENT. This\nbehavior matches that of \"bash\" (but not of simpler shells\nthat use execvp more directly, like \"dash\").\n\nTest stolen from Junio.\n\nSigned-off-by: Jeff King \n---\nHere's the patch we've been discussing with a commit message and a few\nminor modifications:\n\n 1. I split the find-in-path function so that Frans can build on it\n with further checks if he wants. However, note that it will need a\n little more refactoring; it finds the first entry in the PATH,\n whereas I think he would want the first executable entry\n (admittedly, having a non-executable entry followed by an\n executable one which _also_ fails for a different reason is a\n pretty wild corner case).\n\n 2. I pulled the test from what Junio posted earlier. I started to\n write a full test script that checked each of the cases I\n mentioned earlier. However, in all but the alias case (which is\n what is tested here), the behavior is really only distinguishable\n by the error messages, and I didn't want to get into testing what\n strerror(EACCES) prints. I can re-roll if we really want to go\n there.\n\n cache.h | 2 ++\n exec_cmd.c | 2 +-\n run-command.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++--\n t/t0061-run-command.sh | 13 ++++++++++\n 4 files changed, 80 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex e5e1aa4..59e1c44 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1276,4 +1276,6 @@ extern struct startup_info *startup_info;\n /* builtin/merge.c */\n int checkout_fast_forward(const unsigned char *from, const unsigned char *to);\n \n+int sane_execvp(const char *file, char *const argv[]);\n+\n #endif /* CACHE_H */\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 171e841..125fa6f 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -134,7 +134,7 @@ int execv_git_cmd(const char **argv) {\n \ttrace_argv_printf(nargv, \"trace: exec:\");\n \n \t/* execvp() can only ever return if it fails */\n-\texecvp(\"git\", (char **)nargv);\n+\tsane_execvp(\"git\", (char **)nargv);\n \n \ttrace_printf(\"trace: exec failed: %s\\n\", strerror(errno));\n \ndiff --git a/run-command.c b/run-command.c\nindex 1db8abf..7123436 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -76,6 +76,68 @@ static inline void dup_devnull(int to)\n }\n #endif\n \n+static char *locate_in_PATH(const char *file)\n+{\n+\tconst char *p = getenv(\"PATH\");\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tif (!p || !*p)\n+\t\treturn 0;\n+\n+\twhile (1) {\n+\t\tconst char *end = strchrnul(p, ':');\n+\n+\t\tstrbuf_reset(&buf);\n+\n+\t\t/* POSIX specifies an empty entry as the current directory. */\n+\t\tif (end != p) {\n+\t\t\tstrbuf_add(&buf, p, end - p);\n+\t\t\tstrbuf_addch(&buf, '/');\n+\t\t}\n+\t\tstrbuf_addstr(&buf, file);\n+\n+\t\tif (!access(buf.buf, F_OK))\n+\t\t\treturn strbuf_detach(&buf, NULL);\n+\n+\t\tif (!*end)\n+\t\t\tbreak;\n+\t\tp = end + 1;\n+\t}\n+\n+\tstrbuf_release(&buf);\n+\treturn NULL;\n+}\n+\n+static int exists_in_PATH(const char *file)\n+{\n+\tchar *r = locate_in_PATH(file);\n+\tfree(r);\n+\treturn r != NULL;\n+}\n+\n+int sane_execvp(const char *file, char * const argv[])\n+{\n+\tif (!execvp(file, argv))\n+\t\treturn 0;\n+\n+\t/*\n+\t * When a command can't be found because one of the directories\n+\t * listed in $PATH is unsearchable, execvp reports EACCES, but\n+\t * careful usability testing (read: analysis of occasional bug\n+\t * reports) reveals that \"No such file or directory\" is more\n+\t * intuitive.\n+\t *\n+\t * We avoid commands with \"/\", because execvp will not do $PATH\n+\t * lookups in that case.\n+\t *\n+\t * The reassignment of EACCES to errno looks like a no-op below,\n+\t * but we need to protect against exists_in_PATH overwriting errno.\n+\t */\n+\tif (errno == EACCES && !strchr(file, '/'))\n+\t\terrno = exists_in_PATH(file) ? EACCES : ENOENT;\n+\treturn -1;\n+}\n+\n static const char **prepare_shell_cmd(const char **argv)\n {\n \tint argc, nargc = 0;\n@@ -114,7 +176,7 @@ static int execv_shell_cmd(const char **argv)\n {\n \tconst char **nargv = prepare_shell_cmd(argv);\n \ttrace_argv_printf(nargv, \"trace: exec:\");\n-\texecvp(nargv[0], (char **)nargv);\n+\tsane_execvp(nargv[0], (char **)nargv);\n \tfree(nargv);\n \treturn -1;\n }\n@@ -339,7 +401,7 @@ fail_pipe:\n \t\t} else if (cmd->use_shell) {\n \t\t\texecv_shell_cmd(cmd->argv);\n \t\t} else {\n-\t\t\texecvp(cmd->argv[0], (char *const*) cmd->argv);\n+\t\t\tsane_execvp(cmd->argv[0], (char *const*) cmd->argv);\n \t\t}\n \t\tif (errno == ENOENT) {\n \t\t\tif (!cmd->silent_exec_failure)\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex 8d4938f..17e969d 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -34,4 +34,17 @@ test_expect_success POSIXPERM 'run_command reports EACCES' '\n \tgrep \"fatal: cannot exec.*hello.sh\" err\n '\n \n+test_expect_success POSIXPERM 'unreadable directory in PATH' '\n+\tmkdir local-command &&\n+\ttest_when_finished \"chmod u+rwx local-command && rm -fr local-command\" &&\n+\tgit config alias.nitfol \"!echo frotz\" &&\n+\tchmod a-rx local-command &&\n+\t(\n+\t\tPATH=./local-command:$PATH &&\n+\t\tgit nitfol >actual\n+\t) &&\n+\techo frotz >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.9.5.7.g11b89\n"},{"id":"188139","messageId":"CAH6sp9N=JsWp7iQ=AAdXHe0J+aB5L9cBq2_0BJgUO=Y-vgAbNg@mail.gmail.com","threadId":"30109","inReplyTo":"20120330075217.GA8384@sigill.intra.peff.net","subject":"Re: [PATCH] run-command: treat inaccessible directories as ENOENT","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2012-03-30T08:23:16Z","receivedAt":"2012-03-30T08:23:16Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Fri, Mar 30, 2012 at 9:52 AM, Jeff King wrote:\n\n>  1. I split the find-in-path function so that Frans can build on it\n>     with further checks if he wants. However, note that it will need a\n>     little more refactoring; it finds the first entry in the PATH,\n>     whereas I think he would want the first executable entry\n>     (admittedly, having a non-executable entry followed by an\n>     executable one which _also_ fails for a different reason is a\n>     pretty wild corner case).\n\nThanks. I think this corner case is something to fix when someone is\nrunning into it. Bash doesn't cover this case either (stops looking at\nthe first found entry) so I would go as far as saying that it doesn't\nhappen.\n\n\n>  2. I pulled the test from what Junio posted earlier. I started to\n>     write a full test script that checked each of the cases I\n>     mentioned earlier. However, in all but the alias case (which is\n>     what is tested here), the behavior is really only distinguishable\n>     by the error messages, and I didn't want to get into testing what\n>     strerror(EACCES) prints. I can re-roll if we really want to go\n>     there.\n\nI wouldn't think there is much point in testing strerror output.\n\n\n\n> +test_expect_success POSIXPERM 'unreadable directory in PATH' '\n> +       mkdir local-command &&\n> +       test_when_finished \"chmod u+rwx local-command && rm -fr local-command\" &&\n> +       git config alias.nitfol \"!echo frotz\" &&\n> +       chmod a-rx local-command &&\n> +       (\n> +               PATH=./local-command:$PATH &&\n> +               git nitfol >actual\n> +       ) &&\n> +       echo frotz >expect &&\n> +       test_cmp expect actual\n> +'\n> +\n>  test_done\n> --\n> 1.7.9.5.7.g11b89\n\nHadn't looked into Junio's test earlier (probably missed it entirely),\nbut isn't it rather more sensible from a unit-test perspective to see\nif start_command returns 127 instead of 128 in this specific case?\nAside from that, this test doesn't seem to fit in t0061, looking at\nthe t???? guidelines.\n\nRest looks sensible to me.\n"},{"id":"188173","messageId":"7vr4wam59r.fsf@alter.siamese.dyndns.org","threadId":"30109","inReplyTo":"20120330075217.GA8384@sigill.intra.peff.net","subject":"Re: [PATCH] run-command: treat inaccessible directories as ENOENT","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-30T16:18:24Z","receivedAt":"2012-03-30T16:18:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Here's the patch we've been discussing with a commit message and a few\n> minor modifications:\n\nThe change to allow the result of \"locate_*\" usable by other callers makes\nsense to me. Thanks.\n\nWill queue.\n"},{"id":"188174","messageId":"7vmx6ym54m.fsf@alter.siamese.dyndns.org","threadId":"30109","inReplyTo":"CAH6sp9N=JsWp7iQ=AAdXHe0J+aB5L9cBq2_0BJgUO=Y-vgAbNg@mail.gmail.com","subject":"Re: [PATCH] run-command: treat inaccessible directories as ENOENT","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-30T16:21:29Z","receivedAt":"2012-03-30T16:21:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Frans Klaver writes:\n\n> isn't it rather more sensible from a unit-test perspective to see\n> if start_command returns 127 instead of 128 in this specific case?\n\n\nYou are welcome to add another test that checks lower level implementation\ndetail, but this specific test is to make sure the gripe \"Why does git\ndeny my aliases when I have inaccessible directory on my PATH?\" will never\ncome back.\n"},{"id":"188198","messageId":"op.wbz2v2k60aolir@keputer","threadId":"30109","inReplyTo":"7vmx6ym54m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] run-command: treat inaccessible directories as ENOENT","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2012-03-30T20:22:39Z","receivedAt":"2012-03-30T20:22:39Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Fri, 30 Mar 2012 18:21:29 +0200, Junio C Hamano \nwrote:\n\n> Frans Klaver writes:\n>\n>> isn't it rather more sensible from a unit-test perspective to see\n>> if start_command returns 127 instead of 128 in this specific case?\n>\n>\n> You are welcome to add another test that checks lower level \n> implementation\n> detail, but this specific test is to make sure the gripe \"Why does git\n> deny my aliases when I have inaccessible directory on my PATH?\" will \n> never\n> come back.\n\nI think I didn't word carefully enough there. I didn't mean to dispute the \nuse of the test. The test I proposed would make sense in t0061, but I \nwould rather have expected the test in Jeff's patch in a tests that \nspecifically targets aliases. It would be less surprising, wouldn't it? \nThe fact that git goes through start_command before doing aliases is \nmerely an implementation detail, from my point of view.\n"}]}