{"thread":{"id":"28996","subject":"[PATCH] run-command.c: Accept EACCES as command not found","startedAt":"2011-11-21T21:53:07Z","lastAt":"2011-12-14T22:06:22Z","messageCount":25,"participants":["Frans Klaver","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"179803","messageId":"1321912387-4569-1-git-send-email-fransklaver@gmail.com","threadId":"28996","inReplyTo":null,"subject":"[PATCH] run-command.c: Accept EACCES as command not found","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-11-21T21:53:07Z","receivedAt":"2011-11-21T21:53:07Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"execvp returns ENOENT if a command was not found after searching PATH.\nIf path contains a directory that current user has insufficient\nprivileges to, EACCES is returned. This may still mean the program\nwasn't found.\n\nIf the latter case is encountered, git errors out without giving aliases\na try, which breaks t0001.3 and alias handling in general.\n\nThis can be fixed by handling the EACCES case equally to the ENOENT\ncase.\n\nSigned-off-by: Frans Klaver <fransklaver@gmail.com>\n---\n\nI'm actually not too happy about the location of the tests. I couldn't\nfind out from the available tests and the documentation where I would\nhave to create the new ones. For now, I've added the tests to the same\nset that I found the issue with.\n\n run-command.c   |   10 ++++++++--\n t/t0001-init.sh |   48 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 56 insertions(+), 2 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 1c51043..ad3c120 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -280,12 +280,18 @@ fail_pipe:\n \t\t} else {\n \t\t\texecvp(cmd->argv[0], (char *const*) cmd->argv);\n \t\t}\n-\t\tif (errno == ENOENT) {\n+\t\tswitch (errno) {\n+\t\tcase ENOENT:\n \t\t\tif (!cmd->silent_exec_failure)\n \t\t\t\terror(\"cannot run %s: %s\", cmd->argv[0],\n \t\t\t\t\tstrerror(ENOENT));\n \t\t\texit(127);\n-\t\t} else {\n+\t\tcase EACCES:\n+\t\t\tif (!cmd->silent_exec_failure)\n+\t\t\t\terror(\"fatal: cannot exec '%s': %s\", cmd->argv[0],\n+\t\t\t\t\tstrerror(EACCES));\n+\t\t\texit(127);\n+\t\tdefault:\n \t\t\tdie_errno(\"cannot exec '%s'\", cmd->argv[0]);\n \t\t}\n \t}\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex ad66410..d40966a 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -63,6 +63,54 @@ test_expect_success 'plain through aliased command, outside any git repo' '\n \tcheck_config plain-aliased/.git false unset\n '\n \n+test_expect_success 'plain through aliased command, inaccessible path, outside any git repo' '\n+\t(\n+\t\tsane_unset GIT_DIR GIT_WORK_TREE &&\n+\t\tHOME=$(pwd)/alias-config-path &&\n+\t\texport HOME &&\n+\t\tmkdir alias-config-path &&\n+\t\techo \"[alias] aliasedinit = init\" >alias-config-path/.gitconfig &&\n+\n+\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\n+\t\tmkdir searchpath &&\n+\t\tchmod 400 searchpath &&\n+\t\tPATH=$(pwd)/searchpath:$PATH &&\n+\t\texport PATH &&\n+\n+\t\tmkdir plain-aliased-path &&\n+\t\tcd plain-aliased-path &&\n+\t\tgit aliasedinit\n+\t) &&\n+\tcheck_config plain-aliased-path/.git false unset\n+'\n+\n+test_expect_success 'plain through aliased command, inaccessible command, outside any git repo' '\n+\t(\n+\t\tsane_unset GIT_DIR GIT_WORK_TREE &&\n+\t\tHOME=$(pwd)/alias-config-cmd &&\n+\t\texport HOME &&\n+\t\tmkdir alias-config-cmd &&\n+\t\techo \"[alias] aliasedinit = init\" >alias-config-cmd/.gitconfig &&\n+\n+\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\n+\t\tmkdir searchpathcmd &&\n+\t\tchmod 755 searchpathcmd &&\n+\t\tPATH=$(pwd)/searchpathcmd:$PATH &&\n+\t\texport PATH &&\n+\n+\t\ttouch searchpathcmd/git-aliasedinit &&\n+\n+\t\tmkdir plain-aliased-cmd &&\n+\t\tcd plain-aliased-cmd &&\n+\t\tgit aliasedinit\n+\t) &&\n+\tcheck_config plain-aliased-cmd/.git false unset\n+'\n+\n test_expect_failure 'plain nested through aliased command' '\n \t(\n \t\tsane_unset GIT_DIR GIT_WORK_TREE &&\n-- \n1.7.7\n"},{"id":"179805","messageId":"7vbos5f7ix.fsf@alter.siamese.dyndns.org","threadId":"28996","inReplyTo":"1321912387-4569-1-git-send-email-fransklaver@gmail.com","subject":"Re: [PATCH] run-command.c: Accept EACCES as command not found","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-21T22:13:58Z","receivedAt":"2011-11-21T22:13:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Frans Klaver <fransklaver@gmail.com> writes:\n\n> execvp returns ENOENT if a command was not found after searching PATH.\n> If path contains a directory that current user has insufficient\n> privileges to, EACCES is returned. This may still mean the program\n> wasn't found.\n>\n> If the latter case is encountered, git errors out without giving aliases\n> a try,...\n\nIsn't that a *good* thing in general, though, so that the user can\ndiagnose the breakage in the $PATH and fix it?\n"},{"id":"179809","messageId":"op.v5bjtk1r0aolir@keputer","threadId":"28996","inReplyTo":"7vbos5f7ix.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] run-command.c: Accept EACCES as command not found","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-11-21T23:06:46Z","receivedAt":"2011-11-21T23:06:46Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Mon, 21 Nov 2011 23:13:58 +0100, Junio C Hamano <gitster@pobox.com>  \nwrote:\n\n> Frans Klaver <fransklaver@gmail.com> writes:\n>\n>> execvp returns ENOENT if a command was not found after searching PATH.\n>> If path contains a directory that current user has insufficient\n>> privileges to, EACCES is returned. This may still mean the program\n>> wasn't found.\n>>\n>> If the latter case is encountered, git errors out without giving aliases\n>> a try,...\n>\n> Isn't that a *good* thing in general, though, so that the user can\n> diagnose the breakage in the $PATH and fix it?\n\nActually I went through diagnosing and fixing it. After tracking it down,  \nI did wonder about this question myself and I didn't come to a definitive  \nconclusion on it. On one hand I do agree that it may be an incentive for  \nthe user to fix his path. On the other hand I found it an obscure one to  \ntrack down; git's behavior doesn't match bash behavior:\n\n$ git config --global alias.aliasedinit init &&\nmkdir searchpath && chmod 400 searchpath && PATH=$(pwd)/searchpath:$PATH  \n&& export PATH &&\nmkdir someproject && cd someproject &&\ngit aliasedinit\nfatal: cannot exec 'git-aliasedinit': Permission denied\n\n$ git-aliasedinit\nbash: git-aliasedinit: command not found\n\nThis isn't very intuitive to track down an incorrect PATH with, imo. You  \nhave to dig into git core code, learn about how git handles commands,  \nlearn about debugging forked processes, and find out that execvp uses  \nEACCES for more than just \"permission denied\" _just_ to find out you've  \ngot a wrong environment variable lying about. That's a full day of work  \ngone for a newbie. If bash would also tell me in natural language that  \npermission was denied, I wouldn't even have considered doing this patch.\n\nFor my part the root of the problem lies with the use of EACCES by execvp  \nhere, not so much with how git uses it. For this particular case, EACCES  \ndoesn't just mean \"it exists, but you cannot execute this\", it may also  \nmean \"not found, but one of the paths could not be accessed\". If git were  \nto provide a really helpful message, we'd have to detect which paths got  \ndenied. Once we know that, we can even on the spot decide to error out or  \nnot. In other words, we'd have to figure out which meaning of EACCES is  \nactually used. Based on that, git can error out, warn or ignore at will.\n\nIn any case, I thought it best to have other developers have a look at it.  \nI can put a bit more of that information in the commit message, but I'd be  \njust as happy to drop the patch and keep the exercise.\n\nFrans\n"},{"id":"179811","messageId":"7v62idf2vy.fsf@alter.siamese.dyndns.org","threadId":"28996","inReplyTo":"op.v5bjtk1r0aolir@keputer","subject":"Re: [PATCH] run-command.c: Accept EACCES as command not found","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-21T23:54:09Z","receivedAt":"2011-11-21T23:54:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Frans Klaver\" <fransklaver@gmail.com> writes:\n\n> Actually I went through diagnosing and fixing it. After tracking it\n> down, I did wonder about this question myself and I didn't come to a\n> definitive  conclusion on it. On one hand I do agree that it may be an\n> incentive for  the user to fix his path. On the other hand I found it\n> an obscure one to  track down; git's behavior doesn't match bash\n> behavior:\n>\n> $ git config --global alias.aliasedinit init &&\n> mkdir searchpath && chmod 400 searchpath &&\n> PATH=$(pwd)/searchpath:$PATH && export PATH &&\n> mkdir someproject && cd someproject &&\n> git aliasedinit\n> fatal: cannot exec 'git-aliasedinit': Permission denied\n\nImagine you did not have alias.aliasedinit in ~/.gitconfig but had a\nscript called $(pwd)/searchpath/git-aliasedinit which we would fail to\nexecute. What message would we get in that case? Currently I think we get\npermission denied.\n\nWould we get the same with your patch, or something that does not hint\nat all that there is a permission problem?\n\nSee also the \"tangent\" part of\n\n    http://thread.gmane.org/gmane.comp.version-control.git/171755\n\nand the discussion that follows it. I do not think we reached any\nconclusion nor a patch.\n"},{"id":"179818","messageId":"CAH6sp9MxbDhQ3RiA6jO1fswAZX3R6C2fv0gzJdpGp432ovWsjQ@mail.gmail.com","threadId":"28996","inReplyTo":"7v62idf2vy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] run-command.c: Accept EACCES as command not found","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-11-22T09:31:48Z","receivedAt":"2011-11-22T09:31:48Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Tue, Nov 22, 2011 at 12:54 AM, Junio C Hamano <gitster@pobox.com> wrote:\n\n> Imagine you did not have alias.aliasedinit in ~/.gitconfig but had a\n> script called $(pwd)/searchpath/git-aliasedinit which we would fail to\n> execute. What message would we get in that case? Currently I think we get\n> permission denied.\n\nCorrect.\n\n> Would we get the same with your patch, or something that does not hint\n> at all that there is a permission problem?\n\nNope. That would be just as confusing and inarguably incorrect at that\n-- bash differentiates between commands that exist, but cannot be\nexecuted due to permissions (access denied) and paths that cannot be\nread (they are ignored in the search).\n\n\n> See also the \"tangent\" part of\n>\n>    http://thread.gmane.org/gmane.comp.version-control.git/171755\n>\n> and the discussion that follows it. I do not think we reached any\n> conclusion nor a patch.\n\nThere's no black-on-white conclusion there. I get the impression that\nno one really has an idea of what they want when encountering EACCES.\nGit has to do what's reasonable to provide the user with information.\nCurrently I think it does too little. Jonathan N. gave the option of\noptionally using libexplain[1]. It's pretty verbose and accurate:\n\nfatal: cannot exec 'git-frotz': execvp(pathname = \"git-frotz\", argv =\n[\"git-frotz\"]) failed, Permission denied (13, EACCES) because the\nprocess does not have search permission to the pathname\n\"/home/frans/devsw/searchpath\" directory, the process effective UID\n1000 \"frans\" matches the directory owner UID 1000 \"frans\" and the\nowner permission mode is \"r--\", and the process is not privileged\n(does not have the DAC_READ_SEARCH capability): Success\n\nI wouldn't be in favor of adding the dependency just to enable users\nto track down PATH issues though. Also, I think \"Cannot access\n/home/frans/devsw/searchpath\" would just as well do the trick.\n\nFor Jonathan's example[2] libexplain doesn't have a clear answer either:\nfatal: cannot exec 'git-frotz': execvp(pathname = \"git-frotz\", argv =\n[\"git-frotz\"]) failed, Permission denied (13, EACCES): Permission\ndenied\n\nIf git is going to do some diagnostics on why the execvp returned\nEACCES, it can still give a few hints. Most of the more likely options\nare then ruled out.\n\nFrans\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/171860\n[2] http://article.gmane.org/gmane.comp.version-control.git/171848\n"},{"id":"179862","messageId":"CAH6sp9N2ycsoU=is3BVanH33CowD+sMNmWq=Z1MsPJX=HGYY+g@mail.gmail.com","threadId":"28996","inReplyTo":"CAH6sp9MxbDhQ3RiA6jO1fswAZX3R6C2fv0gzJdpGp432ovWsjQ@mail.gmail.com","subject":"Re: [PATCH] run-command.c: Accept EACCES as command not found","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-11-23T08:17:43Z","receivedAt":"2011-11-23T08:17:43Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Tue, Nov 22, 2011 at 10:31 AM, Frans Klaver <fransklaver@gmail.com> wrote:\n\n> If git is going to do some diagnostics on why the execvp returned\n> EACCES, it can still give a few hints. Most of the more likely options\n> are then ruled out.\n\nIf there are no objections, I'm going to cook up a patch that\n\n- Keeps the current behavior (bail on EACCES)\n- Adds a more helpful diagnostic message somewhat like libexplain's,\nbut more terse and if possible with slightly more domain knowledge\n- Takes into account the notes made following\nhttp://article.gmane.org/gmane.comp.version-control.git/171838\n\nFrans\n"},{"id":"179881","messageId":"CACsJy8ATJ33i5YaM-APtUPq_fDkj9=JpKj9pmvqWK2QodgbexQ@mail.gmail.com","threadId":"28996","inReplyTo":"CAH6sp9N2ycsoU=is3BVanH33CowD+sMNmWq=Z1MsPJX=HGYY+g@mail.gmail.com","subject":"Re: [PATCH] run-command.c: Accept EACCES as command not found","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-23T12:04:58Z","receivedAt":"2011-11-23T12:04:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Nov 23, 2011 at 3:17 PM, Frans Klaver <fransklaver@gmail.com> wrote:\n> If there are no objections, I'm going to cook up a patch that\n>\n> - Keeps the current behavior (bail on EACCES)\n> - Adds a more helpful diagnostic message somewhat like libexplain's,\n> but more terse and if possible with slightly more domain knowledge\n\nIf you print diagnostic messages with trace_printf() and friends (only\nshowed when GIT_TRACE variable is set), then there's no need for being\nterse.\n-- \nDuy\n"},{"id":"179885","messageId":"CAH6sp9PMjywExthnizo-UOf26V-9f1q1DjAtTfA+Buihuuj+fg@mail.gmail.com","threadId":"28996","inReplyTo":"CACsJy8ATJ33i5YaM-APtUPq_fDkj9=JpKj9pmvqWK2QodgbexQ@mail.gmail.com","subject":"Re: [PATCH] run-command.c: Accept EACCES as command not found","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-11-23T13:25:38Z","receivedAt":"2011-11-23T13:25:38Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Wed, Nov 23, 2011 at 1:04 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:\n\n> If you print diagnostic messages with trace_printf() and friends (only\n> showed when GIT_TRACE variable is set), then there's no need for being\n> terse.\n\nI'll keep that in mind, thanks.\n\nFrans\n"},{"id":"179921","messageId":"op.v5e8mgbc0aolir@keputer","threadId":"28996","inReplyTo":"CAH6sp9N2ycsoU=is3BVanH33CowD+sMNmWq=Z1MsPJX=HGYY+g@mail.gmail.com","subject":"Re: [PATCH] run-command.c: Accept EACCES as command not found","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-11-23T22:55:18Z","receivedAt":"2011-11-23T22:55:18Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Wed, 23 Nov 2011 09:17:43 +0100, Frans Klaver <fransklaver@gmail.com>  \nwrote:\n\n> On Tue, Nov 22, 2011 at 10:31 AM, Frans Klaver <fransklaver@gmail.com>  \n> wrote:\n>\n>> If git is going to do some diagnostics on why the execvp returned\n>> EACCES, it can still give a few hints. Most of the more likely options\n>> are then ruled out.\n>\n> If there are no objections, I'm going to cook up a patch that\n>\n> - Keeps the current behavior (bail on EACCES)\n> - Adds a more helpful diagnostic message somewhat like libexplain's,\n> but more terse and if possible with slightly more domain knowledge\n> - Takes into account the notes made following\n> http://article.gmane.org/gmane.comp.version-control.git/171838\n\nSo here be some tests I intend to use (based on t0061.3):\n\nrun_command reports EACCES, file permissions:\n         cat hello-script >hello.sh &&\n         chmod -x hello.sh &&\n         test_must_fail test-run-command run-command ./hello.sh 2>err &&\n\n         grep \"fatal: cannot exec.*hello.sh\" err\n\n\nrun_command reports EACCES, search path permisions:\n         mkdir -p inaccessible &&\n         PATH=$(pwd)/inaccessible:$PATH &&\n         export PATH &&\n\n         cat hello-script >inaccessible/hello.sh &&\n         chmod 400 inaccessible &&\n         test_must_fail test-run-command run-command hello.sh 2>err &&\n\n         grep \"fatal: cannot exec.*hello.sh\" err &&\n         grep \"incorrect PATH entry\" err\n\n\nrun_command reports EACCES, interpreter fails:\n         cat incorrect-interpreter-script >hello.sh &&\n         chmod +x incorrect-interpreter-script &&\n         chmod -x someinterpreter &&\n         test_must_fail test-run-command run-command ./hello.sh 2>err &&\n\n         grep \"fatal: cannot exec.*hello.sh\" err &&\n         grep \"cannot execute interpreter\" err\n\n\nPossibly getting (over)ambitious on the interpreter test, but hey, gotta  \naim high.\n\nIf anybody has a test case that isn't covered, I'd be much obliged.\n\nFrans\n"},{"id":"180413","messageId":"1323207503-26581-1-git-send-email-fransklaver@gmail.com","threadId":"28996","inReplyTo":"op.v5e8mgbc0aolir@keputer","subject":"[PATCH 0/2] run-command: Add EACCES diagnostics","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-12-06T21:38:21Z","receivedAt":"2011-12-06T21:38:21Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"So here's a couple of patches that introduce some more elaborate investigation\ninto what went wrong when receiving EACCES. This is probably something that\ncould be expanded in the future, as running a command doesn't always produce\nequally obvious error messages.\n\n\"run-command: Add checks after execvp fails with EACCES\" provides some basic checks\non the permissions in PATH, and gives just a warning that none of its checks \nindicate a problem, so the user should check at least the interpreter permissions.\n\n\"run-command: Add interpreter permissions check\" actually adds interpreter checking.\n\n---\n\n run-command.c          |  172 ++++++++++++++++++++++++++++++++++++++++++++++++\n t/t0061-run-command.sh |   38 ++++++++++-\n 2 files changed, 209 insertions(+), 1 deletions(-)\n"},{"id":"180414","messageId":"1323207503-26581-2-git-send-email-fransklaver@gmail.com","threadId":"28996","inReplyTo":"1323207503-26581-1-git-send-email-fransklaver@gmail.com","subject":"[PATCH 1/2] run-command: Add checks after execvp fails with EACCES","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-12-06T21:38:22Z","receivedAt":"2011-12-06T21:38:22Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"execvp returns ENOENT if a command was not found after searching PATH.\nIf path contains a directory that current user has insufficient\nprivileges to, EACCES is returned. This may still mean the program\nwasn't found and may cause confusion to the user, especially when the\nfile mentioned doesn't exist -- that is, the user would expect NOENT to\nbe returned -- and the user was actually hoping for an alias to be executed.\n\nTo help users track down the core issue more easily, perform some checks\non the path and file permissions involved. Output errors when paths or\nfiles don't have enough permissions.\n\nSigned-off-by: Frans Klaver <fransklaver@gmail.com>\n---\n run-command.c          |  118 ++++++++++++++++++++++++++++++++++++++++++++++++\n t/t0061-run-command.sh |   16 ++++++-\n 2 files changed, 133 insertions(+), 1 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 1c51043..5e38c5a 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -2,6 +2,7 @@\n #include \"run-command.h\"\n #include \"exec_cmd.h\"\n #include \"argv-array.h\"\n+#include \"dir.h\"\n \n static inline void close_pair(int fd[2])\n {\n@@ -134,6 +135,119 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n \treturn code;\n }\n \n+#ifndef WIN32\n+static int is_in_group(gid_t gid)\n+{\n+\tgid_t *groups;\n+\tint ngroups, gc;\n+\tint yes;\n+\n+\tif (gid == getgid())\n+\t\treturn 1;\n+\n+\tgroups = NULL;\n+\tngroups = getgroups(0, NULL);\n+\tif (ngroups > 0) {\n+\t\tgroups = (gid_t *)xmalloc(ngroups * sizeof(gid_t));\n+\t\tif (getgroups(ngroups, groups) < 0) {\n+\t\t\tfree(groups);\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n+\tyes = 0;\n+\tfor (gc = 0; gc < ngroups; gc++)\n+\t\tif (groups[gc] == gid)\n+\t\t\tyes = 1;\n+\n+\tfree(groups);\n+\treturn yes;\n+}\n+\n+static int have_read_execute_permissions(const char *path)\n+{\n+\tstruct stat s;\n+\ttrace_printf(\"checking '%s'\\n\", path);\n+\n+\tif (stat(path, &s) < 0) {\n+\t\ttrace_printf(\"could not stat '%s': %s\\n\",\n+\t\t\t\tpath, strerror(errno));\n+\t\treturn 0;\n+\t}\n+\ttrace_printf(\"uid: %d, gid: %d\\n\", s.st_uid, s.st_gid);\n+\ttrace_printf(\"mode: %o\\n\", s.st_mode);\n+\n+\t/* check world permissions */\n+\tif ((s.st_mode&(S_IXOTH|S_IROTH)) == (S_IXOTH|S_IROTH))\n+\t\treturn 1;\n+\n+\t/* check group permissions & membership */\n+\tif ((s.st_mode&(S_IXGRP|S_IRGRP)) == (S_IXGRP|S_IRGRP) &&\n+\t\tis_in_group(s.st_gid))\n+\t\treturn 1;\n+\n+\t/* check owner permissions & ownership */\n+\tif ((s.st_mode&(S_IXUSR|S_IRUSR)) == (S_IXUSR|S_IRUSR) &&\n+\t\ts.st_uid == getuid())\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n+static void diagnose_execvp_eacces(const char *cmd, const char **argv)\n+{\n+\t/* man 2 execve states that EACCES is returned for:\n+\t * - Search permission is denied on a component of the path prefix\n+\t *   of cmd or the name of a script interpreter\n+\t * - The file or script interpreter is not a regular file\n+\t * - Execute permission is denied for the file, script or ELF\n+\t *   interpreter\n+\t * - The file system is mounted noexec\n+\t */\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tchar *path = getenv(\"PATH\");\n+\tchar *next;\n+\n+\tif (strchr(cmd, '/')) {\n+\t\tif (!have_read_execute_permissions(cmd))\n+\t\t\terror(\"no read/execute permissions on '%s'\\n\", cmd);\n+\t\treturn;\n+\t}\n+\n+\tfor (;;) {\n+\t\tnext = strchrnul(path, ':');\n+\t\tif (path < next)\n+\t\t\tstrbuf_add(&sb, path, next - path);\n+\t\telse\n+\t\t\tstrbuf_addch(&sb, '.');\n+\n+\t\tif (!have_read_execute_permissions(sb.buf))\n+\t\t\terror(\"no read/execute permissions on '%s'\\n\", sb.buf);\n+\n+\t\tif (sb.len && sb.buf[sb.len - 1] != '/')\n+\t\t\tstrbuf_addch(&sb, '/');\n+\t\tstrbuf_addstr(&sb, cmd);\n+\n+\t\tif (file_exists(sb.buf)) {\n+\t\t\tif (!have_read_execute_permissions(sb.buf))\n+\t\t\t\terror(\"no read/execute permissions on '%s'\\n\",\n+\t\t\t\t\t\tsb.buf);\n+\t\t\telse\n+\t\t\t\twarn(\"file '%s' exists and permissions \"\n+\t\t\t\t\"seem OK.\\nIf this is a script, see if you \"\n+\t\t\t\t\"have sufficient privileges to run the \"\n+\t\t\t\t\"interpreter\", sb.buf);\n+\t\t}\n+\n+\t\tstrbuf_release(&sb);\n+\n+\t\tif (!*next)\n+\t\t\tbreak;\n+\t\tpath = next + 1;\n+\t}\n+}\n+#endif\n+\n int start_command(struct child_process *cmd)\n {\n \tint need_in, need_out, need_err;\n@@ -285,6 +399,10 @@ fail_pipe:\n \t\t\t\terror(\"cannot run %s: %s\", cmd->argv[0],\n \t\t\t\t\tstrerror(ENOENT));\n \t\t\texit(127);\n+\t\t} else if (errno == EACCES) {\n+\t\t\tdiagnose_execvp_eacces(cmd->argv[0], cmd->argv);\n+\t\t\tdie(\"cannot exec '%s': %s\", cmd->argv[0],\n+\t\t\t\tstrerror(EACCES));\n \t\t} else {\n \t\t\tdie_errno(\"cannot exec '%s'\", cmd->argv[0]);\n \t\t}\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex 8d4938f..b39bd16 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -26,7 +26,7 @@ test_expect_success 'run_command can run a command' '\n \ttest_cmp empty err\n '\n \n-test_expect_success POSIXPERM 'run_command reports EACCES' '\n+test_expect_success POSIXPERM 'run_command reports EACCES, file permissions' '\n \tcat hello-script >hello.sh &&\n \tchmod -x hello.sh &&\n \ttest_must_fail test-run-command run-command ./hello.sh 2>err &&\n@@ -34,4 +34,18 @@ test_expect_success POSIXPERM 'run_command reports EACCES' '\n \tgrep \"fatal: cannot exec.*hello.sh\" err\n '\n \n+test_expect_success POSIXPERM 'run_command reports EACCES, search path permisions' '\n+\tmkdir -p inaccessible &&\n+\tPATH=$(pwd)/inaccessible:$PATH &&\n+\texport PATH &&\n+\n+\tcat hello-script >inaccessible/hello.sh &&\n+\tchmod 400 inaccessible &&\n+\ttest_must_fail test-run-command run-command hello.sh 2>err &&\n+\tchmod 755 inaccessible &&\n+\n+\tgrep \"fatal: cannot exec.*hello.sh\" err &&\n+\tgrep \"no read/execute permissions on\" err\n+'\n+\n test_done\n-- \n1.7.8\n"},{"id":"180415","messageId":"1323207503-26581-3-git-send-email-fransklaver@gmail.com","threadId":"28996","inReplyTo":"1323207503-26581-1-git-send-email-fransklaver@gmail.com","subject":"[PATCH 2/2] run-command: Add interpreter permissions check","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-12-06T21:38:23Z","receivedAt":"2011-12-06T21:38:23Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"If a script is started and the interpreter of that script given in the\nshebang cannot be started due to permissions, we can get a rather\nobscure situation. All permission checks pass for the script itself,\nbut we still get EACCES from execvp.\n\nTry to find out if the above is the case and warn the user about it.\n\nSigned-off-by: Frans Klaver <fransklaver@gmail.com>\n---\n run-command.c          |   66 +++++++++++++++++++++++++++++++++++++++++++----\n t/t0061-run-command.sh |   22 ++++++++++++++++\n 2 files changed, 82 insertions(+), 6 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 5e38c5a..b8cf8d4 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -194,6 +194,63 @@ static int have_read_execute_permissions(const char *path)\n \treturn 0;\n }\n \n+static void check_interpreter(const char *cmd)\n+{\n+\tFILE *f;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\t/* bash reads an 80 character line when determining the interpreter.\n+\t * BSD apparently only allows 32 characters, as it is the size of\n+\t * your average binary executable header.\n+\t */\n+\tchar firstline[80];\n+\tchar *interpreter = NULL;\n+\tsize_t s, i;\n+\n+\tf = fopen(cmd, \"r\");\n+\tif (!f) {\n+\t\terror(\"cannot open file '%s': %s\\n\", cmd, strerror(errno));\n+\t\treturn;\n+\t}\n+\n+\ts = fread(firstline, 1, sizeof(firstline), f);\n+\tif (s < 2) {\n+\t\ttrace_printf(\"cannot determine file type\");\n+\t\tfclose(f);\n+\t\treturn;\n+\t}\n+\n+\tif (firstline[0] != '#' || firstline[1] != '!') {\n+\t\ttrace_printf(\"file '%s' is not a script or\"\n+\t\t\t\t\" is a script without '#!'\", cmd);\n+\t\tfclose(f);\n+\t\treturn;\n+\t}\n+\n+\t/* see if the given path has the executable bit set */\n+\tfor (i = 2; i < s; i++) {\n+\t\tif (!interpreter && firstline[i] != ' ' && firstline[i] != '\\t')\n+\t\t\tinterpreter = firstline + i;\n+\n+\t\tif (interpreter && (firstline[i] == ' ' ||\n+\t\t\t\tfirstline[i] == '\\n')) {\n+\t\t\tstrbuf_add(&sb, interpreter,\n+\t\t\t\t\t(firstline + i) - interpreter);\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\tif (!sb.len) {\n+\t\terror(\"could not determine interpreter\");\n+\t\tstrbuf_release(&sb);\n+\t\treturn;\n+\t}\n+\n+\tif (!have_read_execute_permissions(sb.buf))\n+\t\terror(\"bad interpreter: no read/execute permissions on '%s'\\n\",\n+\t\t\t\tsb.buf);\n+\n+\tstrbuf_release(&sb);\n+}\n+\n static void diagnose_execvp_eacces(const char *cmd, const char **argv)\n {\n \t/* man 2 execve states that EACCES is returned for:\n@@ -209,8 +266,8 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)\n \tchar *next;\n \n \tif (strchr(cmd, '/')) {\n-\t\tif (!have_read_execute_permissions(cmd))\n-\t\t\terror(\"no read/execute permissions on '%s'\\n\", cmd);\n+\t\tif (have_read_execute_permissions(cmd))\n+\t\t\tcheck_interpreter(cmd);\n \t\treturn;\n \t}\n \n@@ -233,10 +290,7 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)\n \t\t\t\terror(\"no read/execute permissions on '%s'\\n\",\n \t\t\t\t\t\tsb.buf);\n \t\t\telse\n-\t\t\t\twarn(\"file '%s' exists and permissions \"\n-\t\t\t\t\"seem OK.\\nIf this is a script, see if you \"\n-\t\t\t\t\"have sufficient privileges to run the \"\n-\t\t\t\t\"interpreter\", sb.buf);\n+\t\t\t\tcheck_interpreter(sb.buf);\n \t\t}\n \n \t\tstrbuf_release(&sb);\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex b39bd16..39bfaef 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -13,6 +13,18 @@ cat >hello-script <<-EOF\n EOF\n >empty\n \n+cat >someinterpreter <<-EOF\n+\t#!$SHELL_PATH\n+\tcat hello-script\n+EOF\n+>empty\n+\n+cat >incorrect-interpreter-script <<-EOF\n+\t#!someinterpreter\n+\tcat hello-script\n+EOF\n+>empty\n+\n test_expect_success 'start_command reports ENOENT' '\n \ttest-run-command start-command-ENOENT ./does-not-exist\n '\n@@ -48,4 +60,14 @@ test_expect_success POSIXPERM 'run_command reports EACCES, search path permision\n \tgrep \"no read/execute permissions on\" err\n '\n \n+test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '\n+\tcat incorrect-interpreter-script >hello.sh &&\n+\tchmod +x hello.sh &&\n+\tchmod -x someinterpreter &&\n+\ttest_must_fail test-run-command run-command ./hello.sh 2>err &&\n+\n+\tgrep \"fatal: cannot exec.*hello.sh\" err &&\n+\tgrep \"bad interpreter\" err\n+'\n+\n test_done\n-- \n1.7.8\n"},{"id":"180423","messageId":"7vpqg1e3au.fsf@alter.siamese.dyndns.org","threadId":"28996","inReplyTo":"1323207503-26581-2-git-send-email-fransklaver@gmail.com","subject":"Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-06T22:35:53Z","receivedAt":"2011-12-06T22:35:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Frans Klaver <fransklaver@gmail.com> writes:\n\n> +#ifndef WIN32\n> +static int is_in_group(gid_t gid)\n> ...\n> +static int have_read_execute_permissions(const char *path)\n> +{\n> +\tstruct stat s;\n> +\ttrace_printf(\"checking '%s'\\n\", path);\n> +\n> +\tif (stat(path, &s) < 0) {\n> + ...\n> +\t/* check world permissions */\n> +\tif ((s.st_mode&(S_IXOTH|S_IROTH)) == (S_IXOTH|S_IROTH))\n> +\t\treturn 1;\n\nHmm, do you need to do this with stat(2)?\n\nWouldn't access(2) with R_OK|X_OK give you exactly what you want without\nthis much trouble?\n\nI also think that your permission check is incorrectly implemented.\n\n    $ cd /var/tmp && date >j && chmod 044 j && ls -l j\n    ----r--r-- 1 junio junio 29 Dec  6 14:32 j\n    $ cat j\n    cat: j: Permission denied\n    $ su pogo\n    Password:\n    $ cat j\n    Tue Dec  6 14:32:23 PST 2011\n    \nThat's a world-readable but unreadable-only-to-me file.\n\n> +static void diagnose_execvp_eacces(const char *cmd, const char **argv)\n> +{\n> +\t/* man 2 execve states that EACCES is returned for:\n\n\t/*\n         * Just a style, but we tend to write multi-line comment like\n         * this, without anything else on opening and closing lines of\n         * the comment block.\n         */\n\n> +\t * - The file system is mounted noexec\n> +\t */\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tchar *path = getenv(\"PATH\");\n> +\tchar *next;\n> +\n> +\tif (strchr(cmd, '/')) {\n> +\t\tif (!have_read_execute_permissions(cmd))\n> +\t\t\terror(\"no read/execute permissions on '%s'\\n\", cmd);\n> +\t\treturn;\n> +\t}\n\nOk, execvp() failed and \"cmd\" has at least one slash, so we know we did\nnot look for it in $PATH.  We check only one and return (did you need\ngetenv() in that case?).\n\n> +\tfor (;;) {\n> +\t\tnext = strchrnul(path, ':');\n> +\t\tif (path < next)\n> +\t\t\tstrbuf_add(&sb, path, next - path);\n> +\t\telse\n> +\t\t\tstrbuf_addch(&sb, '.');\n\nNice touch that you did not forget an empty component on $PATH.\n\n> +\t\tif (!have_read_execute_permissions(sb.buf))\n> +\t\t\terror(\"no read/execute permissions on '%s'\\n\", sb.buf);\n\nDon't you want to continue here upon error, after resetting sb? You just\nsaw the directory is unreadble, so you know next file_exists() will fail\nbefore you try it.\n\n> +\t\tif (sb.len && sb.buf[sb.len - 1] != '/')\n> +\t\t\tstrbuf_addch(&sb, '/');\n> +\t\tstrbuf_addstr(&sb, cmd);\n> +\n> +\t\tif (file_exists(sb.buf)) {\n> +\t\t\tif (!have_read_execute_permissions(sb.buf))\n> +\t\t\t\terror(\"no read/execute permissions on '%s'\\n\",\n> +\t\t\t\t\t\tsb.buf);\n> +\t\t\telse\n> +\t\t\t\twarn(\"file '%s' exists and permissions \"\n> +\t\t\t\t\"seem OK.\\nIf this is a script, see if you \"\n> +\t\t\t\t\"have sufficient privileges to run the \"\n> +\t\t\t\t\"interpreter\", sb.buf);\n\nDoes \"warn()\" do the right thing for multi-line strings like this?\n"},{"id":"180424","messageId":"7vk469e2rn.fsf@alter.siamese.dyndns.org","threadId":"28996","inReplyTo":"1323207503-26581-3-git-send-email-fransklaver@gmail.com","subject":"Re: [PATCH 2/2] run-command: Add interpreter permissions check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-06T22:47:24Z","receivedAt":"2011-12-06T22:47:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Frans Klaver <fransklaver@gmail.com> writes:\n\n> If a script is started and the interpreter of that script given in the\n> shebang cannot be started due to permissions, we can get a rather\n> obscure situation. All permission checks pass for the script itself,\n> but we still get EACCES from execvp.\n>\n> Try to find out if the above is the case and warn the user about it.\n>\n> Signed-off-by: Frans Klaver <fransklaver@gmail.com>\n> ---\n>  run-command.c          |   66 +++++++++++++++++++++++++++++++++++++++++++----\n>  t/t0061-run-command.sh |   22 ++++++++++++++++\n>  2 files changed, 82 insertions(+), 6 deletions(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 5e38c5a..b8cf8d4 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -194,6 +194,63 @@ static int have_read_execute_permissions(const char *path)\n>  \treturn 0;\n>  }\n>  \n> +static void check_interpreter(const char *cmd)\n> +{\n> +\tFILE *f;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\t/* bash reads an 80 character line when determining the interpreter.\n> +\t * BSD apparently only allows 32 characters, as it is the size of\n> +\t * your average binary executable header.\n> +\t */\n> +\tchar firstline[80];\n> +\tchar *interpreter = NULL;\n> +\tsize_t s, i;\n> +\n> +\tf = fopen(cmd, \"r\");\n> +\tif (!f) {\n> +\t\terror(\"cannot open file '%s': %s\\n\", cmd, strerror(errno));\n> +\t\treturn;\n> +\t}\n> +\n> +\ts = fread(firstline, 1, sizeof(firstline), f);\n> +\tif (s < 2) {\n> +\t\ttrace_printf(\"cannot determine file type\");\n> +\t\tfclose(f);\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (firstline[0] != '#' || firstline[1] != '!') {\n> +\t\ttrace_printf(\"file '%s' is not a script or\"\n> +\t\t\t\t\" is a script without '#!'\", cmd);\n> +\t\tfclose(f);\n> +\t\treturn;\n> +\t}\n\nNice touches to silently pass scripts that do not begin with she-bang.\n\n> +\n> +\t/* see if the given path has the executable bit set */\n> +\tfor (i = 2; i < s; i++) {\n> +\t\tif (!interpreter && firstline[i] != ' ' && firstline[i] != '\\t')\n> +\t\t\tinterpreter = firstline + i;\n> +\n> +\t\tif (interpreter && (firstline[i] == ' ' ||\n> +\t\t\t\tfirstline[i] == '\\n')) {\n\nCurious.\n\n\"#!<TAB>/bin/bash<TAB><LF>\" would cause you to check \"/bin/bash<TAB>\"?\n\n> +\t\t\tstrbuf_add(&sb, interpreter,\n> +\t\t\t\t\t(firstline + i) - interpreter);\n> +\t\t\tbreak;\n> +\t\t}\n\nWouldn't strcspn() work better instead of this loop?\n\n> +\t}\n> +\tif (!sb.len) {\n> +\t\terror(\"could not determine interpreter\");\n> +\t\tstrbuf_release(&sb);\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (!have_read_execute_permissions(sb.buf))\n> +\t\terror(\"bad interpreter: no read/execute permissions on '%s'\\n\",\n> +\t\t\t\tsb.buf);\n> +\n> +\tstrbuf_release(&sb);\n> +}\n> +\n>  static void diagnose_execvp_eacces(const char *cmd, const char **argv)\n>  {\n>  \t/* man 2 execve states that EACCES is returned for:\n> @@ -209,8 +266,8 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)\n>  \tchar *next;\n>  \n>  \tif (strchr(cmd, '/')) {\n> -\t\tif (!have_read_execute_permissions(cmd))\n> -\t\t\terror(\"no read/execute permissions on '%s'\\n\", cmd);\n> +\t\tif (have_read_execute_permissions(cmd))\n> +\t\t\tcheck_interpreter(cmd);\n\nI would have expected the overall logic to be more like this:\n\n\tif we cannot read and execute it then\n        \tthat in itself is an error (i.e. the error message from [1/2])\n\telse if we can read it then\n\t\tlet's see if there is an error in the interpreter.\n\nIt is unnatural to see \"if we can read and execute, then see if there is\nanything wrong with the interpreter\" and _nothing else_ here. If you made\nthe \"have_read_execute_permissions()\" to issue the error message you used\nto give in your [1/2] patch here, that is OK from the point of view of the\noverall code structure, but then the function is no longer \"do we have\npermissions\" boolean check and needs to be renamed. And if you didn't,\nthen I have to wonder why we do not need the error message you added in\nyour [1/2].\n\n> @@ -233,10 +290,7 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)\n>  \t\t\t\terror(\"no read/execute permissions on '%s'\\n\",\n>  \t\t\t\t\t\tsb.buf);\n>  \t\t\telse\n> -\t\t\t\twarn(\"file '%s' exists and permissions \"\n> -\t\t\t\t\"seem OK.\\nIf this is a script, see if you \"\n> -\t\t\t\t\"have sufficient privileges to run the \"\n> -\t\t\t\t\"interpreter\", sb.buf);\n> +\t\t\t\tcheck_interpreter(sb.buf);\n>  \t\t}\n>  \n>  \t\tstrbuf_release(&sb);\n> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\n> index b39bd16..39bfaef 100755\n> --- a/t/t0061-run-command.sh\n> +++ b/t/t0061-run-command.sh\n> @@ -13,6 +13,18 @@ cat >hello-script <<-EOF\n>  EOF\n>  >empty\n>  \n> +cat >someinterpreter <<-EOF\n> +\t#!$SHELL_PATH\n> +\tcat hello-script\n> +EOF\n> +>empty\n> +\n> +cat >incorrect-interpreter-script <<-EOF\n> +\t#!someinterpreter\n> +\tcat hello-script\n> +EOF\n> +>empty\n> +\n>  test_expect_success 'start_command reports ENOENT' '\n>  \ttest-run-command start-command-ENOENT ./does-not-exist\n>  '\n> @@ -48,4 +60,14 @@ test_expect_success POSIXPERM 'run_command reports EACCES, search path permision\n>  \tgrep \"no read/execute permissions on\" err\n>  '\n>  \n> +test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '\n> +\tcat incorrect-interpreter-script >hello.sh &&\n> +\tchmod +x hello.sh &&\n> +\tchmod -x someinterpreter &&\n> +\ttest_must_fail test-run-command run-command ./hello.sh 2>err &&\n> +\n> +\tgrep \"fatal: cannot exec.*hello.sh\" err &&\n> +\tgrep \"bad interpreter\" err\n> +'\n> +\n>  test_done\n"},{"id":"180471","messageId":"CAH6sp9NsRDWoMtnBUXOP-OMFwjjUm-OuRLpNvcS4pC1S=C93EQ@mail.gmail.com","threadId":"28996","inReplyTo":"7vpqg1e3au.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-12-07T08:31:24Z","receivedAt":"2011-12-07T08:31:24Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"Thanks for the review. There's a lot of things you mention that I\neither didn't see (staring blind, you know) or that I didn't know of.\n\nOn Tue, Dec 6, 2011 at 11:35 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Frans Klaver <fransklaver@gmail.com> writes:\n>\n>> +#ifndef WIN32\n>> +static int is_in_group(gid_t gid)\n>> ...\n>> +static int have_read_execute_permissions(const char *path)\n>> +{\n>> +     struct stat s;\n>> +     trace_printf(\"checking '%s'\\n\", path);\n>> +\n>> +     if (stat(path, &s) < 0) {\n>> + ...\n>> +     /* check world permissions */\n>> +     if ((s.st_mode&(S_IXOTH|S_IROTH)) == (S_IXOTH|S_IROTH))\n>> +             return 1;\n>\n> Hmm, do you need to do this with stat(2)?\n>\n> Wouldn't access(2) with R_OK|X_OK give you exactly what you want without\n> this much trouble?\n\nProbably. I'll use access instead in a reroll.\n\n\n> I also think that your permission check is incorrectly implemented.\n>\n>    $ cd /var/tmp && date >j && chmod 044 j && ls -l j\n>    ----r--r-- 1 junio junio 29 Dec  6 14:32 j\n>    $ cat j\n>    cat: j: Permission denied\n>    $ su pogo\n>    Password:\n>    $ cat j\n>    Tue Dec  6 14:32:23 PST 2011\n>\n> That's a world-readable but unreadable-only-to-me file.\n\nHmm, this is a case that didn't fit my expectations. Thanks for catching.\n\n\n\n>> +static void diagnose_execvp_eacces(const char *cmd, const char **argv)\n>> +{\n>> +     /* man 2 execve states that EACCES is returned for:\n>\n>        /*\n>         * Just a style, but we tend to write multi-line comment like\n>         * this, without anything else on opening and closing lines of\n>         * the comment block.\n>         */\n>\n>> +      * - The file system is mounted noexec\n>> +      */\n>> +     struct strbuf sb = STRBUF_INIT;\n>> +     char *path = getenv(\"PATH\");\n>> +     char *next;\n>> +\n>> +     if (strchr(cmd, '/')) {\n>> +             if (!have_read_execute_permissions(cmd))\n>> +                     error(\"no read/execute permissions on '%s'\\n\", cmd);\n>> +             return;\n>> +     }\n>\n> Ok, execvp() failed and \"cmd\" has at least one slash, so we know we did\n> not look for it in $PATH.  We check only one and return (did you need\n> getenv() in that case?).\n\nObviously not. Missed that.\n\n>\n>> +     for (;;) {\n>> +             next = strchrnul(path, ':');\n>> +             if (path < next)\n>> +                     strbuf_add(&sb, path, next - path);\n>> +             else\n>> +                     strbuf_addch(&sb, '.');\n>\n> Nice touch that you did not forget an empty component on $PATH.\n\nYes, that's a relic from me starting work based on one of your\nproposed patches[1]. So that one goes to you.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/171838\n\n\n>> +             if (!have_read_execute_permissions(sb.buf))\n>> +                     error(\"no read/execute permissions on '%s'\\n\", sb.buf);\n>\n> Don't you want to continue here upon error, after resetting sb? You just\n> saw the directory is unreadble, so you know next file_exists() will fail\n> before you try it.\n\nYes. I thought about that. I didn't do that because of the fact that I\nhad to do more than just resetting sb. The path variable has to be\nupdated as well. I had the choice of adding a level of indentation {},\nduplicating the code, or just do a check I know before will fail.\nThere's probably something to say for each one of them. I'll probably\nrefactor that a bit more.\n\n\n>> +             if (sb.len && sb.buf[sb.len - 1] != '/')\n>> +                     strbuf_addch(&sb, '/');\n>> +             strbuf_addstr(&sb, cmd);\n>> +\n>> +             if (file_exists(sb.buf)) {\n>> +                     if (!have_read_execute_permissions(sb.buf))\n>> +                             error(\"no read/execute permissions on '%s'\\n\",\n>> +                                             sb.buf);\n>> +                     else\n>> +                             warn(\"file '%s' exists and permissions \"\n>> +                             \"seem OK.\\nIf this is a script, see if you \"\n>> +                             \"have sufficient privileges to run the \"\n>> +                             \"interpreter\", sb.buf);\n>\n> Does \"warn()\" do the right thing for multi-line strings like this?\n\nI don't know/remember. It seemed like a natural thing to do, but I'll find out.\n"},{"id":"180472","messageId":"CAH6sp9MqwKppcrtP7YM8FZAs=odmUicTvsxiYyH0ENmJrPxqEA@mail.gmail.com","threadId":"28996","inReplyTo":"7vk469e2rn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] run-command: Add interpreter permissions check","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-12-07T08:37:48Z","receivedAt":"2011-12-07T08:37:48Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Tue, Dec 6, 2011 at 11:47 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Frans Klaver <fransklaver@gmail.com> writes:\n>\n>> If a script is started and the interpreter of that script given in the\n>> shebang cannot be started due to permissions, we can get a rather\n>> obscure situation. All permission checks pass for the script itself,\n>> but we still get EACCES from execvp.\n>>\n>> Try to find out if the above is the case and warn the user about it.\n>>\n>> Signed-off-by: Frans Klaver <fransklaver@gmail.com>\n>> ---\n>>  run-command.c          |   66 +++++++++++++++++++++++++++++++++++++++++++----\n>>  t/t0061-run-command.sh |   22 ++++++++++++++++\n>>  2 files changed, 82 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/run-command.c b/run-command.c\n>> index 5e38c5a..b8cf8d4 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -194,6 +194,63 @@ static int have_read_execute_permissions(const char *path)\n>>       return 0;\n>>  }\n>>\n>> +static void check_interpreter(const char *cmd)\n>> +{\n>> +     FILE *f;\n>> +     struct strbuf sb = STRBUF_INIT;\n>> +     /* bash reads an 80 character line when determining the interpreter.\n>> +      * BSD apparently only allows 32 characters, as it is the size of\n>> +      * your average binary executable header.\n>> +      */\n>> +     char firstline[80];\n>> +     char *interpreter = NULL;\n>> +     size_t s, i;\n>> +\n>> +     f = fopen(cmd, \"r\");\n>> +     if (!f) {\n>> +             error(\"cannot open file '%s': %s\\n\", cmd, strerror(errno));\n>> +             return;\n>> +     }\n>> +\n>> +     s = fread(firstline, 1, sizeof(firstline), f);\n>> +     if (s < 2) {\n>> +             trace_printf(\"cannot determine file type\");\n>> +             fclose(f);\n>> +             return;\n>> +     }\n>> +\n>> +     if (firstline[0] != '#' || firstline[1] != '!') {\n>> +             trace_printf(\"file '%s' is not a script or\"\n>> +                             \" is a script without '#!'\", cmd);\n>> +             fclose(f);\n>> +             return;\n>> +     }\n>\n> Nice touches to silently pass scripts that do not begin with she-bang.\n>\n>> +\n>> +     /* see if the given path has the executable bit set */\n>> +     for (i = 2; i < s; i++) {\n>> +             if (!interpreter && firstline[i] != ' ' && firstline[i] != '\\t')\n>> +                     interpreter = firstline + i;\n>> +\n>> +             if (interpreter && (firstline[i] == ' ' ||\n>> +                             firstline[i] == '\\n')) {\n>\n> Curious.\n>\n> \"#!<TAB>/bin/bash<TAB><LF>\" would cause you to check \"/bin/bash<TAB>\"?\n\nApparently so. Thanks for catching.\n\n\n>> +                     strbuf_add(&sb, interpreter,\n>> +                                     (firstline + i) - interpreter);\n>> +                     break;\n>> +             }\n>\n> Wouldn't strcspn() work better instead of this loop?\n\nProbably. Will revise.\n\n\n>> +     }\n>> +     if (!sb.len) {\n>> +             error(\"could not determine interpreter\");\n>> +             strbuf_release(&sb);\n>> +             return;\n>> +     }\n>> +\n>> +     if (!have_read_execute_permissions(sb.buf))\n>> +             error(\"bad interpreter: no read/execute permissions on '%s'\\n\",\n>> +                             sb.buf);\n>> +\n>> +     strbuf_release(&sb);\n>> +}\n>> +\n>>  static void diagnose_execvp_eacces(const char *cmd, const char **argv)\n>>  {\n>>       /* man 2 execve states that EACCES is returned for:\n>> @@ -209,8 +266,8 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)\n>>       char *next;\n>>\n>>       if (strchr(cmd, '/')) {\n>> -             if (!have_read_execute_permissions(cmd))\n>> -                     error(\"no read/execute permissions on '%s'\\n\", cmd);\n>> +             if (have_read_execute_permissions(cmd))\n>> +                     check_interpreter(cmd);\n>\n> I would have expected the overall logic to be more like this:\n>\n>        if we cannot read and execute it then\n>                that in itself is an error (i.e. the error message from [1/2])\n>        else if we can read it then\n>                let's see if there is an error in the interpreter.\n>\n> It is unnatural to see \"if we can read and execute, then see if there is\n> anything wrong with the interpreter\" and _nothing else_ here. If you made\n> the \"have_read_execute_permissions()\" to issue the error message you used\n> to give in your [1/2] patch here, that is OK from the point of view of the\n> overall code structure, but then the function is no longer \"do we have\n> permissions\" boolean check and needs to be renamed. And if you didn't,\n> then I have to wonder why we do not need the error message you added in\n> your [1/2].\n\nHm, yea makes sense. I'll rethink this a bit.\n\nAgain, thanks for the review.\n"},{"id":"180619","messageId":"op.v56xbxqs0aolir@keputer","threadId":"28996","inReplyTo":"7vpqg1e3au.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-12-08T21:44:11Z","receivedAt":"2011-12-08T21:44:11Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Tue, 06 Dec 2011 23:35:53 +0100, Junio C Hamano <gitster@pobox.com>  \nwrote:\n\n> Frans Klaver <fransklaver@gmail.com> writes:\n>\n>> +#ifndef WIN32\n>> +static int is_in_group(gid_t gid)\n>> ...\n>> +static int have_read_execute_permissions(const char *path)\n>> +{\n>> +\tstruct stat s;\n>> +\ttrace_printf(\"checking '%s'\\n\", path);\n>> +\n>> +\tif (stat(path, &s) < 0) {\n>> + ...\n>> +\t/* check world permissions */\n>> +\tif ((s.st_mode&(S_IXOTH|S_IROTH)) == (S_IXOTH|S_IROTH))\n>> +\t\treturn 1;\n>\n> Hmm, do you need to do this with stat(2)?\n>\n> Wouldn't access(2) with R_OK|X_OK give you exactly what you want without\n> this much trouble?\n\nI just had a good look through the man page of access(2), and I think it  \ndepends. access works for the real uid, which is what I attempted to  \nimplement in the above check as well. However, do we actually need to use  \nthe real uid or do we need the set uid (geteuid(2))? Would it be safe to  \nassume we don't setuid?\n\n\n> I also think that your permission check is incorrectly implemented.\n>\n>     $ cd /var/tmp && date >j && chmod 044 j && ls -l j\n>     ----r--r-- 1 junio junio 29 Dec  6 14:32 j\n>     $ cat j\n>     cat: j: Permission denied\n>     $ su pogo\n>     Password:\n>     $ cat j\n>     Tue Dec  6 14:32:23 PST 2011\n> That's a world-readable but unreadable-only-to-me file.\n\nWill fix if we can't use access(2) due to what I mentioned above.\n\n\n\n>> +\t\t\t\twarn(\"file '%s' exists and permissions \"\n>> +\t\t\t\t\"seem OK.\\nIf this is a script, see if you \"\n>> +\t\t\t\t\"have sufficient privileges to run the \"\n>> +\t\t\t\t\"interpreter\", sb.buf);\n>\n> Does \"warn()\" do the right thing for multi-line strings like this?\n\nLooking back on it, I think I actually wanted to use warning() from  \nusage.c. I'll still have to check if that does the multi-line thing as I  \nexpect it to.\n"},{"id":"180674","messageId":"7vaa71hd5l.fsf@alter.siamese.dyndns.org","threadId":"28996","inReplyTo":"op.v56xbxqs0aolir@keputer","subject":"Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-09T17:23:50Z","receivedAt":"2011-12-09T17:23:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Frans Klaver\" <fransklaver@gmail.com> writes:\n\n>> Wouldn't access(2) with R_OK|X_OK give you exactly what you want without\n>> this much trouble?\n>\n> I just had a good look through the man page of access(2), and I think\n> it depends. access works for the real uid, which is what I attempted\n> to implement in the above check as well. However, do we actually need\n> to use the real uid or do we need the set uid (geteuid(2))?\n\nDoes it matter? We do not use seteuid or setegid ourselves and we do not\nexpect to be installed as owned by root with u+s bit set.\n\naccess(2) checks with real uid exactly because it would not make a\ndifference to normal user level programs _and_ it makes it easier for a\nsuid programs to check with the real identity, and our use case falls into\nthe former, no?\n"},{"id":"180719","messageId":"op.v58rlrrm0aolir@keputer","threadId":"28996","inReplyTo":"7vaa71hd5l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-12-09T21:35:41Z","receivedAt":"2011-12-09T21:35:41Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Fri, 09 Dec 2011 18:23:50 +0100, Junio C Hamano <gitster@pobox.com>  \nwrote:\n\n> \"Frans Klaver\" <fransklaver@gmail.com> writes:\n>\n>>> Wouldn't access(2) with R_OK|X_OK give you exactly what you want  \n>>> without\n>>> this much trouble?\n>>\n>> I just had a good look through the man page of access(2), and I think\n>> it depends. access works for the real uid, which is what I attempted\n>> to implement in the above check as well. However, do we actually need\n>> to use the real uid or do we need the set uid (geteuid(2))?\n>\n> Does it matter? We do not use seteuid or setegid ourselves and we do not\n> expect to be installed as owned by root with u+s bit set.\n\nThat's what I thought, but needed to know for sure that this was the case.\n\n\n> access(2) checks with real uid exactly because it would not make a\n> difference to normal user level programs _and_ it makes it easier for a\n> suid programs to check with the real identity, and our use case falls  \n> into the former, no?\n>\n\nCertainly looks like. Thanks. I'll reroll somewhere next week.\n\nFrans\n"},{"id":"181044","messageId":"1323788917-4141-1-git-send-email-fransklaver@gmail.com","threadId":"28996","inReplyTo":"op.v5e8mgbc0aolir@keputer","subject":"[PATCH 0/2 v2] run-command: Add eacces diagnostics","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-12-13T15:08:35Z","receivedAt":"2011-12-13T15:08:35Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"This replaces $gmane/186388\n\nI had a lot of short stints incorporating the review remarks, so I might just\nhave missed something.\n\n[PATCH 1/2] run-command: Add checks after execvp fails with EACCES\n[PATCH 2/2] run-command: Add interpreter permissions check\n\nrun-command.c          |  130 ++++++++++++++++++++++++++++++++++++++++++++++++\nt/t0061-run-command.sh |   38 ++++++++++++++-\n2 files changed, 167 insertions(+), 1 deletions(-)\n"},{"id":"181045","messageId":"1323788917-4141-2-git-send-email-fransklaver@gmail.com","threadId":"28996","inReplyTo":"1323788917-4141-1-git-send-email-fransklaver@gmail.com","subject":"[PATCH 1/2] run-command: Add checks after execvp fails with EACCES","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-12-13T15:08:36Z","receivedAt":"2011-12-13T15:08:36Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"execvp returns ENOENT if a command was not found after searching PATH.\nIf path contains a directory that current user has insufficient\nprivileges to, EACCES is returned. This may still mean the program\nwasn't found and may cause confusion to the user, especially when the\nfile mentioned doesn't exist -- that is, the user would expect NOENT to\nbe returned -- and the user was actually hoping for an alias to be executed.\n\nTo help users track down the core issue more easily, perform some checks\non the path and file permissions involved. Output errors when paths or\nfiles don't have enough permissions.\n\nSigned-off-by: Frans Klaver <fransklaver@gmail.com>\n---\n run-command.c          |   79 ++++++++++++++++++++++++++++++++++++++++++++++++\n t/t0061-run-command.sh |   16 +++++++++-\n 2 files changed, 94 insertions(+), 1 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 1c51043..3f136f4 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -2,6 +2,7 @@\n #include \"run-command.h\"\n #include \"exec_cmd.h\"\n #include \"argv-array.h\"\n+#include \"dir.h\"\n \n static inline void close_pair(int fd[2])\n {\n@@ -134,6 +135,80 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n \treturn code;\n }\n \n+#ifndef WIN32\n+static int have_read_execute_permissions(const char *path)\n+{\n+\tif (access(path, R_OK|X_OK) == 0)\n+\t\treturn 1;\n+\n+\tif (errno == EACCES)\n+\t\treturn 0;\n+\n+\ttrace_printf(\"could not determine permissions for '%s': %s\\n\", path,\n+\t\t\t\tstrerror(errno));\n+\treturn 0;\n+}\n+\n+static void diagnose_execvp_eacces(const char *cmd, const char **argv)\n+{\n+\t/*\n+\t * man 2 execve states that EACCES is returned for:\n+\t * - Search permission is denied on a component of the path prefix\n+\t *   of cmd or the name of a script interpreter\n+\t * - The file or script interpreter is not a regular file\n+\t * - Execute permission is denied for the file, script or ELF\n+\t *   interpreter\n+\t * - The file system is mounted noexec\n+\t */\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tchar *path;\n+\tchar *next;\n+\n+\tif (strchr(cmd, '/')) {\n+\t\tif (!have_read_execute_permissions(cmd))\n+\t\t\terror(\"no read/execute permissions on '%s'\\n\", cmd);\n+\t\treturn;\n+\t}\n+\n+\tpath = getenv(\"PATH\");\n+\twhile (path) {\n+\t\tnext = strchrnul(path, ':');\n+\t\tif (path < next)\n+\t\t\tstrbuf_add(&sb, path, next - path);\n+\t\telse\n+\t\t\tstrbuf_addch(&sb, '.');\n+\n+\t\tif (!*next)\n+\t\t\tpath = NULL;\n+\t\telse\n+\t\t\tpath = next + 1;\n+\n+\t\tif (!have_read_execute_permissions(sb.buf)) {\n+\t\t\terror(\"no read/execute permissions on '%s'\\n\", sb.buf);\n+\t\t\tstrbuf_release(&sb);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (sb.len && sb.buf[sb.len - 1] != '/')\n+\t\t\tstrbuf_addch(&sb, '/');\n+\t\tstrbuf_addstr(&sb, cmd);\n+\n+\t\tif (file_exists(sb.buf)) {\n+\t\t\tif (!have_read_execute_permissions(sb.buf))\n+\t\t\t\terror(\"no read/execute permissions on '%s'\\n\",\n+\t\t\t\t\t\tsb.buf);\n+\t\t\telse\n+\t\t\t\twarning(\"file '%s' exists and permissions \"\n+\t\t\t\t\"seem OK.\\nIf this is a script, see if you \"\n+\t\t\t\t\"have sufficient privileges to run the \"\n+\t\t\t\t\"interpreter\", sb.buf);\n+\t\t}\n+\n+\t\tstrbuf_release(&sb);\n+\t}\n+}\n+#endif\n+\n int start_command(struct child_process *cmd)\n {\n \tint need_in, need_out, need_err;\n@@ -285,6 +360,10 @@ fail_pipe:\n \t\t\t\terror(\"cannot run %s: %s\", cmd->argv[0],\n \t\t\t\t\tstrerror(ENOENT));\n \t\t\texit(127);\n+\t\t} else if (errno == EACCES) {\n+\t\t\tdiagnose_execvp_eacces(cmd->argv[0], cmd->argv);\n+\t\t\tdie(\"cannot exec '%s': %s\", cmd->argv[0],\n+\t\t\t\tstrerror(EACCES));\n \t\t} else {\n \t\t\tdie_errno(\"cannot exec '%s'\", cmd->argv[0]);\n \t\t}\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex 8d4938f..b39bd16 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -26,7 +26,7 @@ test_expect_success 'run_command can run a command' '\n \ttest_cmp empty err\n '\n \n-test_expect_success POSIXPERM 'run_command reports EACCES' '\n+test_expect_success POSIXPERM 'run_command reports EACCES, file permissions' '\n \tcat hello-script >hello.sh &&\n \tchmod -x hello.sh &&\n \ttest_must_fail test-run-command run-command ./hello.sh 2>err &&\n@@ -34,4 +34,18 @@ test_expect_success POSIXPERM 'run_command reports EACCES' '\n \tgrep \"fatal: cannot exec.*hello.sh\" err\n '\n \n+test_expect_success POSIXPERM 'run_command reports EACCES, search path permisions' '\n+\tmkdir -p inaccessible &&\n+\tPATH=$(pwd)/inaccessible:$PATH &&\n+\texport PATH &&\n+\n+\tcat hello-script >inaccessible/hello.sh &&\n+\tchmod 400 inaccessible &&\n+\ttest_must_fail test-run-command run-command hello.sh 2>err &&\n+\tchmod 755 inaccessible &&\n+\n+\tgrep \"fatal: cannot exec.*hello.sh\" err &&\n+\tgrep \"no read/execute permissions on\" err\n+'\n+\n test_done\n-- \n1.7.8\n"},{"id":"181046","messageId":"1323788917-4141-3-git-send-email-fransklaver@gmail.com","threadId":"28996","inReplyTo":"1323788917-4141-1-git-send-email-fransklaver@gmail.com","subject":"[PATCH 2/2] run-command: Add interpreter permissions check","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-12-13T15:08:37Z","receivedAt":"2011-12-13T15:08:37Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"If a script is started and the interpreter of that script given in the\nshebang cannot be started due to permissions, we can get a rather\nobscure situation. All permission checks pass for the script itself,\nbut we still get EACCES from execvp.\n\nTry to find out if the above is the case and warn the user about it.\n\nSigned-off-by: Frans Klaver <fransklaver@gmail.com>\n---\n run-command.c          |   59 ++++++++++++++++++++++++++++++++++++++++++++---\n t/t0061-run-command.sh |   22 ++++++++++++++++++\n 2 files changed, 77 insertions(+), 4 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 3f136f4..9ddd409 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -149,6 +149,55 @@ static int have_read_execute_permissions(const char *path)\n \treturn 0;\n }\n \n+static void check_interpreter(const char *cmd)\n+{\n+\tFILE *f;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\t/*\n+\t * bash reads an 80 character line when determining the interpreter.\n+\t * BSD apparently only allows 32 characters, as it is the size of\n+\t * your average binary executable header.\n+\t */\n+\tchar firstline[80];\n+\tsize_t s, start, end;\n+\n+\tf = fopen(cmd, \"r\");\n+\tif (!f) {\n+\t\terror(\"cannot open file '%s': %s\\n\", cmd, strerror(errno));\n+\t\treturn;\n+\t}\n+\n+\ts = fread(firstline, 1, sizeof(firstline), f);\n+\tif (s < 2) {\n+\t\ttrace_printf(\"cannot determine file type\");\n+\t\tfclose(f);\n+\t\treturn;\n+\t}\n+\n+\tif (firstline[0] != '#' || firstline[1] != '!') {\n+\t\ttrace_printf(\"file '%s' is not a script or\"\n+\t\t\t\t\" is a script without '#!'\", cmd);\n+\t\tfclose(f);\n+\t\treturn;\n+\t}\n+\n+\t/* see if the given path has the executable bit set */\n+\tstart = strspn(&firstline[2], \" \\t\") + 2;\n+\tend = strcspn(&firstline[start], \" \\t\\r\\n\") + start;\n+\tif (start >= end) {\n+\t\terror(\"could not determine interpreter\\n\");\n+\t\treturn;\n+\t}\n+\n+\tstrbuf_add(&sb, &firstline[start], end - start);\n+\n+\tif (!have_read_execute_permissions(sb.buf))\n+\t\terror(\"bad interpreter: no read/execute permissions on '%s'\\n\",\n+\t\t\t\tsb.buf);\n+\n+\tstrbuf_release(&sb);\n+}\n+\n static void diagnose_execvp_eacces(const char *cmd, const char **argv)\n {\n \t/*\n@@ -167,6 +216,8 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)\n \tif (strchr(cmd, '/')) {\n \t\tif (!have_read_execute_permissions(cmd))\n \t\t\terror(\"no read/execute permissions on '%s'\\n\", cmd);\n+\t\telse\n+\t\t\tcheck_interpreter(cmd);\n \t\treturn;\n \t}\n \n@@ -197,11 +248,11 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)\n \t\t\tif (!have_read_execute_permissions(sb.buf))\n \t\t\t\terror(\"no read/execute permissions on '%s'\\n\",\n \t\t\t\t\t\tsb.buf);\n+\t\t\telse if (access(sb.buf, R_OK) == 0)\n+\t\t\t\tcheck_interpreter(sb.buf);\n \t\t\telse\n-\t\t\t\twarning(\"file '%s' exists and permissions \"\n-\t\t\t\t\"seem OK.\\nIf this is a script, see if you \"\n-\t\t\t\t\"have sufficient privileges to run the \"\n-\t\t\t\t\"interpreter\", sb.buf);\n+\t\t\t\ttrace_printf(\"cannot determine interpreter \"\n+\t\t\t\t\t\t\"on '%s'\\n\", sb.buf);\n \t\t}\n \n \t\tstrbuf_release(&sb);\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex b39bd16..39bfaef 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -13,6 +13,18 @@ cat >hello-script <<-EOF\n EOF\n >empty\n \n+cat >someinterpreter <<-EOF\n+\t#!$SHELL_PATH\n+\tcat hello-script\n+EOF\n+>empty\n+\n+cat >incorrect-interpreter-script <<-EOF\n+\t#!someinterpreter\n+\tcat hello-script\n+EOF\n+>empty\n+\n test_expect_success 'start_command reports ENOENT' '\n \ttest-run-command start-command-ENOENT ./does-not-exist\n '\n@@ -48,4 +60,14 @@ test_expect_success POSIXPERM 'run_command reports EACCES, search path permision\n \tgrep \"no read/execute permissions on\" err\n '\n \n+test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '\n+\tcat incorrect-interpreter-script >hello.sh &&\n+\tchmod +x hello.sh &&\n+\tchmod -x someinterpreter &&\n+\ttest_must_fail test-run-command run-command ./hello.sh 2>err &&\n+\n+\tgrep \"fatal: cannot exec.*hello.sh\" err &&\n+\tgrep \"bad interpreter\" err\n+'\n+\n test_done\n-- \n1.7.8\n"},{"id":"181063","messageId":"7vliqguwhq.fsf@alter.siamese.dyndns.org","threadId":"28996","inReplyTo":"1323788917-4141-2-git-send-email-fransklaver@gmail.com","subject":"Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-13T19:01:21Z","receivedAt":"2011-12-13T19:01:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Frans Klaver <fransklaver@gmail.com> writes:\n\n> +static void diagnose_execvp_eacces(const char *cmd, const char **argv)\n> +{\n> +\t/*\n> +\t * man 2 execve states that EACCES is returned for:\n> +\t * - Search permission is denied on a component of the path prefix\n> +\t *   of cmd or the name of a script interpreter\n> +\t * - The file or script interpreter is not a regular file\n> +\t * - Execute permission is denied for the file, script or ELF\n> +\t *   interpreter\n> +\t * - The file system is mounted noexec\n> +\t */\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tchar *path;\n> +\tchar *next;\n> +\n> +\tif (strchr(cmd, '/')) {\n> +\t\tif (!have_read_execute_permissions(cmd))\n> +\t\t\terror(\"no read/execute permissions on '%s'\\n\", cmd);\n> +\t\treturn;\n> +\t}\n> +\n\nThree points.\n\n - error() gives you a LF at the end, so you do not have to have your own.\n\n - That \"have_..._ions()\" is too long and ugly.\n\n - The only thing you care about this callsite is if you have enough\n   permission to execute the \"cmd\".\n\nIn fact, you should not unconditionally require read permissions here.\n\n    $ chmod a-r $(type --path git) && /bin/ls -l $(type --path git)\n    --wx--x--x 109 junio junio 5126580 Dec 13 09:47 /home/junio/git-active/bin/git\n    $ /home/junio/git-active/bin/git --version\n    git version 1.7.8.249.gb1b73\n\nYou may need read permission when the file is a script (i.e. not binary\nexecutable).\n\n> +\tpath = getenv(\"PATH\");\n> +\twhile (path) {\n> +\t\tnext = strchrnul(path, ':');\n> +\t\tif (path < next)\n> +\t\t\tstrbuf_add(&sb, path, next - path);\n> +\t\telse\n> +\t\t\tstrbuf_addch(&sb, '.');\n> +\n> +\t\tif (!*next)\n> +\t\t\tpath = NULL;\n> +\t\telse\n> +\t\t\tpath = next + 1;\n> +\n> +\t\tif (!have_read_execute_permissions(sb.buf)) {\n\nWhen checking if you can run \"foo/bar/baz\", directories \"foo/\" and \"foo/bar/\"\ndo not have to be readable.  They only have to have executable bit to allow\ndescending into them, and typically this is called \"searchable\" (see man chmod).\n\n    $ mkdir -p /var/tmp/a/b && cp $(type --path git) /var/tmp/a/b/git\n    $ chmod 111 /var/tmp/a /var/tmp/a/b\n    $ /var/tmp/a/b/git --version\n    git version 1.7.8.249.gb1b73\n\nI'd suggest having two helper functions, instead of the single one with\noverlong \"have...ions\" name.\n\n - can_search_directory() checks with access(X_OK);\n\n - can_execute_file() checks with access(X_OK|R_OK), even though R_OK is\n   not always needed.\n\nUse the former here where you check the directory that contains the\ncommand, and use the latter up above where you check the command that is\nsupposed to be executable, and also down below after you checked sb.buf is\na path to a file that may be the command that is supposed to be\nexecutable.\n\nThen patch 2/2 can extend can_execute() to enhance its support for scripts\nby reading the hash-bang line and validating it, etc.\n"},{"id":"181139","messageId":"CAH6sp9Mf=EjkVN9mDN59ZCxCU0sCFLa8E=7YxM1J8LCCMr=xYQ@mail.gmail.com","threadId":"28996","inReplyTo":"7vliqguwhq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-12-14T14:31:25Z","receivedAt":"2011-12-14T14:31:25Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Tue, Dec 13, 2011 at 8:01 PM, Junio C Hamano <gitster@pobox.com> wrote:\n\n>\n>  - That \"have_..._ions()\" is too long and ugly.\n\nI half expected that one and I agree. I vaguely remember typing it,\ndeleting it and typing it again when I started on that one.\n\n\n>\n>  - The only thing you care about this callsite is if you have enough\n>   permission to execute the \"cmd\".\n>\n> In fact, you should not unconditionally require read permissions here.\n>\n>    $ chmod a-r $(type --path git) && /bin/ls -l $(type --path git)\n>    --wx--x--x 109 junio junio 5126580 Dec 13 09:47 /home/junio/git-active/bin/git\n>    $ /home/junio/git-active/bin/git --version\n>    git version 1.7.8.249.gb1b73\n>\n> You may need read permission when the file is a script (i.e. not binary\n> executable).\n[...]\n> When checking if you can run \"foo/bar/baz\", directories \"foo/\" and \"foo/bar/\"\n> do not have to be readable.  They only have to have executable bit to allow\n> descending into them, and typically this is called \"searchable\" (see man chmod).\n>\n>    $ mkdir -p /var/tmp/a/b && cp $(type --path git) /var/tmp/a/b/git\n>    $ chmod 111 /var/tmp/a /var/tmp/a/b\n>    $ /var/tmp/a/b/git --version\n>    git version 1.7.8.249.gb1b73\n>\n> I'd suggest having two helper functions, instead of the single one with\n> overlong \"have...ions\" name.\n>\n>  - can_search_directory() checks with access(X_OK);\n>\n>  - can_execute_file() checks with access(X_OK|R_OK), even though R_OK is\n>   not always needed.\n\nOn the whole I like the suggestion. We should probably take it a bit\nfurther. Since the x and r bits basically have nothing to do with each\nother, and we need +rx only on scripts, I could just rely on fopen()\nfor the +r check. I will still add the can_execute_file() and\ncan_search_dir() helpers to support readability, as access(path, X_OK)\nmeans different things in the different contexts. I would then\nprobably go for is_searchable() and is_executable() as function names.\nis_executable then means \"is file and has executable flag set\",\nis_searchable means \"is directory and has executable flag set\".\nBasically files won't be searchable and directories won't be\nexecutable. If execvp fails on a command that is executable, but not\nreadable, it is definitely a script and we can generate an error in\nthat case. 1/2 would then probably use access(path, R_OK), while 2/2\nwould start using fopen.\n\nSince fopen() uses the effective uid/gid, it then makes sense to use\neaccess(3) instead of access(2) if available. It would be stupid to\nhave bugs arise just because of a mismatch between the [ug]ids used by\nthe two access checks. I'm aware of the fact that eaccess isn't a\nstandard function, so a #define HAVE... fallback to at least access()\nwould probably be required.\n\n\n>\n> Use the former here where you check the directory that contains the\n> command, and use the latter up above where you check the command that is\n> supposed to be executable, and also down below after you checked sb.buf is\n> a path to a file that may be the command that is supposed to be\n> executable.\n>\n> Then patch 2/2 can extend can_execute() to enhance its support for scripts\n> by reading the hash-bang line and validating it, etc.\n\nI'd rather keep the hash-bang check outside of that function and use\ncan_execute/is_executable for checking the interpreter as well, if\nonly for keeping the possibility of easily promoting them into an API.\n\nI'd rather move check_interpreter into where it's called now, but pull\nout the logic to find the interpreter. This will keep the error text\ngeneration in diagnose_execvp_eacces. I think the code will make more\nsense this way. There's tons of more errors that can be caused by a\nfaulty interpreter, and it'll be easier to cover more cases this way\nin the future.\n\nThanks for the insightful reviews so far.\n\nLet me know what you think,\nFrans\n"},{"id":"181190","messageId":"op.v6h2cwuw0aolir@keputer.lokaal","threadId":"28996","inReplyTo":"CAH6sp9Mf=EjkVN9mDN59ZCxCU0sCFLa8E=7YxM1J8LCCMr=xYQ@mail.gmail.com","subject":"Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2011-12-14T22:06:22Z","receivedAt":"2011-12-14T22:06:22Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"On Wed, 14 Dec 2011 15:31:25 +0100, Frans Klaver <fransklaver@gmail.com>  \nwrote:\n\n> Since fopen() uses the effective uid/gid, it then makes sense to use\n> eaccess(3) instead of access(2) if available. It would be stupid to\n> have bugs arise just because of a mismatch between the [ug]ids used by\n> the two access checks. I'm aware of the fact that eaccess isn't a\n> standard function, so a #define HAVE... fallback to at least access()\n> would probably be required.\n\nJust to be clear, I don't really want to restart the discussion of which  \nuid to use, but it is something to consider now or in the future. The next  \nroll will use access(2) as far as I'm concerned.\n"}]}