[PATCH] run-command: treat inaccessible directories as ENOENT

When execvp reports EACCES, it can be one of two things:

  1. We found a file to execute, but did not have
     permissions to do so.

  2. We did not have permissions to look in some directory
     in the $PATH.

In the former case, we want to consider this a
permissions problem and report it to the user as such (since
getting this for something like "git foo" is likely a
configuration error).

In the latter case, there is a good chance that the
inaccessible directory does not contain anything of
interest. Reporting "permission denied" is confusing to the
user (and prevents our usual "did you mean...?" lookup). It
also prevents git from trying alias lookup, since we do so
only when an external command does not exist (not when it
exists but has an error).

This patch detects EACCES from execvp, checks whether we are
in case (2), and if so converts errno to ENOENT. This
behavior matches that of "bash" (but not of simpler shells
that use execvp more directly, like "dash").

Test stolen from Junio.

Signed-off-by: Jeff King <redacted>
---
Here's the patch we've been discussing with a commit message and a few
minor modifications:

  1. I split the find-in-path function so that Frans can build on it
     with further checks if he wants. However, note that it will need a
     little more refactoring; it finds the first entry in the PATH,
     whereas I think he would want the first executable entry
     (admittedly, having a non-executable entry followed by an
     executable one which _also_ fails for a different reason is a
     pretty wild corner case).

  2. I pulled the test from what Junio posted earlier. I started to
     write a full test script that checked each of the cases I
     mentioned earlier. However, in all but the alias case (which is
     what is tested here), the behavior is really only distinguishable
     by the error messages, and I didn't want to get into testing what
     strerror(EACCES) prints. I can re-roll if we really want to go
     there.

 cache.h                |    2 ++
 exec_cmd.c             |    2 +-
 run-command.c          |   66 ++++++++++++++++++++++++++++++++++++++++++++++--
 t/t0061-run-command.sh |   13 ++++++++++
 4 files changed, 80 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index e5e1aa4..59e1c44 100644
--- a/cache.h
+++ b/cache.h

@@ -1276,4 +1276,6 @@ extern struct startup_info *startup_info;
 /* builtin/merge.c */
 int checkout_fast_forward(const unsigned char *from, const unsigned char *to);
 
+int sane_execvp(const char *file, char *const argv[]);
+
 #endif /* CACHE_H */

diff --git a/exec_cmd.c b/exec_cmd.c
index 171e841..125fa6f 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c

@@ -134,7 +134,7 @@ int execv_git_cmd(const char **argv) {
 	trace_argv_printf(nargv, "trace: exec:");
 
 	/* execvp() can only ever return if it fails */
-	execvp("git", (char **)nargv);
+	sane_execvp("git", (char **)nargv);
 
 	trace_printf("trace: exec failed: %s\n", strerror(errno));
 

diff --git a/run-command.c b/run-command.c
index 1db8abf..7123436 100644
--- a/run-command.c
+++ b/run-command.c

@@ -76,6 +76,68 @@ static inline void dup_devnull(int to)
 }
 #endif
 
