[PATCH] run-command.c: Accept EACCES as command not found

[PATCH] run-command.c: Accept EACCES as command not found

[Frans Klaver]

execvp returns ENOENT if a command was not found after searching PATH.
If path contains a directory that current user has insufficient
privileges to, EACCES is returned. This may still mean the program
wasn't found.

If the latter case is encountered, git errors out without giving aliases
a try, which breaks t0001.3 and alias handling in general.

This can be fixed by handling the EACCES case equally to the ENOENT
case.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---

I'm actually not too happy about the location of the tests. I couldn't
find out from the available tests and the documentation where I would
have to create the new ones. For now, I've added the tests to the same
set that I found the issue with.

 run-command.c   |   10 ++++++++--
 t/t0001-init.sh |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/run-command.c b/run-command.c
index 1c51043..ad3c120 100644
--- a/run-command.c
+++ b/run-command.c
@@ -280,12 +280,18 @@ fail_pipe:
 		} else {
 			execvp(cmd->argv[0], (char *const*) cmd->argv);
 		}
-		if (errno == ENOENT) {
+		switch (errno) {
+		case ENOENT:
 			if (!cmd->silent_exec_failure)
 				error("cannot run %s: %s", cmd->argv[0],
 					strerror(ENOENT));
 			exit(127);
-		} else {
+		case EACCES:
+			if (!cmd->silent_exec_failure)
+				error("fatal: cannot exec '%s': %s", cmd->argv[0],
+					strerror(EACCES));
+			exit(127);
+		default:
 			die_errno("cannot exec '%s'", cmd->argv[0]);
 		}
 	}
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index ad66410..d40966a 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -63,6 +63,54 @@ test_expect_success 'plain through aliased command, outside any git repo' '
 	check_config plain-aliased/.git false unset
 '
 
+test_expect_success 'plain through aliased command, inaccessible path, outside any git repo' '
+	(
+		sane_unset GIT_DIR GIT_WORK_TREE &&
+		HOME=$(pwd)/alias-config-path &&
+		export HOME &&
+		mkdir alias-config-path &&
+		echo "[alias] aliasedinit = init" >alias-config-path/.gitconfig &&
+
+		GIT_CEILING_DIRECTORIES=$(pwd) &&
+		export GIT_CEILING_DIRECTORIES &&
+
+		mkdir searchpath &&
+		chmod 400 searchpath &&
+		PATH=$(pwd)/searchpath:$PATH &&
+		export PATH &&
+
+		mkdir plain-aliased-path &&
+		cd plain-aliased-path &&
+		git aliasedinit
+	) &&
+	check_config plain-aliased-path/.git false unset
+'
+
+test_expect_success 'plain through aliased command, inaccessible command, outside any git repo' '
+	(
+		sane_unset GIT_DIR GIT_WORK_TREE &&
+		HOME=$(pwd)/alias-config-cmd &&
+		export HOME &&
+		mkdir alias-config-cmd &&
+		echo "[alias] aliasedinit = init" >alias-config-cmd/.gitconfig &&
+
+		GIT_CEILING_DIRECTORIES=$(pwd) &&
+		export GIT_CEILING_DIRECTORIES &&
+
+		mkdir searchpathcmd &&
+		chmod 755 searchpathcmd &&
+		PATH=$(pwd)/searchpathcmd:$PATH &&
+		export PATH &&
+
+		touch searchpathcmd/git-aliasedinit &&
+
+		mkdir plain-aliased-cmd &&
+		cd plain-aliased-cmd &&
+		git aliasedinit
+	) &&
+	check_config plain-aliased-cmd/.git false unset
+'
+
 test_expect_failure 'plain nested through aliased command' '
 	(
 		sane_unset GIT_DIR GIT_WORK_TREE &&
-- 
1.7.7

Re: [PATCH] run-command.c: Accept EACCES as command not found

[Junio C Hamano]

Frans Klaver <fransklaver@gmail.com> writes:

> execvp returns ENOENT if a command was not found after searching PATH.
> If path contains a directory that current user has insufficient
> privileges to, EACCES is returned. This may still mean the program
> wasn't found.
>
> If the latter case is encountered, git errors out without giving aliases
> a try,...

Isn't that a *good* thing in general, though, so that the user can
diagnose the breakage in the $PATH and fix it?

Re: [PATCH] run-command.c: Accept EACCES as command not found

[Frans Klaver]

On Mon, 21 Nov 2011 23:13:58 +0100, Junio C Hamano <gitster@pobox.com>  
wrote:

> Frans Klaver <fransklaver@gmail.com> writes:
>
>> execvp returns ENOENT if a command was not found after searching PATH.
>> If path contains a directory that current user has insufficient
>> privileges to, EACCES is returned. This may still mean the program
>> wasn't found.
>>
>> If the latter case is encountered, git errors out without giving aliases
>> a try,...
>
> Isn't that a *good* thing in general, though, so that the user can
> diagnose the breakage in the $PATH and fix it?

Actually I went through diagnosing and fixing it. After tracking it down,  
I did wonder about this question myself and I didn't come to a definitive  
conclusion on it. On one hand I do agree that it may be an incentive for  
the user to fix his path. On the other hand I found it an obscure one to  
track down; git's behavior doesn't match bash behavior:

$ git config --global alias.aliasedinit init &&
mkdir searchpath && chmod 400 searchpath && PATH=$(pwd)/searchpath:$PATH  
&& export PATH &&
mkdir someproject && cd someproject &&
git aliasedinit
fatal: cannot exec 'git-aliasedinit': Permission denied

$ git-aliasedinit
bash: git-aliasedinit: command not found

This isn't very intuitive to track down an incorrect PATH with, imo. You  
have to dig into git core code, learn about how git handles commands,  
learn about debugging forked processes, and find out that execvp uses  
EACCES for more than just "permission denied" _just_ to find out you've  
got a wrong environment variable lying about. That's a full day of work  
gone for a newbie. If bash would also tell me in natural language that  
permission was denied, I wouldn't even have considered doing this patch.

For my part the root of the problem lies with the use of EACCES by execvp  
here, not so much with how git uses it. For this particular case, EACCES  
doesn't just mean "it exists, but you cannot execute this", it may also  
mean "not found, but one of the paths could not be accessed". If git were  
to provide a really helpful message, we'd have to detect which paths got  
denied. Once we know that, we can even on the spot decide to error out or  
not. In other words, we'd have to figure out which meaning of EACCES is  
actually used. Based on that, git can error out, warn or ignore at will.

In any case, I thought it best to have other developers have a look at it.  
I can put a bit more of that information in the commit message, but I'd be  
just as happy to drop the patch and keep the exercise.

Frans

Re: [PATCH] run-command.c: Accept EACCES as command not found

[Junio C Hamano]

"Frans Klaver" <fransklaver@gmail.com> writes:

> Actually I went through diagnosing and fixing it. After tracking it
> down, I did wonder about this question myself and I didn't come to a
> definitive  conclusion on it. On one hand I do agree that it may be an
> incentive for  the user to fix his path. On the other hand I found it
> an obscure one to  track down; git's behavior doesn't match bash
> behavior:
>
> $ git config --global alias.aliasedinit init &&
> mkdir searchpath && chmod 400 searchpath &&
> PATH=$(pwd)/searchpath:$PATH && export PATH &&
> mkdir someproject && cd someproject &&
> git aliasedinit
> fatal: cannot exec 'git-aliasedinit': Permission denied

Imagine you did not have alias.aliasedinit in ~/.gitconfig but had a
script called $(pwd)/searchpath/git-aliasedinit which we would fail to
execute. What message would we get in that case? Currently I think we get
permission denied.

Would we get the same with your patch, or something that does not hint
at all that there is a permission problem?

See also the "tangent" part of

    http://thread.gmane.org/gmane.comp.version-control.git/171755

and the discussion that follows it. I do not think we reached any
conclusion nor a patch.

Re: [PATCH] run-command.c: Accept EACCES as command not found

[Frans Klaver]

On Tue, Nov 22, 2011 at 12:54 AM, Junio C Hamano <gitster@pobox.com> wrote:

> Imagine you did not have alias.aliasedinit in ~/.gitconfig but had a
> script called $(pwd)/searchpath/git-aliasedinit which we would fail to
> execute. What message would we get in that case? Currently I think we get
> permission denied.

Correct.

> Would we get the same with your patch, or something that does not hint
> at all that there is a permission problem?

Nope. That would be just as confusing and inarguably incorrect at that
-- bash differentiates between commands that exist, but cannot be
executed due to permissions (access denied) and paths that cannot be
read (they are ignored in the search).


> See also the "tangent" part of
>
>    http://thread.gmane.org/gmane.comp.version-control.git/171755
>
> and the discussion that follows it. I do not think we reached any
> conclusion nor a patch.

There's no black-on-white conclusion there. I get the impression that
no one really has an idea of what they want when encountering EACCES.
Git has to do what's reasonable to provide the user with information.
Currently I think it does too little. Jonathan N. gave the option of
optionally using libexplain[1]. It's pretty verbose and accurate:

fatal: cannot exec 'git-frotz': execvp(pathname = "git-frotz", argv =
["git-frotz"]) failed, Permission denied (13, EACCES) because the
process does not have search permission to the pathname
"/home/frans/devsw/searchpath" directory, the process effective UID
1000 "frans" matches the directory owner UID 1000 "frans" and the
owner permission mode is "r--", and the process is not privileged
(does not have the DAC_READ_SEARCH capability): Success

I wouldn't be in favor of adding the dependency just to enable users
to track down PATH issues though. Also, I think "Cannot access
/home/frans/devsw/searchpath" would just as well do the trick.

For Jonathan's example[2] libexplain doesn't have a clear answer either:
fatal: cannot exec 'git-frotz': execvp(pathname = "git-frotz", argv =
["git-frotz"]) failed, Permission denied (13, EACCES): Permission
denied

If git is going to do some diagnostics on why the execvp returned
EACCES, it can still give a few hints. Most of the more likely options
are then ruled out.

Frans

[1] http://article.gmane.org/gmane.comp.version-control.git/171860
[2] http://article.gmane.org/gmane.comp.version-control.git/171848

Re: [PATCH] run-command.c: Accept EACCES as command not found

[Frans Klaver]

On Tue, Nov 22, 2011 at 10:31 AM, Frans Klaver <fransklaver@gmail.com> wrote:

> If git is going to do some diagnostics on why the execvp returned
> EACCES, it can still give a few hints. Most of the more likely options
> are then ruled out.

If there are no objections, I'm going to cook up a patch that

- Keeps the current behavior (bail on EACCES)
- Adds a more helpful diagnostic message somewhat like libexplain's,
but more terse and if possible with slightly more domain knowledge
- Takes into account the notes made following
http://article.gmane.org/gmane.comp.version-control.git/171838

Frans

Re: [PATCH] run-command.c: Accept EACCES as command not found

[Nguyen Thai Ngoc Duy]

On Wed, Nov 23, 2011 at 3:17 PM, Frans Klaver <fransklaver@gmail.com> wrote:
> If there are no objections, I'm going to cook up a patch that
>
> - Keeps the current behavior (bail on EACCES)
> - Adds a more helpful diagnostic message somewhat like libexplain's,
> but more terse and if possible with slightly more domain knowledge

If you print diagnostic messages with trace_printf() and friends (only
showed when GIT_TRACE variable is set), then there's no need for being
terse.
-- 
Duy

Re: [PATCH] run-command.c: Accept EACCES as command not found

[Frans Klaver]

On Wed, Nov 23, 2011 at 1:04 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:

> If you print diagnostic messages with trace_printf() and friends (only
> showed when GIT_TRACE variable is set), then there's no need for being
> terse.

I'll keep that in mind, thanks.

Frans

Re: [PATCH] run-command.c: Accept EACCES as command not found

[Frans Klaver]

On Wed, 23 Nov 2011 09:17:43 +0100, Frans Klaver <fransklaver@gmail.com>  
wrote:

> On Tue, Nov 22, 2011 at 10:31 AM, Frans Klaver <fransklaver@gmail.com>  
> wrote:
>
>> If git is going to do some diagnostics on why the execvp returned
>> EACCES, it can still give a few hints. Most of the more likely options
>> are then ruled out.
>
> If there are no objections, I'm going to cook up a patch that
>
> - Keeps the current behavior (bail on EACCES)
> - Adds a more helpful diagnostic message somewhat like libexplain's,
> but more terse and if possible with slightly more domain knowledge
> - Takes into account the notes made following
> http://article.gmane.org/gmane.comp.version-control.git/171838

So here be some tests I intend to use (based on t0061.3):

run_command reports EACCES, file permissions:
         cat hello-script >hello.sh &&
         chmod -x hello.sh &&
         test_must_fail test-run-command run-command ./hello.sh 2>err &&

         grep "fatal: cannot exec.*hello.sh" err


run_command reports EACCES, search path permisions:
         mkdir -p inaccessible &&
         PATH=$(pwd)/inaccessible:$PATH &&
         export PATH &&

         cat hello-script >inaccessible/hello.sh &&
         chmod 400 inaccessible &&
         test_must_fail test-run-command run-command hello.sh 2>err &&

         grep "fatal: cannot exec.*hello.sh" err &&
         grep "incorrect PATH entry" err


run_command reports EACCES, interpreter fails:
         cat incorrect-interpreter-script >hello.sh &&
         chmod +x incorrect-interpreter-script &&
         chmod -x someinterpreter &&
         test_must_fail test-run-command run-command ./hello.sh 2>err &&

         grep "fatal: cannot exec.*hello.sh" err &&
         grep "cannot execute interpreter" err


Possibly getting (over)ambitious on the interpreter test, but hey, gotta  
aim high.

If anybody has a test case that isn't covered, I'd be much obliged.

Frans

[PATCH 0/2] run-command: Add EACCES diagnostics

[Frans Klaver]

So here's a couple of patches that introduce some more elaborate investigation
into what went wrong when receiving EACCES. This is probably something that
could be expanded in the future, as running a command doesn't always produce
equally obvious error messages.

"run-command: Add checks after execvp fails with EACCES" provides some basic checks
on the permissions in PATH, and gives just a warning that none of its checks 
indicate a problem, so the user should check at least the interpreter permissions.

"run-command: Add interpreter permissions check" actually adds interpreter checking.

---

 run-command.c          |  172 ++++++++++++++++++++++++++++++++++++++++++++++++
 t/t0061-run-command.sh |   38 ++++++++++-
 2 files changed, 209 insertions(+), 1 deletions(-)

[PATCH 1/2] run-command: Add checks after execvp fails with EACCES

[Frans Klaver]

execvp returns ENOENT if a command was not found after searching PATH.
If path contains a directory that current user has insufficient
privileges to, EACCES is returned. This may still mean the program
wasn't found and may cause confusion to the user, especially when the
file mentioned doesn't exist -- that is, the user would expect NOENT to
be returned -- and the user was actually hoping for an alias to be executed.

To help users track down the core issue more easily, perform some checks
on the path and file permissions involved. Output errors when paths or
files don't have enough permissions.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 run-command.c          |  118 ++++++++++++++++++++++++++++++++++++++++++++++++
 t/t0061-run-command.sh |   16 ++++++-
 2 files changed, 133 insertions(+), 1 deletions(-)

diff --git a/run-command.c b/run-command.c
index 1c51043..5e38c5a 100644
--- a/run-command.c
+++ b/run-command.c
@@ -2,6 +2,7 @@
 #include "run-command.h"
 #include "exec_cmd.h"
 #include "argv-array.h"
+#include "dir.h"
 
 static inline void close_pair(int fd[2])
 {
@@ -134,6 +135,119 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
 	return code;
 }
 
+#ifndef WIN32
+static int is_in_group(gid_t gid)
+{
+	gid_t *groups;
+	int ngroups, gc;
+	int yes;
+
+	if (gid == getgid())
+		return 1;
+
+	groups = NULL;
+	ngroups = getgroups(0, NULL);
+	if (ngroups > 0) {
+		groups = (gid_t *)xmalloc(ngroups * sizeof(gid_t));
+		if (getgroups(ngroups, groups) < 0) {
+			free(groups);
+			return 0;
+		}
+	}
+
+	yes = 0;
+	for (gc = 0; gc < ngroups; gc++)
+		if (groups[gc] == gid)
+			yes = 1;
+
+	free(groups);
+	return yes;
+}
+
+static int have_read_execute_permissions(const char *path)
+{
+	struct stat s;
+	trace_printf("checking '%s'\n", path);
+
+	if (stat(path, &s) < 0) {
+		trace_printf("could not stat '%s': %s\n",
+				path, strerror(errno));
+		return 0;
+	}
+	trace_printf("uid: %d, gid: %d\n", s.st_uid, s.st_gid);
+	trace_printf("mode: %o\n", s.st_mode);
+
+	/* check world permissions */
+	if ((s.st_mode&(S_IXOTH|S_IROTH)) == (S_IXOTH|S_IROTH))
+		return 1;
+
+	/* check group permissions & membership */
+	if ((s.st_mode&(S_IXGRP|S_IRGRP)) == (S_IXGRP|S_IRGRP) &&
+		is_in_group(s.st_gid))
+		return 1;
+
+	/* check owner permissions & ownership */
+	if ((s.st_mode&(S_IXUSR|S_IRUSR)) == (S_IXUSR|S_IRUSR) &&
+		s.st_uid == getuid())
+		return 1;
+
+	return 0;
+}
+
+static void diagnose_execvp_eacces(const char *cmd, const char **argv)
+{
+	/* man 2 execve states that EACCES is returned for:
+	 * - Search permission is denied on a component of the path prefix
+	 *   of cmd or the name of a script interpreter
+	 * - The file or script interpreter is not a regular file
+	 * - Execute permission is denied for the file, script or ELF
+	 *   interpreter
+	 * - The file system is mounted noexec
+	 */
+	struct strbuf sb = STRBUF_INIT;
+	char *path = getenv("PATH");
+	char *next;
+
+	if (strchr(cmd, '/')) {
+		if (!have_read_execute_permissions(cmd))
+			error("no read/execute permissions on '%s'\n", cmd);
+		return;
+	}
+
+	for (;;) {
+		next = strchrnul(path, ':');
+		if (path < next)
+			strbuf_add(&sb, path, next - path);
+		else
+			strbuf_addch(&sb, '.');
+
+		if (!have_read_execute_permissions(sb.buf))
+			error("no read/execute permissions on '%s'\n", sb.buf);
+
+		if (sb.len && sb.buf[sb.len - 1] != '/')
+			strbuf_addch(&sb, '/');
+		strbuf_addstr(&sb, cmd);
+
+		if (file_exists(sb.buf)) {
+			if (!have_read_execute_permissions(sb.buf))
+				error("no read/execute permissions on '%s'\n",
+						sb.buf);
+			else
+				warn("file '%s' exists and permissions "
+				"seem OK.\nIf this is a script, see if you "
+				"have sufficient privileges to run the "
+				"interpreter", sb.buf);
+		}
+
+		strbuf_release(&sb);
+
+		if (!*next)
+			break;
+		path = next + 1;
+	}
+}
+#endif
+
 int start_command(struct child_process *cmd)
 {
 	int need_in, need_out, need_err;
@@ -285,6 +399,10 @@ fail_pipe:
 				error("cannot run %s: %s", cmd->argv[0],
 					strerror(ENOENT));
 			exit(127);
+		} else if (errno == EACCES) {
+			diagnose_execvp_eacces(cmd->argv[0], cmd->argv);
+			die("cannot exec '%s': %s", cmd->argv[0],
+				strerror(EACCES));
 		} else {
 			die_errno("cannot exec '%s'", cmd->argv[0]);
 		}
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 8d4938f..b39bd16 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -26,7 +26,7 @@ test_expect_success 'run_command can run a command' '
 	test_cmp empty err
 '
 
-test_expect_success POSIXPERM 'run_command reports EACCES' '
+test_expect_success POSIXPERM 'run_command reports EACCES, file permissions' '
 	cat hello-script >hello.sh &&
 	chmod -x hello.sh &&
 	test_must_fail test-run-command run-command ./hello.sh 2>err &&
@@ -34,4 +34,18 @@ test_expect_success POSIXPERM 'run_command reports EACCES' '
 	grep "fatal: cannot exec.*hello.sh" err
 '
 
+test_expect_success POSIXPERM 'run_command reports EACCES, search path permisions' '
+	mkdir -p inaccessible &&
+	PATH=$(pwd)/inaccessible:$PATH &&
+	export PATH &&
+
+	cat hello-script >inaccessible/hello.sh &&
+	chmod 400 inaccessible &&
+	test_must_fail test-run-command run-command hello.sh 2>err &&
+	chmod 755 inaccessible &&
+
+	grep "fatal: cannot exec.*hello.sh" err &&
+	grep "no read/execute permissions on" err
+'
+
 test_done
-- 
1.7.8

Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES

[Junio C Hamano]

Frans Klaver <fransklaver@gmail.com> writes:

> +#ifndef WIN32
> +static int is_in_group(gid_t gid)
> ...
> +static int have_read_execute_permissions(const char *path)
> +{
> +	struct stat s;
> +	trace_printf("checking '%s'\n", path);
> +
> +	if (stat(path, &s) < 0) {
> + ...
> +	/* check world permissions */
> +	if ((s.st_mode&(S_IXOTH|S_IROTH)) == (S_IXOTH|S_IROTH))
> +		return 1;

Hmm, do you need to do this with stat(2)?

Wouldn't access(2) with R_OK|X_OK give you exactly what you want without
this much trouble?

I also think that your permission check is incorrectly implemented.

    $ cd /var/tmp && date >j && chmod 044 j && ls -l j
    ----r--r-- 1 junio junio 29 Dec  6 14:32 j
    $ cat j
    cat: j: Permission denied
    $ su pogo
    Password:
    $ cat j
    Tue Dec  6 14:32:23 PST 2011
    
That's a world-readable but unreadable-only-to-me file.

> +static void diagnose_execvp_eacces(const char *cmd, const char **argv)
> +{
> +	/* man 2 execve states that EACCES is returned for:

	/*
         * Just a style, but we tend to write multi-line comment like
         * this, without anything else on opening and closing lines of
         * the comment block.
         */

> +	 * - The file system is mounted noexec
> +	 */
> +	struct strbuf sb = STRBUF_INIT;
> +	char *path = getenv("PATH");
> +	char *next;
> +
> +	if (strchr(cmd, '/')) {
> +		if (!have_read_execute_permissions(cmd))
> +			error("no read/execute permissions on '%s'\n", cmd);
> +		return;
> +	}

Ok, execvp() failed and "cmd" has at least one slash, so we know we did
not look for it in $PATH.  We check only one and return (did you need
getenv() in that case?).

> +	for (;;) {
> +		next = strchrnul(path, ':');
> +		if (path < next)
> +			strbuf_add(&sb, path, next - path);
> +		else
> +			strbuf_addch(&sb, '.');

Nice touch that you did not forget an empty component on $PATH.

> +		if (!have_read_execute_permissions(sb.buf))
> +			error("no read/execute permissions on '%s'\n", sb.buf);

Don't you want to continue here upon error, after resetting sb? You just
saw the directory is unreadble, so you know next file_exists() will fail
before you try it.

> +		if (sb.len && sb.buf[sb.len - 1] != '/')
> +			strbuf_addch(&sb, '/');
> +		strbuf_addstr(&sb, cmd);
> +
> +		if (file_exists(sb.buf)) {
> +			if (!have_read_execute_permissions(sb.buf))
> +				error("no read/execute permissions on '%s'\n",
> +						sb.buf);
> +			else
> +				warn("file '%s' exists and permissions "
> +				"seem OK.\nIf this is a script, see if you "
> +				"have sufficient privileges to run the "
> +				"interpreter", sb.buf);

Does "warn()" do the right thing for multi-line strings like this?

Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES

[Frans Klaver]

Thanks for the review. There's a lot of things you mention that I
either didn't see (staring blind, you know) or that I didn't know of.

On Tue, Dec 6, 2011 at 11:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Frans Klaver <fransklaver@gmail.com> writes:
>
>> +#ifndef WIN32
>> +static int is_in_group(gid_t gid)
>> ...
>> +static int have_read_execute_permissions(const char *path)
>> +{
>> +     struct stat s;
>> +     trace_printf("checking '%s'\n", path);
>> +
>> +     if (stat(path, &s) < 0) {
>> + ...
>> +     /* check world permissions */
>> +     if ((s.st_mode&(S_IXOTH|S_IROTH)) == (S_IXOTH|S_IROTH))
>> +             return 1;
>
> Hmm, do you need to do this with stat(2)?
>
> Wouldn't access(2) with R_OK|X_OK give you exactly what you want without
> this much trouble?

Probably. I'll use access instead in a reroll.


> I also think that your permission check is incorrectly implemented.
>
>    $ cd /var/tmp && date >j && chmod 044 j && ls -l j
>    ----r--r-- 1 junio junio 29 Dec  6 14:32 j
>    $ cat j
>    cat: j: Permission denied
>    $ su pogo
>    Password:
>    $ cat j
>    Tue Dec  6 14:32:23 PST 2011
>
> That's a world-readable but unreadable-only-to-me file.

Hmm, this is a case that didn't fit my expectations. Thanks for catching.



>> +static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>> +{
>> +     /* man 2 execve states that EACCES is returned for:
>
>        /*
>         * Just a style, but we tend to write multi-line comment like
>         * this, without anything else on opening and closing lines of
>         * the comment block.
>         */
>
>> +      * - The file system is mounted noexec
>> +      */
>> +     struct strbuf sb = STRBUF_INIT;
>> +     char *path = getenv("PATH");
>> +     char *next;
>> +
>> +     if (strchr(cmd, '/')) {
>> +             if (!have_read_execute_permissions(cmd))
>> +                     error("no read/execute permissions on '%s'\n", cmd);
>> +             return;
>> +     }
>
> Ok, execvp() failed and "cmd" has at least one slash, so we know we did
> not look for it in $PATH.  We check only one and return (did you need
> getenv() in that case?).

Obviously not. Missed that.

>
>> +     for (;;) {
>> +             next = strchrnul(path, ':');
>> +             if (path < next)
>> +                     strbuf_add(&sb, path, next - path);
>> +             else
>> +                     strbuf_addch(&sb, '.');
>
> Nice touch that you did not forget an empty component on $PATH.

Yes, that's a relic from me starting work based on one of your
proposed patches[1]. So that one goes to you.

[1] http://article.gmane.org/gmane.comp.version-control.git/171838


>> +             if (!have_read_execute_permissions(sb.buf))
>> +                     error("no read/execute permissions on '%s'\n", sb.buf);
>
> Don't you want to continue here upon error, after resetting sb? You just
> saw the directory is unreadble, so you know next file_exists() will fail
> before you try it.

Yes. I thought about that. I didn't do that because of the fact that I
had to do more than just resetting sb. The path variable has to be
updated as well. I had the choice of adding a level of indentation {},
duplicating the code, or just do a check I know before will fail.
There's probably something to say for each one of them. I'll probably
refactor that a bit more.


>> +             if (sb.len && sb.buf[sb.len - 1] != '/')
>> +                     strbuf_addch(&sb, '/');
>> +             strbuf_addstr(&sb, cmd);
>> +
>> +             if (file_exists(sb.buf)) {
>> +                     if (!have_read_execute_permissions(sb.buf))
>> +                             error("no read/execute permissions on '%s'\n",
>> +                                             sb.buf);
>> +                     else
>> +                             warn("file '%s' exists and permissions "
>> +                             "seem OK.\nIf this is a script, see if you "
>> +                             "have sufficient privileges to run the "
>> +                             "interpreter", sb.buf);
>
> Does "warn()" do the right thing for multi-line strings like this?

I don't know/remember. It seemed like a natural thing to do, but I'll find out.

Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES

[Frans Klaver]

On Tue, 06 Dec 2011 23:35:53 +0100, Junio C Hamano <gitster@pobox.com>  
wrote:

> Frans Klaver <fransklaver@gmail.com> writes:
>
>> +#ifndef WIN32
>> +static int is_in_group(gid_t gid)
>> ...
>> +static int have_read_execute_permissions(const char *path)
>> +{
>> +	struct stat s;
>> +	trace_printf("checking '%s'\n", path);
>> +
>> +	if (stat(path, &s) < 0) {
>> + ...
>> +	/* check world permissions */
>> +	if ((s.st_mode&(S_IXOTH|S_IROTH)) == (S_IXOTH|S_IROTH))
>> +		return 1;
>
> Hmm, do you need to do this with stat(2)?
>
> Wouldn't access(2) with R_OK|X_OK give you exactly what you want without
> this much trouble?

I just had a good look through the man page of access(2), and I think it  
depends. access works for the real uid, which is what I attempted to  
implement in the above check as well. However, do we actually need to use  
the real uid or do we need the set uid (geteuid(2))? Would it be safe to  
assume we don't setuid?


> I also think that your permission check is incorrectly implemented.
>
>     $ cd /var/tmp && date >j && chmod 044 j && ls -l j
>     ----r--r-- 1 junio junio 29 Dec  6 14:32 j
>     $ cat j
>     cat: j: Permission denied
>     $ su pogo
>     Password:
>     $ cat j
>     Tue Dec  6 14:32:23 PST 2011
> That's a world-readable but unreadable-only-to-me file.

Will fix if we can't use access(2) due to what I mentioned above.



>> +				warn("file '%s' exists and permissions "
>> +				"seem OK.\nIf this is a script, see if you "
>> +				"have sufficient privileges to run the "
>> +				"interpreter", sb.buf);
>
> Does "warn()" do the right thing for multi-line strings like this?

Looking back on it, I think I actually wanted to use warning() from  
usage.c. I'll still have to check if that does the multi-line thing as I  
expect it to.

Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES

[Junio C Hamano]

"Frans Klaver" <fransklaver@gmail.com> writes:

>> Wouldn't access(2) with R_OK|X_OK give you exactly what you want without
>> this much trouble?
>
> I just had a good look through the man page of access(2), and I think
> it depends. access works for the real uid, which is what I attempted
> to implement in the above check as well. However, do we actually need
> to use the real uid or do we need the set uid (geteuid(2))?

Does it matter? We do not use seteuid or setegid ourselves and we do not
expect to be installed as owned by root with u+s bit set.

access(2) checks with real uid exactly because it would not make a
difference to normal user level programs _and_ it makes it easier for a
suid programs to check with the real identity, and our use case falls into
the former, no?

Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES

[Frans Klaver]

On Fri, 09 Dec 2011 18:23:50 +0100, Junio C Hamano <gitster@pobox.com>  
wrote:

> "Frans Klaver" <fransklaver@gmail.com> writes:
>
>>> Wouldn't access(2) with R_OK|X_OK give you exactly what you want  
>>> without
>>> this much trouble?
>>
>> I just had a good look through the man page of access(2), and I think
>> it depends. access works for the real uid, which is what I attempted
>> to implement in the above check as well. However, do we actually need
>> to use the real uid or do we need the set uid (geteuid(2))?
>
> Does it matter? We do not use seteuid or setegid ourselves and we do not
> expect to be installed as owned by root with u+s bit set.

That's what I thought, but needed to know for sure that this was the case.


> access(2) checks with real uid exactly because it would not make a
> difference to normal user level programs _and_ it makes it easier for a
> suid programs to check with the real identity, and our use case falls  
> into the former, no?
>

Certainly looks like. Thanks. I'll reroll somewhere next week.

Frans

[PATCH 2/2] run-command: Add interpreter permissions check

[Frans Klaver]

If a script is started and the interpreter of that script given in the
shebang cannot be started due to permissions, we can get a rather
obscure situation. All permission checks pass for the script itself,
but we still get EACCES from execvp.

Try to find out if the above is the case and warn the user about it.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 run-command.c          |   66 +++++++++++++++++++++++++++++++++++++++++++----
 t/t0061-run-command.sh |   22 ++++++++++++++++
 2 files changed, 82 insertions(+), 6 deletions(-)

diff --git a/run-command.c b/run-command.c
index 5e38c5a..b8cf8d4 100644
--- a/run-command.c
+++ b/run-command.c
@@ -194,6 +194,63 @@ static int have_read_execute_permissions(const char *path)
 	return 0;
 }
 
+static void check_interpreter(const char *cmd)
+{
+	FILE *f;
+	struct strbuf sb = STRBUF_INIT;
+	/* bash reads an 80 character line when determining the interpreter.
+	 * BSD apparently only allows 32 characters, as it is the size of
+	 * your average binary executable header.
+	 */
+	char firstline[80];
+	char *interpreter = NULL;
+	size_t s, i;
+
+	f = fopen(cmd, "r");
+	if (!f) {
+		error("cannot open file '%s': %s\n", cmd, strerror(errno));
+		return;
+	}
+
+	s = fread(firstline, 1, sizeof(firstline), f);
+	if (s < 2) {
+		trace_printf("cannot determine file type");
+		fclose(f);
+		return;
+	}
+
+	if (firstline[0] != '#' || firstline[1] != '!') {
+		trace_printf("file '%s' is not a script or"
+				" is a script without '#!'", cmd);
+		fclose(f);
+		return;
+	}
+
+	/* see if the given path has the executable bit set */
+	for (i = 2; i < s; i++) {
+		if (!interpreter && firstline[i] != ' ' && firstline[i] != '\t')
+			interpreter = firstline + i;
+
+		if (interpreter && (firstline[i] == ' ' ||
+				firstline[i] == '\n')) {
+			strbuf_add(&sb, interpreter,
+					(firstline + i) - interpreter);
+			break;
+		}
+	}
+	if (!sb.len) {
+		error("could not determine interpreter");
+		strbuf_release(&sb);
+		return;
+	}
+
+	if (!have_read_execute_permissions(sb.buf))
+		error("bad interpreter: no read/execute permissions on '%s'\n",
+				sb.buf);
+
+	strbuf_release(&sb);
+}
+
 static void diagnose_execvp_eacces(const char *cmd, const char **argv)
 {
 	/* man 2 execve states that EACCES is returned for:
@@ -209,8 +266,8 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)
 	char *next;
 
 	if (strchr(cmd, '/')) {
-		if (!have_read_execute_permissions(cmd))
-			error("no read/execute permissions on '%s'\n", cmd);
+		if (have_read_execute_permissions(cmd))
+			check_interpreter(cmd);
 		return;
 	}
 
@@ -233,10 +290,7 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)
 				error("no read/execute permissions on '%s'\n",
 						sb.buf);
 			else
-				warn("file '%s' exists and permissions "
-				"seem OK.\nIf this is a script, see if you "
-				"have sufficient privileges to run the "
-				"interpreter", sb.buf);
+				check_interpreter(sb.buf);
 		}
 
 		strbuf_release(&sb);
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index b39bd16..39bfaef 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -13,6 +13,18 @@ cat >hello-script <<-EOF
 EOF
 >empty
 
+cat >someinterpreter <<-EOF
+	#!$SHELL_PATH
+	cat hello-script
+EOF
+>empty
+
+cat >incorrect-interpreter-script <<-EOF
+	#!someinterpreter
+	cat hello-script
+EOF
+>empty
+
 test_expect_success 'start_command reports ENOENT' '
 	test-run-command start-command-ENOENT ./does-not-exist
 '
@@ -48,4 +60,14 @@ test_expect_success POSIXPERM 'run_command reports EACCES, search path permision
 	grep "no read/execute permissions on" err
 '
 
+test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
+	cat incorrect-interpreter-script >hello.sh &&
+	chmod +x hello.sh &&
+	chmod -x someinterpreter &&
+	test_must_fail test-run-command run-command ./hello.sh 2>err &&
+
+	grep "fatal: cannot exec.*hello.sh" err &&
+	grep "bad interpreter" err
+'
+
 test_done
-- 
1.7.8

Re: [PATCH 2/2] run-command: Add interpreter permissions check

[Junio C Hamano]

Frans Klaver <fransklaver@gmail.com> writes:

> If a script is started and the interpreter of that script given in the
> shebang cannot be started due to permissions, we can get a rather
> obscure situation. All permission checks pass for the script itself,
> but we still get EACCES from execvp.
>
> Try to find out if the above is the case and warn the user about it.
>
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> ---
>  run-command.c          |   66 +++++++++++++++++++++++++++++++++++++++++++----
>  t/t0061-run-command.sh |   22 ++++++++++++++++
>  2 files changed, 82 insertions(+), 6 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 5e38c5a..b8cf8d4 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -194,6 +194,63 @@ static int have_read_execute_permissions(const char *path)
>  	return 0;
>  }
>  
> +static void check_interpreter(const char *cmd)
> +{
> +	FILE *f;
> +	struct strbuf sb = STRBUF_INIT;
> +	/* bash reads an 80 character line when determining the interpreter.
> +	 * BSD apparently only allows 32 characters, as it is the size of
> +	 * your average binary executable header.
> +	 */
> +	char firstline[80];
> +	char *interpreter = NULL;
> +	size_t s, i;
> +
> +	f = fopen(cmd, "r");
> +	if (!f) {
> +		error("cannot open file '%s': %s\n", cmd, strerror(errno));
> +		return;
> +	}
> +
> +	s = fread(firstline, 1, sizeof(firstline), f);
> +	if (s < 2) {
> +		trace_printf("cannot determine file type");
> +		fclose(f);
> +		return;
> +	}
> +
> +	if (firstline[0] != '#' || firstline[1] != '!') {
> +		trace_printf("file '%s' is not a script or"
> +				" is a script without '#!'", cmd);
> +		fclose(f);
> +		return;
> +	}

Nice touches to silently pass scripts that do not begin with she-bang.

> +
> +	/* see if the given path has the executable bit set */
> +	for (i = 2; i < s; i++) {
> +		if (!interpreter && firstline[i] != ' ' && firstline[i] != '\t')
> +			interpreter = firstline + i;
> +
> +		if (interpreter && (firstline[i] == ' ' ||
> +				firstline[i] == '\n')) {

Curious.

"#!<TAB>/bin/bash<TAB><LF>" would cause you to check "/bin/bash<TAB>"?

> +			strbuf_add(&sb, interpreter,
> +					(firstline + i) - interpreter);
> +			break;
> +		}

Wouldn't strcspn() work better instead of this loop?

> +	}
> +	if (!sb.len) {
> +		error("could not determine interpreter");
> +		strbuf_release(&sb);
> +		return;
> +	}
> +
> +	if (!have_read_execute_permissions(sb.buf))
> +		error("bad interpreter: no read/execute permissions on '%s'\n",
> +				sb.buf);
> +
> +	strbuf_release(&sb);
> +}
> +
>  static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>  {
>  	/* man 2 execve states that EACCES is returned for:
> @@ -209,8 +266,8 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>  	char *next;
>  
>  	if (strchr(cmd, '/')) {
> -		if (!have_read_execute_permissions(cmd))
> -			error("no read/execute permissions on '%s'\n", cmd);
> +		if (have_read_execute_permissions(cmd))
> +			check_interpreter(cmd);

I would have expected the overall logic to be more like this:

	if we cannot read and execute it then
        	that in itself is an error (i.e. the error message from [1/2])
	else if we can read it then
		let's see if there is an error in the interpreter.

It is unnatural to see "if we can read and execute, then see if there is
anything wrong with the interpreter" and _nothing else_ here. If you made
the "have_read_execute_permissions()" to issue the error message you used
to give in your [1/2] patch here, that is OK from the point of view of the
overall code structure, but then the function is no longer "do we have
permissions" boolean check and needs to be renamed. And if you didn't,
then I have to wonder why we do not need the error message you added in
your [1/2].

> @@ -233,10 +290,7 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>  				error("no read/execute permissions on '%s'\n",
>  						sb.buf);
>  			else
> -				warn("file '%s' exists and permissions "
> -				"seem OK.\nIf this is a script, see if you "
> -				"have sufficient privileges to run the "
> -				"interpreter", sb.buf);
> +				check_interpreter(sb.buf);
>  		}
>  
>  		strbuf_release(&sb);
> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index b39bd16..39bfaef 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -13,6 +13,18 @@ cat >hello-script <<-EOF
>  EOF
>  >empty
>  
> +cat >someinterpreter <<-EOF
> +	#!$SHELL_PATH
> +	cat hello-script
> +EOF
> +>empty
> +
> +cat >incorrect-interpreter-script <<-EOF
> +	#!someinterpreter
> +	cat hello-script
> +EOF
> +>empty
> +
>  test_expect_success 'start_command reports ENOENT' '
>  	test-run-command start-command-ENOENT ./does-not-exist
>  '
> @@ -48,4 +60,14 @@ test_expect_success POSIXPERM 'run_command reports EACCES, search path permision
>  	grep "no read/execute permissions on" err
>  '
>  
> +test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
> +	cat incorrect-interpreter-script >hello.sh &&
> +	chmod +x hello.sh &&
> +	chmod -x someinterpreter &&
> +	test_must_fail test-run-command run-command ./hello.sh 2>err &&
> +
> +	grep "fatal: cannot exec.*hello.sh" err &&
> +	grep "bad interpreter" err
> +'
> +
>  test_done

Re: [PATCH 2/2] run-command: Add interpreter permissions check

[Frans Klaver]

On Tue, Dec 6, 2011 at 11:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Frans Klaver <fransklaver@gmail.com> writes:
>
>> If a script is started and the interpreter of that script given in the
>> shebang cannot be started due to permissions, we can get a rather
>> obscure situation. All permission checks pass for the script itself,
>> but we still get EACCES from execvp.
>>
>> Try to find out if the above is the case and warn the user about it.
>>
>> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
>> ---
>>  run-command.c          |   66 +++++++++++++++++++++++++++++++++++++++++++----
>>  t/t0061-run-command.sh |   22 ++++++++++++++++
>>  2 files changed, 82 insertions(+), 6 deletions(-)
>>
>> diff --git a/run-command.c b/run-command.c
>> index 5e38c5a..b8cf8d4 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -194,6 +194,63 @@ static int have_read_execute_permissions(const char *path)
>>       return 0;
>>  }
>>
>> +static void check_interpreter(const char *cmd)
>> +{
>> +     FILE *f;
>> +     struct strbuf sb = STRBUF_INIT;
>> +     /* bash reads an 80 character line when determining the interpreter.
>> +      * BSD apparently only allows 32 characters, as it is the size of
>> +      * your average binary executable header.
>> +      */
>> +     char firstline[80];
>> +     char *interpreter = NULL;
>> +     size_t s, i;
>> +
>> +     f = fopen(cmd, "r");
>> +     if (!f) {
>> +             error("cannot open file '%s': %s\n", cmd, strerror(errno));
>> +             return;
>> +     }
>> +
>> +     s = fread(firstline, 1, sizeof(firstline), f);
>> +     if (s < 2) {
>> +             trace_printf("cannot determine file type");
>> +             fclose(f);
>> +             return;
>> +     }
>> +
>> +     if (firstline[0] != '#' || firstline[1] != '!') {
>> +             trace_printf("file '%s' is not a script or"
>> +                             " is a script without '#!'", cmd);
>> +             fclose(f);
>> +             return;
>> +     }
>
> Nice touches to silently pass scripts that do not begin with she-bang.
>
>> +
>> +     /* see if the given path has the executable bit set */
>> +     for (i = 2; i < s; i++) {
>> +             if (!interpreter && firstline[i] != ' ' && firstline[i] != '\t')
>> +                     interpreter = firstline + i;
>> +
>> +             if (interpreter && (firstline[i] == ' ' ||
>> +                             firstline[i] == '\n')) {
>
> Curious.
>
> "#!<TAB>/bin/bash<TAB><LF>" would cause you to check "/bin/bash<TAB>"?

Apparently so. Thanks for catching.


>> +                     strbuf_add(&sb, interpreter,
>> +                                     (firstline + i) - interpreter);
>> +                     break;
>> +             }
>
> Wouldn't strcspn() work better instead of this loop?

Probably. Will revise.


>> +     }
>> +     if (!sb.len) {
>> +             error("could not determine interpreter");
>> +             strbuf_release(&sb);
>> +             return;
>> +     }
>> +
>> +     if (!have_read_execute_permissions(sb.buf))
>> +             error("bad interpreter: no read/execute permissions on '%s'\n",
>> +                             sb.buf);
>> +
>> +     strbuf_release(&sb);
>> +}
>> +
>>  static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>>  {
>>       /* man 2 execve states that EACCES is returned for:
>> @@ -209,8 +266,8 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>>       char *next;
>>
>>       if (strchr(cmd, '/')) {
>> -             if (!have_read_execute_permissions(cmd))
>> -                     error("no read/execute permissions on '%s'\n", cmd);
>> +             if (have_read_execute_permissions(cmd))
>> +                     check_interpreter(cmd);
>
> I would have expected the overall logic to be more like this:
>
>        if we cannot read and execute it then
>                that in itself is an error (i.e. the error message from [1/2])
>        else if we can read it then
>                let's see if there is an error in the interpreter.
>
> It is unnatural to see "if we can read and execute, then see if there is
> anything wrong with the interpreter" and _nothing else_ here. If you made
> the "have_read_execute_permissions()" to issue the error message you used
> to give in your [1/2] patch here, that is OK from the point of view of the
> overall code structure, but then the function is no longer "do we have
> permissions" boolean check and needs to be renamed. And if you didn't,
> then I have to wonder why we do not need the error message you added in
> your [1/2].

Hm, yea makes sense. I'll rethink this a bit.

Again, thanks for the review.

[PATCH 0/2 v2] run-command: Add eacces diagnostics

[Frans Klaver]

This replaces $gmane/186388

I had a lot of short stints incorporating the review remarks, so I might just
have missed something.

[PATCH 1/2] run-command: Add checks after execvp fails with EACCES
[PATCH 2/2] run-command: Add interpreter permissions check

run-command.c          |  130 ++++++++++++++++++++++++++++++++++++++++++++++++
t/t0061-run-command.sh |   38 ++++++++++++++-
2 files changed, 167 insertions(+), 1 deletions(-)

[PATCH 1/2] run-command: Add checks after execvp fails with EACCES

[Frans Klaver]

execvp returns ENOENT if a command was not found after searching PATH.
If path contains a directory that current user has insufficient
privileges to, EACCES is returned. This may still mean the program
wasn't found and may cause confusion to the user, especially when the
file mentioned doesn't exist -- that is, the user would expect NOENT to
be returned -- and the user was actually hoping for an alias to be executed.

To help users track down the core issue more easily, perform some checks
on the path and file permissions involved. Output errors when paths or
files don't have enough permissions.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 run-command.c          |   79 ++++++++++++++++++++++++++++++++++++++++++++++++
 t/t0061-run-command.sh |   16 +++++++++-
 2 files changed, 94 insertions(+), 1 deletions(-)

diff --git a/run-command.c b/run-command.c
index 1c51043..3f136f4 100644
--- a/run-command.c
+++ b/run-command.c
@@ -2,6 +2,7 @@
 #include "run-command.h"
 #include "exec_cmd.h"
 #include "argv-array.h"
+#include "dir.h"
 
 static inline void close_pair(int fd[2])
 {
@@ -134,6 +135,80 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
 	return code;
 }
 
+#ifndef WIN32
+static int have_read_execute_permissions(const char *path)
+{
+	if (access(path, R_OK|X_OK) == 0)
+		return 1;
+
+	if (errno == EACCES)
+		return 0;
+
+	trace_printf("could not determine permissions for '%s': %s\n", path,
+				strerror(errno));
+	return 0;
+}
+
+static void diagnose_execvp_eacces(const char *cmd, const char **argv)
+{
+	/*
+	 * man 2 execve states that EACCES is returned for:
+	 * - Search permission is denied on a component of the path prefix
+	 *   of cmd or the name of a script interpreter
+	 * - The file or script interpreter is not a regular file
+	 * - Execute permission is denied for the file, script or ELF
+	 *   interpreter
+	 * - The file system is mounted noexec
+	 */
+	struct strbuf sb = STRBUF_INIT;
+	char *path;
+	char *next;
+
+	if (strchr(cmd, '/')) {
+		if (!have_read_execute_permissions(cmd))
+			error("no read/execute permissions on '%s'\n", cmd);
+		return;
+	}
+
+	path = getenv("PATH");
+	while (path) {
+		next = strchrnul(path, ':');
+		if (path < next)
+			strbuf_add(&sb, path, next - path);
+		else
+			strbuf_addch(&sb, '.');
+
+		if (!*next)
+			path = NULL;
+		else
+			path = next + 1;
+
+		if (!have_read_execute_permissions(sb.buf)) {
+			error("no read/execute permissions on '%s'\n", sb.buf);
+			strbuf_release(&sb);
+			continue;
+		}
+
+		if (sb.len && sb.buf[sb.len - 1] != '/')
+			strbuf_addch(&sb, '/');
+		strbuf_addstr(&sb, cmd);
+
+		if (file_exists(sb.buf)) {
+			if (!have_read_execute_permissions(sb.buf))
+				error("no read/execute permissions on '%s'\n",
+						sb.buf);
+			else
+				warning("file '%s' exists and permissions "
+				"seem OK.\nIf this is a script, see if you "
+				"have sufficient privileges to run the "
+				"interpreter", sb.buf);
+		}
+
+		strbuf_release(&sb);
+	}
+}
+#endif
+
 int start_command(struct child_process *cmd)
 {
 	int need_in, need_out, need_err;
@@ -285,6 +360,10 @@ fail_pipe:
 				error("cannot run %s: %s", cmd->argv[0],
 					strerror(ENOENT));
 			exit(127);
+		} else if (errno == EACCES) {
+			diagnose_execvp_eacces(cmd->argv[0], cmd->argv);
+			die("cannot exec '%s': %s", cmd->argv[0],
+				strerror(EACCES));
 		} else {
 			die_errno("cannot exec '%s'", cmd->argv[0]);
 		}
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 8d4938f..b39bd16 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -26,7 +26,7 @@ test_expect_success 'run_command can run a command' '
 	test_cmp empty err
 '
 
-test_expect_success POSIXPERM 'run_command reports EACCES' '
+test_expect_success POSIXPERM 'run_command reports EACCES, file permissions' '
 	cat hello-script >hello.sh &&
 	chmod -x hello.sh &&
 	test_must_fail test-run-command run-command ./hello.sh 2>err &&
@@ -34,4 +34,18 @@ test_expect_success POSIXPERM 'run_command reports EACCES' '
 	grep "fatal: cannot exec.*hello.sh" err
 '
 
+test_expect_success POSIXPERM 'run_command reports EACCES, search path permisions' '
+	mkdir -p inaccessible &&
+	PATH=$(pwd)/inaccessible:$PATH &&
+	export PATH &&
+
+	cat hello-script >inaccessible/hello.sh &&
+	chmod 400 inaccessible &&
+	test_must_fail test-run-command run-command hello.sh 2>err &&
+	chmod 755 inaccessible &&
+
+	grep "fatal: cannot exec.*hello.sh" err &&
+	grep "no read/execute permissions on" err
+'
+
 test_done
-- 
1.7.8

Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES

[Junio C Hamano]

Frans Klaver <fransklaver@gmail.com> writes:

> +static void diagnose_execvp_eacces(const char *cmd, const char **argv)
> +{
> +	/*
> +	 * man 2 execve states that EACCES is returned for:
> +	 * - Search permission is denied on a component of the path prefix
> +	 *   of cmd or the name of a script interpreter
> +	 * - The file or script interpreter is not a regular file
> +	 * - Execute permission is denied for the file, script or ELF
> +	 *   interpreter
> +	 * - The file system is mounted noexec
> +	 */
> +	struct strbuf sb = STRBUF_INIT;
> +	char *path;
> +	char *next;
> +
> +	if (strchr(cmd, '/')) {
> +		if (!have_read_execute_permissions(cmd))
> +			error("no read/execute permissions on '%s'\n", cmd);
> +		return;
> +	}
> +

Three points.

 - error() gives you a LF at the end, so you do not have to have your own.

 - That "have_..._ions()" is too long and ugly.

 - The only thing you care about this callsite is if you have enough
   permission to execute the "cmd".

In fact, you should not unconditionally require read permissions here.

    $ chmod a-r $(type --path git) && /bin/ls -l $(type --path git)
    --wx--x--x 109 junio junio 5126580 Dec 13 09:47 /home/junio/git-active/bin/git
    $ /home/junio/git-active/bin/git --version
    git version 1.7.8.249.gb1b73

You may need read permission when the file is a script (i.e. not binary
executable).

> +	path = getenv("PATH");
> +	while (path) {
> +		next = strchrnul(path, ':');
> +		if (path < next)
> +			strbuf_add(&sb, path, next - path);
> +		else
> +			strbuf_addch(&sb, '.');
> +
> +		if (!*next)
> +			path = NULL;
> +		else
> +			path = next + 1;
> +
> +		if (!have_read_execute_permissions(sb.buf)) {

When checking if you can run "foo/bar/baz", directories "foo/" and "foo/bar/"
do not have to be readable.  They only have to have executable bit to allow
descending into them, and typically this is called "searchable" (see man chmod).

    $ mkdir -p /var/tmp/a/b && cp $(type --path git) /var/tmp/a/b/git
    $ chmod 111 /var/tmp/a /var/tmp/a/b
    $ /var/tmp/a/b/git --version
    git version 1.7.8.249.gb1b73

I'd suggest having two helper functions, instead of the single one with
overlong "have...ions" name.

 - can_search_directory() checks with access(X_OK);

 - can_execute_file() checks with access(X_OK|R_OK), even though R_OK is
   not always needed.

Use the former here where you check the directory that contains the
command, and use the latter up above where you check the command that is
supposed to be executable, and also down below after you checked sb.buf is
a path to a file that may be the command that is supposed to be
executable.

Then patch 2/2 can extend can_execute() to enhance its support for scripts
by reading the hash-bang line and validating it, etc.

Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES

[Frans Klaver]

On Tue, Dec 13, 2011 at 8:01 PM, Junio C Hamano <gitster@pobox.com> wrote:

>
>  - That "have_..._ions()" is too long and ugly.

I half expected that one and I agree. I vaguely remember typing it,
deleting it and typing it again when I started on that one.


>
>  - The only thing you care about this callsite is if you have enough
>   permission to execute the "cmd".
>
> In fact, you should not unconditionally require read permissions here.
>
>    $ chmod a-r $(type --path git) && /bin/ls -l $(type --path git)
>    --wx--x--x 109 junio junio 5126580 Dec 13 09:47 /home/junio/git-active/bin/git
>    $ /home/junio/git-active/bin/git --version
>    git version 1.7.8.249.gb1b73
>
> You may need read permission when the file is a script (i.e. not binary
> executable).
[...]
> When checking if you can run "foo/bar/baz", directories "foo/" and "foo/bar/"
> do not have to be readable.  They only have to have executable bit to allow
> descending into them, and typically this is called "searchable" (see man chmod).
>
>    $ mkdir -p /var/tmp/a/b && cp $(type --path git) /var/tmp/a/b/git
>    $ chmod 111 /var/tmp/a /var/tmp/a/b
>    $ /var/tmp/a/b/git --version
>    git version 1.7.8.249.gb1b73
>
> I'd suggest having two helper functions, instead of the single one with
> overlong "have...ions" name.
>
>  - can_search_directory() checks with access(X_OK);
>
>  - can_execute_file() checks with access(X_OK|R_OK), even though R_OK is
>   not always needed.

On the whole I like the suggestion. We should probably take it a bit
further. Since the x and r bits basically have nothing to do with each
other, and we need +rx only on scripts, I could just rely on fopen()
for the +r check. I will still add the can_execute_file() and
can_search_dir() helpers to support readability, as access(path, X_OK)
means different things in the different contexts. I would then
probably go for is_searchable() and is_executable() as function names.
is_executable then means "is file and has executable flag set",
is_searchable means "is directory and has executable flag set".
Basically files won't be searchable and directories won't be
executable. If execvp fails on a command that is executable, but not
readable, it is definitely a script and we can generate an error in
that case. 1/2 would then probably use access(path, R_OK), while 2/2
would start using fopen.

Since fopen() uses the effective uid/gid, it then makes sense to use
eaccess(3) instead of access(2) if available. It would be stupid to
have bugs arise just because of a mismatch between the [ug]ids used by
the two access checks. I'm aware of the fact that eaccess isn't a
standard function, so a #define HAVE... fallback to at least access()
would probably be required.


>
> Use the former here where you check the directory that contains the
> command, and use the latter up above where you check the command that is
> supposed to be executable, and also down below after you checked sb.buf is
> a path to a file that may be the command that is supposed to be
> executable.
>
> Then patch 2/2 can extend can_execute() to enhance its support for scripts
> by reading the hash-bang line and validating it, etc.

I'd rather keep the hash-bang check outside of that function and use
can_execute/is_executable for checking the interpreter as well, if
only for keeping the possibility of easily promoting them into an API.

I'd rather move check_interpreter into where it's called now, but pull
out the logic to find the interpreter. This will keep the error text
generation in diagnose_execvp_eacces. I think the code will make more
sense this way. There's tons of more errors that can be caused by a
faulty interpreter, and it'll be easier to cover more cases this way
in the future.

Thanks for the insightful reviews so far.

Let me know what you think,
Frans

Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES

[Frans Klaver]

On Wed, 14 Dec 2011 15:31:25 +0100, Frans Klaver <fransklaver@gmail.com>  
wrote:

> Since fopen() uses the effective uid/gid, it then makes sense to use
> eaccess(3) instead of access(2) if available. It would be stupid to
> have bugs arise just because of a mismatch between the [ug]ids used by
> the two access checks. I'm aware of the fact that eaccess isn't a
> standard function, so a #define HAVE... fallback to at least access()
> would probably be required.

Just to be clear, I don't really want to restart the discussion of which  
uid to use, but it is something to consider now or in the future. The next  
roll will use access(2) as far as I'm concerned.

[PATCH 2/2] run-command: Add interpreter permissions check

[Frans Klaver]

If a script is started and the interpreter of that script given in the
shebang cannot be started due to permissions, we can get a rather
obscure situation. All permission checks pass for the script itself,
but we still get EACCES from execvp.

Try to find out if the above is the case and warn the user about it.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 run-command.c          |   59 ++++++++++++++++++++++++++++++++++++++++++++---
 t/t0061-run-command.sh |   22 ++++++++++++++++++
 2 files changed, 77 insertions(+), 4 deletions(-)

diff --git a/run-command.c b/run-command.c
index 3f136f4..9ddd409 100644
--- a/run-command.c
+++ b/run-command.c
@@ -149,6 +149,55 @@ static int have_read_execute_permissions(const char *path)
 	return 0;
 }
 
+static void check_interpreter(const char *cmd)
+{
+	FILE *f;
+	struct strbuf sb = STRBUF_INIT;
+	/*
+	 * bash reads an 80 character line when determining the interpreter.
+	 * BSD apparently only allows 32 characters, as it is the size of
+	 * your average binary executable header.
+	 */
+	char firstline[80];
+	size_t s, start, end;
+
+	f = fopen(cmd, "r");
+	if (!f) {
+		error("cannot open file '%s': %s\n", cmd, strerror(errno));
+		return;
+	}
+
+	s = fread(firstline, 1, sizeof(firstline), f);
+	if (s < 2) {
+		trace_printf("cannot determine file type");
+		fclose(f);
+		return;
+	}
+
+	if (firstline[0] != '#' || firstline[1] != '!') {
+		trace_printf("file '%s' is not a script or"
+				" is a script without '#!'", cmd);
+		fclose(f);
+		return;
+	}
+
+	/* see if the given path has the executable bit set */
+	start = strspn(&firstline[2], " \t") + 2;
+	end = strcspn(&firstline[start], " \t\r\n") + start;
+	if (start >= end) {
+		error("could not determine interpreter\n");
+		return;
+	}
+
+	strbuf_add(&sb, &firstline[start], end - start);
+
+	if (!have_read_execute_permissions(sb.buf))
+		error("bad interpreter: no read/execute permissions on '%s'\n",
+				sb.buf);
+
+	strbuf_release(&sb);
+}
+
 static void diagnose_execvp_eacces(const char *cmd, const char **argv)
 {
 	/*
@@ -167,6 +216,8 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)
 	if (strchr(cmd, '/')) {
 		if (!have_read_execute_permissions(cmd))
 			error("no read/execute permissions on '%s'\n", cmd);
+		else
+			check_interpreter(cmd);
 		return;
 	}
 
@@ -197,11 +248,11 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)
 			if (!have_read_execute_permissions(sb.buf))
 				error("no read/execute permissions on '%s'\n",
 						sb.buf);
+			else if (access(sb.buf, R_OK) == 0)
+				check_interpreter(sb.buf);
 			else
-				warning("file '%s' exists and permissions "
-				"seem OK.\nIf this is a script, see if you "
-				"have sufficient privileges to run the "
-				"interpreter", sb.buf);
+				trace_printf("cannot determine interpreter "
+						"on '%s'\n", sb.buf);
 		}
 
 		strbuf_release(&sb);
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index b39bd16..39bfaef 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -13,6 +13,18 @@ cat >hello-script <<-EOF
 EOF
 >empty
 
+cat >someinterpreter <<-EOF
+	#!$SHELL_PATH
+	cat hello-script
+EOF
+>empty
+
+cat >incorrect-interpreter-script <<-EOF
+	#!someinterpreter
+	cat hello-script
+EOF
+>empty
+
 test_expect_success 'start_command reports ENOENT' '
 	test-run-command start-command-ENOENT ./does-not-exist
 '
@@ -48,4 +60,14 @@ test_expect_success POSIXPERM 'run_command reports EACCES, search path permision
 	grep "no read/execute permissions on" err
 '
 
+test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
+	cat incorrect-interpreter-script >hello.sh &&
+	chmod +x hello.sh &&
+	chmod -x someinterpreter &&
+	test_must_fail test-run-command run-command ./hello.sh 2>err &&
+
+	grep "fatal: cannot exec.*hello.sh" err &&
+	grep "bad interpreter" err
+'
+
 test_done
-- 
1.7.8