+static char *locate_in_PATH(const char *file)
+{
+	const char *p = getenv("PATH");
+	struct strbuf buf = STRBUF_INIT;
+
+	if (!p || !*p)
+		return 0;
+
+	while (1) {
+		const char *end = strchrnul(p, ':');
+
+		strbuf_reset(&buf);
+
+		/* POSIX specifies an empty entry as the current directory. */
+		if (end != p) {
+			strbuf_add(&buf, p, end - p);
+			strbuf_addch(&buf, '/');
+		}
+		strbuf_addstr(&buf, file);
+
+		if (!access(buf.buf, F_OK))
+			return strbuf_detach(&buf, NULL);
+
+		if (!*end)
+			break;
+		p = end + 1;
+	}
+
+	strbuf_release(&buf);
+	return NULL;
+}
+
+static int exists_in_PATH(const char *file)
+{
+	char *r = locate_in_PATH(file);
+	free(r);
+	return r != NULL;
+}
+
+int sane_execvp(const char *file, char * const argv[])
+{
+	if (!execvp(file, argv))
+		return 0;
+
+	/*
+	 * When a command can't be found because one of the directories
+	 * listed in $PATH is unsearchable, execvp reports EACCES, but
+	 * careful usability testing (read: analysis of occasional bug
+	 * reports) reveals that "No such file or directory" is more
+	 * intuitive.
+	 *
+	 * We avoid commands with "/", because execvp will not do $PATH
+	 * lookups in that case.
+	 *
+	 * The reassignment of EACCES to errno looks like a no-op below,
+	 * but we need to protect against exists_in_PATH overwriting errno.
+	 */
+	if (errno == EACCES && !strchr(file, '/'))
+		errno = exists_in_PATH(file) ? EACCES : ENOENT;
+	return -1;
+}
+
 static const char **prepare_shell_cmd(const char **argv)
 {
 	int argc, nargc = 0;

@@ -114,7 +176,7 @@ static int execv_shell_cmd(const char **argv)
 {
 	const char **nargv = prepare_shell_cmd(argv);
 	trace_argv_printf(nargv, "trace: exec:");
-	execvp(nargv[0], (char **)nargv);
+	sane_execvp(nargv[0], (char **)nargv);
 	free(nargv);
 	return -1;
 }

@@ -339,7 +401,7 @@ fail_pipe:
 		} else if (cmd->use_shell) {
 			execv_shell_cmd(cmd->argv);
 		} else {
-			execvp(cmd->argv[0], (char *const*) cmd->argv);
+			sane_execvp(cmd->argv[0], (char *const*) cmd->argv);
 		}
 		if (errno == ENOENT) {
 			if (!cmd->silent_exec_failure)

diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 8d4938f..17e969d 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh

@@ -34,4 +34,17 @@ test_expect_success POSIXPERM 'run_command reports EACCES' '
 	grep "fatal: cannot exec.*hello.sh" err
 '
 
+test_expect_success POSIXPERM 'unreadable directory in PATH' '
+	mkdir local-command &&
+	test_when_finished "chmod u+rwx local-command && rm -fr local-command" &&
+	git config alias.nitfol "!echo frotz" &&
+	chmod a-rx local-command &&
+	(
+		PATH=./local-command:$PATH &&
+		git nitfol >actual
+	) &&
+	echo frotz >expect &&
+	test_cmp expect actual
+'
+
 test_done

-- 
1.7.9.5.7.g11b89

On Fri, Mar 30, 2012 at 9:52 AM, Jeff King [off-list ref] wrote:

 1. I split the find-in-path function so that Frans can build on it
    with further checks if he wants. However, note that it will need a
    little more refactoring; it finds the first entry in the PATH,
    whereas I think he would want the first executable entry
    (admittedly, having a non-executable entry followed by an
    executable one which _also_ fails for a different reason is a
    pretty wild corner case).

Thanks. I think this corner case is something to fix when someone is
running into it. Bash doesn't cover this case either (stops looking at
the first found entry) so I would go as far as saying that it doesn't
happen.

 2. I pulled the test from what Junio posted earlier. I started to
    write a full test script that checked each of the cases I
    mentioned earlier. However, in all but the alias case (which is
    what is tested here), the behavior is really only distinguishable
    by the error messages, and I didn't want to get into testing what
    strerror(EACCES) prints. I can re-roll if we really want to go
    there.

I wouldn't think there is much point in testing strerror output.

+test_expect_success POSIXPERM 'unreadable directory in PATH' '
+       mkdir local-command &&
+       test_when_finished "chmod u+rwx local-command && rm -fr local-command" &&
+       git config alias.nitfol "!echo frotz" &&
+       chmod a-rx local-command &&
+       (
+               PATH=./local-command:$PATH &&
+               git nitfol >actual
+       ) &&
+       echo frotz >expect &&
+       test_cmp expect actual
+'
+
 test_done
--
1.7.9.5.7.g11b89

Hadn't looked into Junio's test earlier (probably missed it entirely),
but isn't it rather more sensible from a unit-test perspective to see
if start_command returns 127 instead of 128 in this specific case?
Aside from that, this test doesn't seem to fit in t0061, looking at
the t???? guidelines.

Rest looks sensible to me.