Bug? Bad permissions in $PATH breaks Git aliases

Bug? Bad permissions in $PATH breaks Git aliases

[James Pickens]

Hi,

I'm not sure if this should be considered a bug or not, but I've noticed that
when my $PATH contains an inaccessible directory, Git fails to execute aliases.
For example:

git config alias.l log
git l
# works fine
PATH=boguspath:$PATH
mkdir boguspath
chmod 000 boguspath
git l
# fatal: cannot exec 'git-l': Permission denied

I lean towards calling it a bug, since my shell doesn't seem to care if there
are inaccessible directories in my $PATH.  It just ignores them, and I think Git
ought to do the same.

James

Re: Bug? Bad permissions in $PATH breaks Git aliases

[Jeff King]

On Mon, Mar 26, 2012 at 04:48:29PM -0700, James Pickens wrote:

> I'm not sure if this should be considered a bug or not, but I've noticed that
> when my $PATH contains an inaccessible directory, Git fails to execute aliases.
> For example:
> 
> git config alias.l log
> git l
> # works fine
> PATH=boguspath:$PATH
> mkdir boguspath
> chmod 000 boguspath
> git l
> # fatal: cannot exec 'git-l': Permission denied

This seems to come up about once a year. The short of it is that execve
will return EACCESS whether the file exists is not actually executable
by you, or if you have an inaccessible element in your PATH. execvp will
continue the search if it sees EACCESS, but will return EACCESS if it
finds nothing.  So git just sees the EACCESS and doesn't know if you
have bogus entries in your PATH or if there is a permissions problem
with your executable files.

For something like a shell, it's not that big a deal; either way, you
couldn't execute the command in question. For git, it matters more
because we first try to exec an external command, and then fall back to
an alias (because externals take precedence over aliases).

So basically our options are:

  1. Start treating EACCESS silently as ENOENT. The downside is that we
     fail to report the proper error when the file really does have
     permissions problems (we would say "command not found", but that is
     misleading).

  2. Implement our own execvp, differentiating between "path not
     available for looking in" and "we found the command, but there was
     a permissions problem". I think somebody was working on this a few
     months ago (search for "add exeecvp failure diagnostics") but it
     seems to have fizzled.

  3. If we get an EACCESS, remember it, try to do the alias lookup, and
     then if that fails, report the "Permission denied" error (not
     "command not found"). That is following the spirit of what execvp
     does (it will find later entries in the PATH if they are there, but
     otherwise will remember the EACCESS error).

>From what I can tell, dash uses stock execvp, and ends up closest to
(3). Bash seems to have implemented their own path lookup, as it will
distinguish between the two cases as in (2):

  $ mkdir /tmp/foo
  $ chmod 0 /tmp/foo
  $ PATH=/tmp/foo:$PATH
  $ dash -c does-not-exist
  dash: 1: does-not-exist: Permission denied
  $ bash -c does-not-exist
  bash: does-not-exist: command not found

  $ chmod 755 /tmp/foo
  $ >/tmp/foo/does-not-exist
  $ chmod 0 /tmp/foo/does-not-exist
  $ dash -c does-not-exist
  dash: 1: does-not-exist: Permission denied
  $ bash -c does-not-exist
  bash: /tmp/foo/does-not-exist: Permission denied

I think the general feeling last time this came up was "why not just
remove the cruft from your PATH?" But I would personally be OK with
option (3) above, and it is probably not that hard to implement.

-Peff

Re: Bug? Bad permissions in $PATH breaks Git aliases

[Johannes Sixt]

Am 3/27/2012 1:48, schrieb James Pickens:
> I'm not sure if this should be considered a bug or not, but I've noticed that
> when my $PATH contains an inaccessible directory, Git fails to execute aliases.
> ...
> I lean towards calling it a bug, since my shell doesn't seem to care if there
> are inaccessible directories in my $PATH.  It just ignores them, and I think Git
> ought to do the same.

Git is not a shell. And I'm sure it is not the only program that has this
issue. "Don't do it, then."

Git's implementation depends on the system's execvp behavior. As it
stands, current implementations of execvp path lookup and of popular
shells' path lookup differ in this respect. Bad luck. Don't check the
sanity of your PATH by testing how your shell looks up executables.

-- Hannes

Re: Bug? Bad permissions in $PATH breaks Git aliases

[James Pickens]

On Mon, Mar 26, 2012 at 8:19 PM, Jeff King <peff@peff.net> wrote:
> This seems to come up about once a year. The short of it is that execve
> will return EACCESS whether the file exists is not actually executable
> by you, or if you have an inaccessible element in your PATH. execvp will
> continue the search if it sees EACCESS, but will return EACCESS if it
> finds nothing.  So git just sees the EACCESS and doesn't know if you
> have bogus entries in your PATH or if there is a permissions problem
> with your executable files.
>
> For something like a shell, it's not that big a deal; either way, you
> couldn't execute the command in question. For git, it matters more
> because we first try to exec an external command, and then fall back to
> an alias (because externals take precedence over aliases).
>
> So basically our options are:
>
>  1. Start treating EACCESS silently as ENOENT. The downside is that we
>     fail to report the proper error when the file really does have
>     permissions problems (we would say "command not found", but that is
>     misleading).
>
>  2. Implement our own execvp, differentiating between "path not
>     available for looking in" and "we found the command, but there was
>     a permissions problem". I think somebody was working on this a few
>     months ago (search for "add exeecvp failure diagnostics") but it
>     seems to have fizzled.
>
>  3. If we get an EACCESS, remember it, try to do the alias lookup, and
>     then if that fails, report the "Permission denied" error (not
>     "command not found"). That is following the spirit of what execvp
>     does (it will find later entries in the PATH if they are there, but
>     otherwise will remember the EACCESS error).

Thanks for the detailed explanation!

> I think the general feeling last time this came up was "why not just
> remove the cruft from your PATH?"

Because I didn't know there was cruft in my path.  The cruft was put
there by a sloppily written project setup script that is not under my
control, and it may be difficult to get the owner of that script to
fix it.  Even after the script is fixed, we're certain to run into
other sloppy scripts in the future.

It took me quite a while to figure out the real problem the first
couple of times this happened, and I'm sure many of my colleagues who
use the same project setup script would not have figured it out at
all, so if there's anything Git can do to make it easier for them, I
think it's worth doing.  BTW repairing the crufty PATH is not easy,
because the project setup script will have added many unfamiliar
things to PATH, so you have to check them one by one to figure out
which ones are bad.

> But I would personally be OK with
> option (3) above, and it is probably not that hard to implement.

I would be happy with option (3).  This is the part where I sheepishly
confess that I probably can't find time to work on this myself, but if
option (3) is also acceptable to Junio, I may be able to find a
coworker to do it.  So Junio, do you have any objection to (3)?

James

Re: Bug? Bad permissions in $PATH breaks Git aliases

[James Pickens]

On Mar 26, 2012 at 11:14 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Git is not a shell. And I'm sure it is not the only program that has this
> issue. "Don't do it, then."

I haven't found any other program that has this issue yet.  It seems
like a pretty unique situation, since it's basically a side effect of
Git having aliases with lower precedence than executables.  Most
programs don't have that combination - they either don't have aliases
at all, or their aliases have higher precedence than executables.

> Don't check the
> sanity of your PATH by testing how your shell looks up executables.

I'm not claiming that it's sane to have a broken PATH, but as I
mentioned in an earlier email, sometimes my PATH gets broken through
no fault of my own, and it would be nice if Git could be more helpful
in that case.

James

Re: Bug? Bad permissions in $PATH breaks Git aliases

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This seems to come up about once a year....
> ...
> So basically our options are:
>
>   1. Start treating EACCESS silently as ENOENT. The downside is that we
>      fail to report the proper error when the file really does have
>      permissions problems (we would say "command not found", but that is
>      misleading).
>
>   2. Implement our own execvp, differentiating between "path not
>      available for looking in" and "we found the command, but there was
>      a permissions problem". I think somebody was working on this a few
>      months ago (search for "add exeecvp failure diagnostics") but it
>      seems to have fizzled.
>
>   3. If we get an EACCESS, remember it, try to do the alias lookup, and
>      then if that fails, report the "Permission denied" error (not
>      "command not found"). That is following the spirit of what execvp
>      does (it will find later entries in the PATH if they are there, but
>      otherwise will remember the EACCESS error).
>
> From what I can tell, dash uses stock execvp, and ends up closest to
> (3). Bash seems to have implemented their own path lookup, as it will
> distinguish between the two cases as in (2):
> ...
> I think the general feeling last time this came up was "why not just
> remove the cruft from your PATH?" But I would personally be OK with
> option (3) above, and it is probably not that hard to implement.

http://thread.gmane.org/gmane.comp.version-control.git/171755/focus=171838
shows that it was almost exactly a year ago; we tried (2) and nobody liked
it.

I got an impression from the discussion in it that #3 may give confusing
messages to the end users, but I didn't think the issues through.

Re: Bug? Bad permissions in $PATH breaks Git aliases

[Junio C Hamano]

James Pickens <jepicken@gmail.com> writes:

> On Mar 26, 2012 at 11:14 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> Git is not a shell. And I'm sure it is not the only program that has this
>> ... 
>> Don't check the
>> sanity of your PATH by testing how your shell looks up executables.
>
> I'm not claiming that it's sane to have a broken PATH, but as I
> mentioned in an earlier email, sometimes my PATH gets broken through
> no fault of my own, and it would be nice if Git could be more helpful
> in that case.

Hrm, so which was more helpful in diagnosing the broken PATH?  Git by
letting you be aware that there is some problem, or your shell by keeping
me oblivious of the issue?

Re: Bug? Bad permissions in $PATH breaks Git aliases

[James Pickens]

On Tue, Mar 27, 2012 at 8:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
> James Pickens <jepicken@gmail.com> writes:
>> I'm not claiming that it's sane to have a broken PATH, but as I
>> mentioned in an earlier email, sometimes my PATH gets broken through
>> no fault of my own, and it would be nice if Git could be more helpful
>> in that case.
>
> Hrm, so which was more helpful in diagnosing the broken PATH?  Git by
> letting you be aware that there is some problem, or your shell by keeping
> me oblivious of the issue?

In this case the broken parts of my PATH were completely uninteresting
to me - they didn't contain any executables that I would ever use.  So
if it didn't break my Git aliases, I could have continued working with
the broken PATH and never known or cared that it was broken.

But I get your point - sometimes it's more helpful to let the user
know something is amiss than try to guess what was intended.  I just
don't think this is one of those cases, mainly because Git's behavior
is inconsistent with other programs.  Git's behavior is not even
consistent with itself - IMO, a PATH containing a directory that
doesn't exist is just as broken as a PATH containing an inaccessible
directory, but Git only has a problem with the latter.  That doesn't
make sense to me.

James

Re: Bug? Bad permissions in $PATH breaks Git aliases

[Jeff King]

On Tue, Mar 27, 2012 at 08:11:27AM -0700, Junio C Hamano wrote:

> > I think the general feeling last time this came up was "why not just
> > remove the cruft from your PATH?" But I would personally be OK with
> > option (3) above, and it is probably not that hard to implement.
> 
> http://thread.gmane.org/gmane.comp.version-control.git/171755/focus=171838
> shows that it was almost exactly a year ago; we tried (2) and nobody liked
> it.

I was actually thinking of:

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

> I got an impression from the discussion in it that #3 may give confusing
> messages to the end users, but I didn't think the issues through.

The implementation for #3 is straight-forward; I'll post the patches in
a moment. However, it still ends up being confusing, because git ends up
talking about permissions instead of offering its usual help.

Here are a few cases with stock git and no broken entries in PATH:

  (1)
  $ git does-not-exist
  git: 'does-not-exist' is not a git command. See 'git --help'.

  (2)
  $ git cerry-pick
  git: 'cerry-pick' is not a git command. See 'git --help'.

  Did you mean this?
          cherry-pick

  (3)
  $ git config alias.broken does-not-exist
  $ git broken
  Expansion of alias 'broken' failed; 'does-not-exist' is not a git command

  (4)
  $ git config alias.ok '!echo ok'
  $ git ok
  ok

Here are the same cases with a broken entry in PATH:

  $ mkdir foo; chmod 0 foo; PATH=$PWD/foo:$PATH

  (1)
  $ git does-not-exist
  fatal: cannot exec 'git-does-not-exist': Permission denied

  (2)
  $ git cerry-pick
  fatal: cannot exec 'git-cerry-pick': Permission denied

  (3)
  $ git broken
  fatal: cannot exec 'git-broken': Permission denied

  (4)
  $ git ok
  fatal: cannot exec 'git-ok': Permission denied

Case (1) is OK; we report the differing error. But case (2) is worse, as
we don't offer suggestions any more. Cases (3) and (4) are both worse,
because we don't even try to expand the alias (whether it would work or
not).

Here are the same cases with my patches:

  (1)
  $ git does-not-exist
  Failed to run command 'does-not-exist': Permission denied

  (2)
  $ git cerry-pick
  Failed to run command 'cerry-pick': Permission denied

  (3)
  $ git broken
  Expansion of alias 'broken' failed; 'does-not-exist': Permission
  denied

  (4)
  $ git ok
  ok

This is somewhat improved. Case (4) now runs the alias. Case (3) has a
better error message, which is that it tells you it was not "broken"
which was a problem, but its subcommand. But the "permission denied"
error still ends up being somewhat confusing. And in case (2), you don't
get a list of suggestions (nor should you, because we still don't know
whether "cerry-pick" exists and cannot be executed, or if there is a
broken directory in the PATH).

So we've made the situation better, but it's still way less nice than
having a fixed PATH. Which makes me wonder if this half-way effort is
worth it.

-Peff

Re: Bug? Bad permissions in $PATH breaks Git aliases

[Junio C Hamano]

James Pickens <jepicken@gmail.com> writes:

> On Tue, Mar 27, 2012 at 8:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> James Pickens <jepicken@gmail.com> writes:
>>> I'm not claiming that it's sane to have a broken PATH, but as I
>>> mentioned in an earlier email, sometimes my PATH gets broken through
>>> no fault of my own, and it would be nice if Git could be more helpful
>>> in that case.
>>
>> Hrm, so which was more helpful in diagnosing the broken PATH? Git by
>> letting you be aware that there is some problem, or your shell by keeping
>> me oblivious of the issue?
>
> In this case the broken parts of my PATH were completely uninteresting
> to me - they didn't contain any executables that I would ever use.  So
> if it didn't break my Git aliases, I could have continued working with
> the broken PATH and never known or cared that it was broken.
>
> But I get your point - sometimes it's more helpful to let the user
> know something is amiss than try to guess what was intended.

That was not the "point" of my question.  In fact, there was no point.  I
may be a mean person and may often throw rhetorical questions to embarrass
others, but I am not *that* mean to always ask only rhetorical questions ;-).

Judging from your answer, it would have been better for you if Git didn't
even tell you that there was an error due to an unreadable directory.  And
if that is the case, "Git could be more helpful in that case" will lead us
in one direction (i.e. "we simply ignore EACCESS and treat it as ENOENT"),
which is a quite different direction from what others discussed and
suggested in the thread (i.e. "we give more detailed diagnosis, perhaps
saying "your PATH has /usr/local/bin but it cannot be read, so we cannot
tell git-frotz exists there or not").

I just wanted to see what was the desired behaviour you have in mind.

[PATCH 1/2] run-command: propagate EACCES errors to parent

[Jeff King]

The caller of run_command does not directly get access to
the errno from exec, because it happens in the forked child.
However, knowing the specific reason for an exec failure can
help the parent respond better or produce better error
messages.

We already propagate ENOENT to the parent via exit code 127.
Let's do the same for EACCES with exit code 126, which is
already used by bash to indicate the same thing.

Signed-off-by: Jeff King <peff@peff.net>
---
Actually, there is a slight bending of the truth in the commit message.
bash implements its own execvp, and it will only return 126/EACCES if a
file is found via stat(), but is not executable. If there is an
inaccessible directory in the PATH (meaning that stat() will fail), it
will silently convert that to 127/ENOENT.

 run-command.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/run-command.c b/run-command.c
index 1db8abf..e303beb 100644
--- a/run-command.c
+++ b/run-command.c
@@ -185,6 +185,10 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
 			code = -1;
 			failed_errno = ENOENT;
 		}
+		else if (code == 126) {
+			code = -1;
+			failed_errno = EACCES;
+		}
 	} else {
 		error("waitpid is confused (%s)", argv0);
 	}
@@ -346,6 +350,11 @@ fail_pipe:
 				error("cannot run %s: %s", cmd->argv[0],
 					strerror(ENOENT));
 			exit(127);
+		} else if (errno == EACCES) {
+			if (!cmd->silent_exec_failure)
+				error("cannot run %s: %s", cmd->argv[0],
+					strerror(errno));
+			exit(126);
 		} else {
 			die_errno("cannot exec '%s'", cmd->argv[0]);
 		}
-- 
1.7.9.5.5.g9b709b

[PATCH 2/2] git: continue alias lookup on EACCES errors

[Jeff King]

If git receives an EACCES error while trying to execute an
external command, we currently give up and report the error.
However, the EACCES may be caused by an inaccessible
directory in the user's PATH.

In this case, execvp will skip over the inaccessible
directory and keep searching the PATH. If it finds
something, then that gets executed. Otherwise, the earlier
EACCES is remembered and returned.

However, git does not implement the same rule when looking
up aliases. It will return immediately upon seeing EACCES
from execvp, without trying aliases.  This renders aliases
unusable if there is an inaccessible directory in the PATH.

This patch implements a logical extension of execvp's lookup
rules to aliases. We will try to find aliases even after
execvp returns EACCES. If there is an alias, then we expand
it as usual.  If ther eisn't, then we will remember and
report the EACCES error.

Signed-off-by: Jeff King <peff@peff.net>
---
 git.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/git.c b/git.c
index 3805616..917bc9e 100644
--- a/git.c
+++ b/git.c
@@ -496,7 +496,7 @@ static void execv_dashed_external(const char **argv)
 	 * OK to return. Otherwise, we just pass along the status code.
 	 */
 	status = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE | RUN_CLEAN_ON_EXIT);
-	if (status >= 0 || errno != ENOENT)
+	if (status >= 0 || (errno != ENOENT && errno != EACCES))
 		exit(status);
 
 	argv[0] = tmp;
@@ -586,14 +586,16 @@ int main(int argc, const char **argv)
 		static int done_help = 0;
 		static int was_alias = 0;
 		was_alias = run_argv(&argc, &argv);
-		if (errno != ENOENT)
-			break;
-		if (was_alias) {
+		if (was_alias && (errno == ENOENT || errno == EACCES)) {
 			fprintf(stderr, "Expansion of alias '%s' failed; "
-				"'%s' is not a git command\n",
-				cmd, argv[0]);
+				"'%s'%s\n", cmd, argv[0],
+				errno == ENOENT ?
+				  " is not a git command" :
+				  ": Permission denied");
 			exit(1);
 		}
+		if (errno != ENOENT)
+			break;
 		if (!done_help) {
 			cmd = argv[0] = help_unknown_cmd(cmd);
 			done_help = 1;
-- 
1.7.9.5.5.g9b709b

Re: [PATCH 1/2] run-command: propagate EACCES errors to parent

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The caller of run_command does not directly get access to
> the errno from exec, because it happens in the forked child.
> However, knowing the specific reason for an exec failure can
> help the parent respond better or produce better error
> messages.
>
> We already propagate ENOENT to the parent via exit code 127.
> Let's do the same for EACCES with exit code 126, which is
> already used by bash to indicate the same thing.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Actually, there is a slight bending of the truth in the commit message.
> bash implements its own execvp, and it will only return 126/EACCES if a
> file is found via stat(), but is not executable. If there is an
> inaccessible directory in the PATH (meaning that stat() will fail), it
> will silently convert that to 127/ENOENT.

I am wondering what would happen if we treated EACCESS and ENOENT exactly
the same way.  Wouldn't the four breakage scenarios in the cover letter
end up being even better?  Case (3) will still say does-not-exist is not a
git command (instead of "permission denied", which this patch gives), but
your case (2) will see a much better diagnosis.

Take the above with a grain of salt, though, as this is written soon after
I wrote my response to James (the one with "I may be a mean person").

>  run-command.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/run-command.c b/run-command.c
> index 1db8abf..e303beb 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -185,6 +185,10 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
>  			code = -1;
>  			failed_errno = ENOENT;
>  		}
> +		else if (code == 126) {
> +			code = -1;
> +			failed_errno = EACCES;
> +		}
>  	} else {
>  		error("waitpid is confused (%s)", argv0);
>  	}
> @@ -346,6 +350,11 @@ fail_pipe:
>  				error("cannot run %s: %s", cmd->argv[0],
>  					strerror(ENOENT));
>  			exit(127);
> +		} else if (errno == EACCES) {
> +			if (!cmd->silent_exec_failure)
> +				error("cannot run %s: %s", cmd->argv[0],
> +					strerror(errno));
> +			exit(126);
>  		} else {
>  			die_errno("cannot exec '%s'", cmd->argv[0]);
>  		}

Re: [PATCH 1/2] run-command: propagate EACCES errors to parent

[Jeff King]

On Tue, Mar 27, 2012 at 11:24:02AM -0700, Junio C Hamano wrote:

> > Actually, there is a slight bending of the truth in the commit message.
> > bash implements its own execvp, and it will only return 126/EACCES if a
> > file is found via stat(), but is not executable. If there is an
> > inaccessible directory in the PATH (meaning that stat() will fail), it
> > will silently convert that to 127/ENOENT.
> 
> I am wondering what would happen if we treated EACCESS and ENOENT exactly
> the same way.  Wouldn't the four breakage scenarios in the cover letter
> end up being even better?  Case (3) will still say does-not-exist is not a
> git command (instead of "permission denied", which this patch gives), but
> your case (2) will see a much better diagnosis.

Yes, after writing my last message detailing all of the cases, I am
tempted to go that way. The downside is that it is more confusing if you
have a file in your PATH without the execute bit. IOW, we do not
differentiate the common mistake of "directory in PATH is not
accessible" from the uncommon "we found /usr/bin/foo, but it is not
executable by you". While the latter case is much less common, it would
be nice to continue to report EACCES.

Which leads us to either implementing our own execvp, or tracing through
the PATH after execvp fails (which amounts to basically the same thing).

-Peff

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If git receives an EACCES error while trying to execute an
> external command, we currently give up and report the error.
> However, the EACCES may be caused by an inaccessible
> directory in the user's PATH.

Regardless of EACCES/ENOENT change we discussed, the observable behaviour
should be testable.  Something like this?

 t/t0061-run-command.sh |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 8d4938f..dbb1d9e 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_failure POSIXPERM 'run_command reports EACCES' '
 	cat hello-script >hello.sh &&
 	chmod -x hello.sh &&
 	test_must_fail test-run-command run-command ./hello.sh 2>err &&
@@ -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

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jeff King]

On Tue, Mar 27, 2012 at 12:16:36PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > If git receives an EACCES error while trying to execute an
> > external command, we currently give up and report the error.
> > However, the EACCES may be caused by an inaccessible
> > directory in the user's PATH.
> 
> Regardless of EACCES/ENOENT change we discussed, the observable behaviour
> should be testable.  Something like this?

Yes, though I held back on writing tests, because I don't think we've
quite decided what the behavior _should_ be. Should we be
differentiating "chmod -x /bin/ls" from "chmod -x /bin"? Should we be
continuing alias lookup on EACCES? Should we print edit-distance
suggestions on EACCES?

I think the four cases from my previous email would be reasonable things
to test, but I wasn't sure what the expected outcomes should look like.

-Peff

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Mar 27, 2012 at 12:16:36PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > If git receives an EACCES error while trying to execute an
>> > external command, we currently give up and report the error.
>> > However, the EACCES may be caused by an inaccessible
>> > directory in the user's PATH.
>> 
>> Regardless of EACCES/ENOENT change we discussed, the observable behaviour
>> should be testable.  Something like this?
>
> Yes, though I held back on writing tests, because I don't think we've
> quite decided what the behavior _should_ be. Should we be
> differentiating "chmod -x /bin/ls" from "chmod -x /bin"? Should we be
> continuing alias lookup on EACCES? Should we print edit-distance
> suggestions on EACCES?

I am leaning to think that it would be the least surprising if we treat as
if /bin/ls does not even exist if /bin is not searchable.  If /bin/ls is
unreadable or unexecutable but /bin is searchable, then we _know_ it
exists, and we follow the usual exec*p() rule to ignore it so "git ls"
would try to find an alias and when all else fails will give the edit
distance suggestions but should exclude /bin/ls from candidates.  If /bin
itself is unsearchable, we do not even know what it contains, so it is
needless to say that /bin/ls will not be part of suggestion candidates.

That way, the only thing people _could_ complain about is "I have a
directory $HOME/sillybin in my $PATH but do not have an executable bit on
it.  When I try to run 'git stupid', 'git-stupid' in that diretory is not
executed, and I do not even get an error message to point out that I am
missing the executable bit on $HOME/sillybin directory".  And you can say
"Ah, just like the shell.  So make sure you have necessary permission bits
on things".  Very easy and straightforward to explain and understand.

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jeff King]

On Wed, Mar 28, 2012 at 10:42:26AM -0700, Junio C Hamano wrote:

> > Yes, though I held back on writing tests, because I don't think we've
> > quite decided what the behavior _should_ be. Should we be
> > differentiating "chmod -x /bin/ls" from "chmod -x /bin"? Should we be
> > continuing alias lookup on EACCES? Should we print edit-distance
> > suggestions on EACCES?
> 
> I am leaning to think that it would be the least surprising if we treat as
> if /bin/ls does not even exist if /bin is not searchable.  If /bin/ls is
> unreadable or unexecutable but /bin is searchable, then we _know_ it
> exists, and we follow the usual exec*p() rule to ignore it so "git ls"
> would try to find an alias and when all else fails will give the edit
> distance suggestions but should exclude /bin/ls from candidates.  If /bin
> itself is unsearchable, we do not even know what it contains, so it is
> needless to say that /bin/ls will not be part of suggestion candidates.

That sounds sensible to me. I think it involves writing our own
execvp, though, right? If we use stock execvp, we can't tell the
difference between the two cases. OTOH, I think we already have an
implementation in compat/mingw.

> That way, the only thing people _could_ complain about is "I have a
> directory $HOME/sillybin in my $PATH but do not have an executable bit on
> it.  When I try to run 'git stupid', 'git-stupid' in that diretory is not
> executed, and I do not even get an error message to point out that I am
> missing the executable bit on $HOME/sillybin directory".  And you can say
> "Ah, just like the shell.  So make sure you have necessary permission bits
> on things".  Very easy and straightforward to explain and understand.

Agreed.

-Peff

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jonathan Nieder]

Jeff King wrote:
> On Wed, Mar 28, 2012 at 10:42:26AM -0700, Junio C Hamano wrote:

>> I am leaning to think that it would be the least surprising if we treat as
>> if /bin/ls does not even exist if /bin is not searchable.  If /bin/ls is
>> unreadable or unexecutable but /bin is searchable, then we _know_ it
>> exists, and we follow the usual exec*p() rule to ignore it
[...]
> That sounds sensible to me. I think it involves writing our own
> execvp, though, right?

If I understood Junio correctly, then checking for ENOENT and EACCES
should be enough.

Example: when I try

 :; mkdir $HOME/cannotread
 :; chmod -x $HOME/cannotread
 :; echo nonsense >$HOME/bin/cat
 :; chmod -x $HOME/bin/cat
 :; PATH=$HOME/cannotread:$HOME/bin/cat:/usr/local/bin:/usr/bin:/bin
 :; cat /etc/fstab

the shell uses /bin/cat without complaint.

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Mar 28, 2012 at 10:42:26AM -0700, Junio C Hamano wrote:
>
>> > Yes, though I held back on writing tests, because I don't think we've
>> > quite decided what the behavior _should_ be. Should we be
>> > differentiating "chmod -x /bin/ls" from "chmod -x /bin"? Should we be
>> > continuing alias lookup on EACCES? Should we print edit-distance
>> > suggestions on EACCES?
>> 
>> I am leaning to think that it would be the least surprising if we treat as
>> if /bin/ls does not even exist if /bin is not searchable.  If /bin/ls is
>> unreadable or unexecutable but /bin is searchable, then we _know_ it
>> exists, and we follow the usual exec*p() rule to ignore it so "git ls"
>> would try to find an alias and when all else fails will give the edit
>> distance suggestions but should exclude /bin/ls from candidates.  If /bin
>> itself is unsearchable, we do not even know what it contains, so it is
>> needless to say that /bin/ls will not be part of suggestion candidates.
>
> That sounds sensible to me. I think it involves writing our own
> execvp, though, right? If we use stock execvp, we can't tell the
> difference between the two cases.

The stock exec*p() will not hit "/bin/ls" in either case, so we will give
"'ls' is not a git command", without having to differenciate it.  That is
what I meant by "we follow the usual rule to ignore it".

We already have the code necessary to enumerate the possible commands from
components of the PATH in order to give suggestion, so we can run it
after seeing exec*p() failure to see if we did not see any "ls", or we saw
"ls" but it was not executable.  No need to penalize the normal case, no?

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jeff King wrote:
>> On Wed, Mar 28, 2012 at 10:42:26AM -0700, Junio C Hamano wrote:
>
>>> I am leaning to think that it would be the least surprising if we treat as
>>> if /bin/ls does not even exist if /bin is not searchable.  If /bin/ls is
>>> unreadable or unexecutable but /bin is searchable, then we _know_ it
>>> exists, and we follow the usual exec*p() rule to ignore it
> [...]
>> That sounds sensible to me. I think it involves writing our own
>> execvp, though, right?
>
> If I understood Junio correctly, then checking for ENOENT and EACCES
> should be enough.
>
> Example: when I try
>
>  :; mkdir $HOME/cannotread
>  :; chmod -x $HOME/cannotread
>  :; echo nonsense >$HOME/bin/cat
>  :; chmod -x $HOME/bin/cat
>  :; PATH=$HOME/cannotread:$HOME/bin/cat:/usr/local/bin:/usr/bin:/bin
>  :; cat /etc/fstab
>
> the shell uses /bin/cat without complaint.

Yeah, but I think that the case Peff is worried about is:

        $ >~/bin/nosuch
        $ nosuch
        nosuch: Permission denied

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jonathan Nieder]

On Wed, Mar 28, 2012 at 11:31:10AM -0700, Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> Example: when I try
>>
>>  :; mkdir $HOME/cannotread
>>  :; chmod -x $HOME/cannotread
>>  :; echo nonsense >$HOME/bin/cat
>>  :; chmod -x $HOME/bin/cat
>>  :; PATH=$HOME/cannotread:$HOME/bin:/usr/local/bin:/usr/bin:/bin
>>  :; cat /etc/fstab
>>
>> the shell uses /bin/cat without complaint.
>
> Yeah, but I think that the case Peff is worried about is:
>
>         $ >~/bin/nosuch
>         $ nosuch
>         nosuch: Permission denied

Just remembering the EACCES and reporting it when no alias exists
would take care of that, no?  In other words, this seems analogous
to the example of a non-executable "cat" that is reported if no
other cat exists but does not prevent /bin/cat from being run.

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jeff King]

On Wed, Mar 28, 2012 at 11:31:10AM -0700, Junio C Hamano wrote:

> > If I understood Junio correctly, then checking for ENOENT and EACCES
> > should be enough.
> >
> > Example: when I try
> >
> >  :; mkdir $HOME/cannotread
> >  :; chmod -x $HOME/cannotread
> >  :; echo nonsense >$HOME/bin/cat
> >  :; chmod -x $HOME/bin/cat
> >  :; PATH=$HOME/cannotread:$HOME/bin/cat:/usr/local/bin:/usr/bin:/bin
> >  :; cat /etc/fstab
> >
> > the shell uses /bin/cat without complaint.
> 
> Yeah, but I think that the case Peff is worried about is:
> 
>         $ >~/bin/nosuch
>         $ nosuch
>         nosuch: Permission denied

Right. My reading of your suggestion was that we would differentiate
those two cases, which one cannot do simply from the return value and
errno after execvp. The former case (inaccessible directory) is common
and probably harmless. The latter (non-executable file) is rare and
probably an actual error we should point out.

I'd also be OK with saying that the latter is too rare to worry about,
and simply accept it as collateral damage (or we could even flag it with
test_expect_failure and leave it for somebody else to work on later if
they care).

-Peff

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jeff King]

On Wed, Mar 28, 2012 at 01:40:14PM -0500, Jonathan Nieder wrote:

> On Wed, Mar 28, 2012 at 11:31:10AM -0700, Junio C Hamano wrote:
> > Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> >> Example: when I try
> >>
> >>  :; mkdir $HOME/cannotread
> >>  :; chmod -x $HOME/cannotread
> >>  :; echo nonsense >$HOME/bin/cat
> >>  :; chmod -x $HOME/bin/cat
> >>  :; PATH=$HOME/cannotread:$HOME/bin:/usr/local/bin:/usr/bin:/bin
> >>  :; cat /etc/fstab
> >>
> >> the shell uses /bin/cat without complaint.
> >
> > Yeah, but I think that the case Peff is worried about is:
> >
> >         $ >~/bin/nosuch
> >         $ nosuch
> >         nosuch: Permission denied
> 
> Just remembering the EACCES and reporting it when no alias exists
> would take care of that, no?  In other words, this seems analogous
> to the example of a non-executable "cat" that is reported if no
> other cat exists but does not prevent /bin/cat from being run.

That's what the patch I posted earlier does. But it means we _also_
report "permission denied" for inaccessible directories, which is
needlessly confusing (and much more common, I would think).

-Peff

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jeff King]

On Wed, Mar 28, 2012 at 11:29:17AM -0700, Junio C Hamano wrote:

> > That sounds sensible to me. I think it involves writing our own
> > execvp, though, right? If we use stock execvp, we can't tell the
> > difference between the two cases.
> 
> The stock exec*p() will not hit "/bin/ls" in either case, so we will give
> "'ls' is not a git command", without having to differenciate it.  That is
> what I meant by "we follow the usual rule to ignore it".
> 
> We already have the code necessary to enumerate the possible commands from
> components of the PATH in order to give suggestion, so we can run it
> after seeing exec*p() failure to see if we did not see any "ls", or we saw
> "ls" but it was not executable.  No need to penalize the normal case, no?

Yes, we can differentiate after the fact. Though I think it ends up
being almost the same code as just implementing execvp in the first
place.

-Peff

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jonathan Nieder]

Jeff King wrote:

> That's what the patch I posted earlier does. But it means we _also_
> report "permission denied" for inaccessible directories, which is
> needlessly confusing (and much more common, I would think).

So the message could say

	$ nosuch
	nosuch: Permission denied
	hint: A permissions problem was encountered searching for or
	hint: executing that command on the $PATH.
	hint: Check your PATH setting and permissions.

or even

	$ nosuch
	nosuch: No such file or directory or permission denied

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jeff King]

On Wed, Mar 28, 2012 at 02:45:16PM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > That's what the patch I posted earlier does. But it means we _also_
> > report "permission denied" for inaccessible directories, which is
> > needlessly confusing (and much more common, I would think).
> 
> So the message could say
> 
> 	$ nosuch
> 	nosuch: Permission denied
> 	hint: A permissions problem was encountered searching for or
> 	hint: executing that command on the $PATH.
> 	hint: Check your PATH setting and permissions.
> 
> or even
> 
> 	$ nosuch
> 	nosuch: No such file or directory or permission denied

That is slightly better than the current behavior, but for people in
James's situation, it's still quite ugly. How about this patch, which
just treats the inaccessible directory case as ENOENT. This matches
bash's behavior. And we don't need any other patches. In James's
situation, the problem just goes away, and we still get an error on a
nonexecutable file.

It won't continue trying aliases in the latter case, but we could put my
other patches on top if we want to. It's less compelling to do so,
though, because having "git-foo" in your path and not executable
probably _is_ a configuration error that you should deal with.

-Peff

---
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..4bdbea8 100644
--- a/run-command.c
+++ b/run-command.c
@@ -76,6 +76,39 @@ static inline void dup_devnull(int to)
 }
 #endif
 
+static int file_in_path_is_nonexecutable(const char *file)
+{
+	const char *p = getenv("PATH");
+
+	if (!p)
+		return 0;
+
+	while (1) {
+		const char *end = strchrnul(p, ':');
+		const char *path;
+		struct stat st;
+
+		path = mkpath("%.*s/%s", (int)(end - p), p, file);
+		if (!stat(path, &st) && access(path, X_OK) < 0)
+			return 1;
+
+		if (!*end)
+			break;
+
+		p = end + 1;
+	}
+
+	return 0;
+}
+
+int sane_execvp(const char *file, char * const argv[])
+{
+	int ret = execvp(file, argv);
+	if (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))
+		errno = ENOENT;
+	return ret;
+}
+
 static const char **prepare_shell_cmd(const char **argv)
 {
 	int argc, nargc = 0;
@@ -114,7 +147,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 +372,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)

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jeff King]

On Wed, Mar 28, 2012 at 04:18:51PM -0400, Jeff King wrote:

> +int sane_execvp(const char *file, char * const argv[])
> +{
> +	int ret = execvp(file, argv);
> +	if (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))
> +		errno = ENOENT;
> +	return ret;
> +}

Hmm, this should check for (*file == '/') to handle absolute paths
properly. If you have an absolute path, I would tend to think that we
should never rewrite it into ENOENT (so if you have "/foo/bar", even if
"foo" is inaccessible, ENOENT is still the right response).

-Peff

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jonathan Nieder]

(cc-ing Frans who had a related itch if I remember correctly[1])
Hi again,

Jeff King wrote:

> --- a/run-command.c
> +++ b/run-command.c
> @@ -76,6 +76,39 @@ static inline void dup_devnull(int to)
>  }
>  #endif
>  
> +static int file_in_path_is_nonexecutable(const char *file)
> +{
> +	const char *p = getenv("PATH");
> +
> +	if (!p)
> +		return 0;
> +
> +	while (1) {
> +		const char *end = strchrnul(p, ':');
> +		const char *path;
> +		struct stat st;
> +
> +		path = mkpath("%.*s/%s", (int)(end - p), p, file);
> +		if (!stat(path, &st) && access(path, X_OK) < 0)
> +			return 1;
> +
> +		if (!*end)
> +			break;
> +
> +		p = end + 1;
> +	}
> +
> +	return 0;
> +}

Nice.

Nitpicks:

 - (end - p) is not guaranteed to fit inside an int.  What should happen
   when my PATH is very long?

 - the existence check would be simpler spelled as access(path, F_OK).

 - the above checks if there is _any_ nonexecutable instance of "file"
   in the directories listed in $PATH, but isn't what we want to check
   whether _all_ of them are nonexecutable?

> +
> +int sane_execvp(const char *file, char * const argv[])
> +{
> +	int ret = execvp(file, argv);
> +	if (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))
> +		errno = ENOENT;
> +	return ret;
> +}

Makes sense.  No objections from me.

	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.
	 */
	if (errno == EACCES && cannot_find_in_PATH(file))
		errno = ENOENT;
	return -1;

Thanks,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/189077/focus=189913

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +static int file_in_path_is_nonexecutable(const char *file)
> +{
> +	const char *p = getenv("PATH");
> +
> +	if (!p)
> +		return 0;
> +
> +	while (1) {
> +		const char *end = strchrnul(p, ':');
> +		const char *path;
> +		struct stat st;
> +
> +		path = mkpath("%.*s/%s", (int)(end - p), p, file);

Given PATH=":/usr/bin:/bin" and file "frotz" (please call it "cmd" or
something, by the way), end points at the first colon and path becomes
"/frotz".  Oops?

> +		if (!stat(path, &st) && access(path, X_OK) < 0)
> +			return 1;
> +
> +		if (!*end)
> +			break;
> +
> +		p = end + 1;
> +	}
> +
> +	return 0;
> +}
> +
> +int sane_execvp(const char *file, char * const argv[])
> +{
> +	int ret = execvp(file, argv);
> +	if (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))
> +		errno = ENOENT;
> +	return ret;

Double negation makes my head hurt, but unfortunately, we cannot rename it
to "executable_exists_on_path()" and negate its return value.

Anyway, the logic is to set errno to ENOENT when

 - We tried to exec, and got EACCES; and
 - There is a file on the PATH that lacks executable bit.

In such a case, the error from execvp() is not about the file it tried to
execute lacked executable bit, but there was nothing that match the name,
but it couldn't be certain because some directories were not readable.

OK.  I think I can follow that logic.

If there are more than one entry on PATH, and a system call made during
first round of the loop fails but a later round finds a non-executable
file, i.e.

	$ PATH=/nosuch:/home/peff/bin; export PATH
        $ >/home/peff/bin/frotz; chmod -x /home/peff/bin/frotz
        git frotz

we would get EACCES from execvp(), the first round runs stat("/nosuch/frotz")
and sets errno to ENOTDIR, and the second round runs stat() and access()
on "/home/peff/bin/frotz" and returns 1 to say "Yeah, there is a plain
file frotz that cannot be executed".

And sane_execvp() will return ENOTDIR?

So sane_execvp() would probably need to do a bit more (but not that much).

	if (ret < 0 && errno == EACCES)
		errno = file_in_path_is_nonexecutable(file) ? EACCES : ENOENT;
	return ret;

or something.

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jonathan Nieder]

Jeff King wrote:
> On Wed, Mar 28, 2012 at 04:18:51PM -0400, Jeff King wrote:

>> +int sane_execvp(const char *file, char * const argv[])
>> +{
>> +	int ret = execvp(file, argv);
>> +	if (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))
>> +		errno = ENOENT;
>> +	return ret;
>> +}
>
> Hmm, this should check for (*file == '/') to handle absolute paths
> properly.

Or rather for "strchr(file, '/')", because "path/to/cmd" does not mean to
append that string to each term of $PATH.

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jeff King]

On Wed, Mar 28, 2012 at 03:42:21PM -0500, Jonathan Nieder wrote:

> > +		path = mkpath("%.*s/%s", (int)(end - p), p, file);
> [...]
>  - (end - p) is not guaranteed to fit inside an int.  What should happen
>    when my PATH is very long?

That is the cost of using the mkpath convenience function (otherwise,
the compiler will complain that ".*" expects an int). We can do it
manually, but in practice, do you really expect your PATH environment
variable to overflow an int?

>  - the existence check would be simpler spelled as access(path, F_OK).

Yeah, I think that is nicer. I went with !stat() because that is our
usual file_exists test, and I was wondering if there were any
portability issues with access(..., F_OK). However, we seem to use it
already in other places, so it should be fine.

>  - the above checks if there is _any_ nonexecutable instance of "file"
>    in the directories listed in $PATH, but isn't what we want to check
>    whether _all_ of them are nonexecutable?

If there is one that is executable, then execvp would not have returned.
So if there is any entry that is non-executable, then they all are. And
we don't care about the actual number; we only care whether there is one
(in which case it is no ENOENT).

> > +int sane_execvp(const char *file, char * const argv[])
> > +{
> > +	int ret = execvp(file, argv);
> > +	if (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))
> > +		errno = ENOENT;
> > +	return ret;
> > +}
> 
> Makes sense.  No objections from me.
> 
> 	if (!execvp(file, argv))
> 		return 0;
> [...]
> 	return -1;

That is nicer; I have a general avoidance of rewriting return codes, but I
think it is safe to translate a non-zero execvp result into -1.

> 	/*
> 	 * 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.
> 	 */
> 	if (errno == EACCES && cannot_find_in_PATH(file))
> 		errno = ENOENT;

I think we can even simplify cannot_find to "!exists_in_PATH" to make
it even simpler. If it exists and execvp did not execute it, then it
must be non-executable (or there is a race condition :) ).

-Peff

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jeff King]

On Wed, Mar 28, 2012 at 03:51:33PM -0500, Jonathan Nieder wrote:

> > Hmm, this should check for (*file == '/') to handle absolute paths
> > properly.
> 
> Or rather for "strchr(file, '/')", because "path/to/cmd" does not mean to
> append that string to each term of $PATH.

Yes, thanks for a sanity check.

-Peff

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jonathan Nieder]

Jeff King wrote:

> That is the cost of using the mkpath convenience function (otherwise,
> the compiler will complain that ".*" expects an int). We can do it
> manually, but in practice, do you really expect your PATH environment
> variable to overflow an int?

I'd think a check like

	if (end - p > INT_MAX)
		die("holy cow your PATH is big");

would be good enough.  Or even

	assert(end - p <= INT_MAX);

if there is some environment limit I forgot about that makes that
always true.

>> 	/*
>> 	 * 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.
>> 	 */
>> 	if (errno == EACCES && cannot_find_in_PATH(file))
>> 		errno = ENOENT;
>
> I think we can even simplify cannot_find to "!exists_in_PATH" to make
> it even simpler. If it exists and execvp did not execute it, then it
> must be non-executable (or there is a race condition :) ).

Yeah, sounds good.  With Junio's caveat, that makes:

	if (errno == EACCES && !strchr(file, '/'))
		errno = exists_in_PATH(file) ? EACCES : ENOENT;

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jeff King]

On Wed, Mar 28, 2012 at 01:43:39PM -0700, Junio C Hamano wrote:

> > +	while (1) {
> > +		const char *end = strchrnul(p, ':');
> > +		const char *path;
> > +		struct stat st;
> > +
> > +		path = mkpath("%.*s/%s", (int)(end - p), p, file);
> 
> Given PATH=":/usr/bin:/bin" and file "frotz" (please call it "cmd" or
> something, by the way), end points at the first colon and path becomes
> "/frotz".  Oops?

Ugh, yeah. This is what I meant when I said "checking afterwards
basically means re-implementing execvp". :)

Regarding the name, I pulled it from the linux-manpages execvp(3), since
this is supposed to be compatible. POSIX uses the even worse "path" as
the first element. But there is no reason we have to follow those naming
conventions.

> > +int sane_execvp(const char *file, char * const argv[])
> > +{
> > +	int ret = execvp(file, argv);
> > +	if (ret < 0 && errno == EACCES && !file_in_path_is_nonexecutable(file))
> > +		errno = ENOENT;
> > +	return ret;
> 
> Double negation makes my head hurt, but unfortunately, we cannot rename it
> to "executable_exists_on_path()" and negate its return value.

Right, technically it could exist in two places, and we care about
finding the first one.

In practice, I think we can just do exists_on_path() and not even worry
about the executable bit. If it exists, execvp did not run it, and we
got EACCES, then it is not executable. Any other error would trump
EACCES (i.e., execvp would have returned immediately; with EACCES it
waits until it has processed all entries before returning EACCES).

I actually think this would be easier to read if we simply
re-implemented execvp.

> Anyway, the logic is to set errno to ENOENT when
> 
>  - We tried to exec, and got EACCES; and
>  - There is a file on the PATH that lacks executable bit.
> 
> In such a case, the error from execvp() is not about the file it tried to
> execute lacked executable bit, but there was nothing that match the name,
> but it couldn't be certain because some directories were not readable.
> 
> OK.  I think I can follow that logic.

I think you are backwards. There is _no_ file on the PATH that lacks the
executable bit, and therefore the error is about an inaccessible
directory.

You could also search for an inaccessible directory, but that is not
quite right. If you have an inaccessible directory _and_ a matching file
with no executable bit, then you would make the wrong assumption.

> If there are more than one entry on PATH, and a system call made during
> first round of the loop fails but a later round finds a non-executable
> file, i.e.
> 
> 	$ PATH=/nosuch:/home/peff/bin; export PATH
>         $ >/home/peff/bin/frotz; chmod -x /home/peff/bin/frotz
>         git frotz
> 
> we would get EACCES from execvp(), the first round runs stat("/nosuch/frotz")
> and sets errno to ENOTDIR, and the second round runs stat() and access()
> on "/home/peff/bin/frotz" and returns 1 to say "Yeah, there is a plain
> file frotz that cannot be executed".
> 
> And sane_execvp() will return ENOTDIR?
>
> So sane_execvp() would probably need to do a bit more (but not that much).
> 
> 	if (ret < 0 && errno == EACCES)
> 		errno = file_in_path_is_nonexecutable(file) ? EACCES : ENOENT;
> 	return ret;
> 
> or something.

Good point. We definitely need to save the EACCES errno across the
second round lookup.

-Peff

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jeff King]

On Wed, Mar 28, 2012 at 04:01:45PM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > That is the cost of using the mkpath convenience function (otherwise,
> > the compiler will complain that ".*" expects an int). We can do it
> > manually, but in practice, do you really expect your PATH environment
> > variable to overflow an int?
> 
> I'd think a check like
> 
> 	if (end - p > INT_MAX)
> 		die("holy cow your PATH is big");
> 
> would be good enough.  Or even
> 
> 	assert(end - p <= INT_MAX);
> 
> if there is some environment limit I forgot about that makes that
> always true.

You can generally only pass a limited amount through execve. In theory
we could putenv() an arbitrarily large string, but I'm not sure we need
to worry about that. The execve limitation ranges from a few pages to a
few dozen pages by default. On recent versions of linux, it is based on
the stack rlimit. But my reading of execve(2) says that individual items
are still capped at 32 pages.

However, you have a much bigger problem with giant PATH elements, which
is that the whole thing is generally going to get stuck in a PATH_MAX
buffer and truncated. I would expect ENAMETOOLONG or EINVAL from execvp
in that case. That's what dietlibc will do. But glibc being glibc, it's
dynamically allocated there.

-Peff

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Frans Klaver]

On Wed, 28 Mar 2012 22:42:21 +0200, Jonathan Nieder <jrnieder@gmail.com>  
wrote:

> (cc-ing Frans who had a related itch if I remember correctly[1])

Thanks.

> [1]  
> http://thread.gmane.org/gmane.comp.version-control.git/189077/focus=189913

Reminds me that I need to get me some time to work on that again.

Frans

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Regarding the name, I pulled it from the linux-manpages execvp(3), since
> this is supposed to be compatible. POSIX uses the even worse "path" as
> the first element. But there is no reason we have to follow those naming
> conventions.

Let me take that back.  We are checking if there is a non-command in a
directory that is somewhere on the PATH, so calling it file like you did
is a lot saner than calling it cmd.

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jeff King]

Here's a rework of the patch based on the comments so far.

It handles empty path elements properly, and it handles the munging of
errno properly.  It uses a strbuf to avoid any path limitations (in
practice, I don't expect this to be much of an issue, but it matches
what glibc does. And this is the slow error-path anyway, so it's not a
big deal). And it has miscellaneous style fixes and comments.

No tests yet. I'll post some output on that in a minute.

---
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..2b0c311 100644
--- a/run-command.c
+++ b/run-command.c
@@ -76,6 +76,63 @@ static inline void dup_devnull(int to)
 }
 #endif
 
+static int exists_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)) {
+			strbuf_release(&buf);
+			return 1;
+		}
+
+		if (!*end)
+			break;
+		p = end + 1;
+	}
+
+	strbuf_release(&buf);
+	return 0;
+}
+
+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 +171,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 +396,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)

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jeff King]

On Wed, Mar 28, 2012 at 05:57:04PM -0400, Jeff King wrote:

> +static int exists_in_PATH(const char *file)
> +{
> +	const char *p = getenv("PATH");
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	if (!p || !*p)
> +		return 0;

One thing to note: real execvp, when it sees a NULL $PATH, will fill in
some OS-dependent default path. My linux box has _PATH_DEFPATH, but I
don't know how portable that is (I can't find anything useful in POSIX).

> No tests yet. I'll post some output on that in a minute.

So here is a quick test script to show the output for a couple different
cases. Should this be a real test script? A lot of what is being tested
is the actual stderr output in many cases, which we tend to try not to
include in tests.

-- >8 --
#!/bin/sh

rm -rf bin .git

# bin/broken is a PATH directory that cannot be searched
# bin/ok can be searched, but has a broken entry
mkdir bin bin/broken bin/ok
chmod -x bin/broken

# The "yes" command lets us know when things are working.
cat >bin/ok/git-yes <<\EOF
#!/bin/sh
echo yes
EOF
chmod +x bin/ok/git-yes

# and the "no" command is broken, and should be reported as EACCES
 >bin/ok/git-no

git init -q
git config alias.alias-yes yes
git config alias.alias-no no

PATH=$PWD/bin/broken:$PWD/bin/ok:$PATH

set -x
git does-not-exist
git yes
git no
git alias-yes
git alias-no

-- >8 --

The output I get is:

# stock git
+ git does-not-exist
fatal: cannot exec 'git-does-not-exist': Permission denied
+ git yes
yes
+ git no
fatal: cannot exec 'git-no': Permission denied
+ git alias-yes
fatal: cannot exec 'git-alias-yes': Permission denied
+ git alias-no
fatal: cannot exec 'git-alias-no': Permission denied

# my earlier patches to do alias lookup after EACCES
+ git does-not-exist
Failed to run command 'does-not-exist': Permission denied
+ git yes
yes
+ git no
Failed to run command 'no': Permission denied
+ git alias-yes
yes
+ git alias-no
Expansion of alias 'alias-no' failed; 'no': Permission denied

# this patch
+ git does-not-exist
git: 'does-not-exist' is not a git command. See 'git --help'.
+ git yes
yes
+ git no
fatal: cannot exec 'git-no': Permission denied
+ git alias-yes
yes
+ git alias-no
fatal: cannot exec 'git-no': Permission denied

-Peff

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> # this patch
> + git does-not-exist
> git: 'does-not-exist' is not a git command. See 'git --help'.
> + git yes
> yes
> + git no
> fatal: cannot exec 'git-no': Permission denied
> + git alias-yes
> yes
> + git alias-no
> fatal: cannot exec 'git-no': Permission denied

Looks sane and clean.

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Frans Klaver]

Hi,

On Wed, Mar 28, 2012 at 9:40 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Mar 28, 2012 at 11:29:17AM -0700, Junio C Hamano wrote:
>
>> > That sounds sensible to me. I think it involves writing our own
>> > execvp, though, right? If we use stock execvp, we can't tell the
>> > difference between the two cases.
>>
>> The stock exec*p() will not hit "/bin/ls" in either case, so we will give
>> "'ls' is not a git command", without having to differenciate it.  That is
>> what I meant by "we follow the usual rule to ignore it".
>>
>> We already have the code necessary to enumerate the possible commands from
>> components of the PATH in order to give suggestion, so we can run it
>> after seeing exec*p() failure to see if we did not see any "ls", or we saw
>> "ls" but it was not executable.  No need to penalize the normal case, no?
>
> Yes, we can differentiate after the fact. Though I think it ends up
> being almost the same code as just implementing execvp in the first
> place.

It will, but doesn't stock execv*() also provide access to shell
builtins? If that's the case then I wouldn't be bothered by the extra
bit of code we need to understand what execvp has been doing. I think
it would be sane to keep sane_execvp a wrapper instead of a
reimplementation.

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Frans Klaver]

On Wed, Mar 28, 2012 at 11:57 PM, Jeff King <peff@peff.net> wrote:

> +static int exists_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)) {
> +                       strbuf_release(&buf);
> +                       return 1;
> +               }
> +
> +               if (!*end)
> +                       break;
> +               p = end + 1;
> +       }
> +
> +       strbuf_release(&buf);
> +       return 0;
> +}

I expect that if more post-mortem checking is done, this function is
going to need a sibling that provides you with the first found entry
in PATH, so you can do more checks on it.


> +
> +int sane_execvp(const char *file, char * const argv[])
> +{
> +       if (!execvp(file, argv))
> +               return 0;

> +       if (errno == EACCES && !strchr(file, '/'))
> +               errno = exists_in_PATH(file) ? EACCES : ENOENT;
> +       return -1;
> +}

One of the things I ran into while working on [1] is that quite some
errors that are produced can also be caused by the interpreter. This
does cover most of the itch I had earlier. I will still want to have
the interpreter check [2] in though; errno can for example also be set
to ENOENT if the interpreter or a required library isn't available. In
that case you wouldn't want to continue to the aliases, right?

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jeff King]

On Thu, Mar 29, 2012 at 01:16:47PM +0200, Frans Klaver wrote:

> > Yes, we can differentiate after the fact. Though I think it ends up
> > being almost the same code as just implementing execvp in the first
> > place.
> 
> It will, but doesn't stock execv*() also provide access to shell
> builtins? If that's the case then I wouldn't be bothered by the extra
> bit of code we need to understand what execvp has been doing. I think
> it would be sane to keep sane_execvp a wrapper instead of a
> reimplementation.

No, definitely not. Handling builtins is the responsibility of the
shell, not of execvp. It is responsible for falling back to "/bin/sh
$file" if execve returns ENOEXEC.

Anyway, I think the last round I posted is good enough. It is
approaching execvp in complexity, but it is still a little bit simpler.
And because it's on the error code path, if we are incompatible the
worst thing we can screw up is the error message, not the actual exec.

-Peff

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Jeff King]

On Thu, Mar 29, 2012 at 01:31:09PM +0200, Frans Klaver wrote:

> On Wed, Mar 28, 2012 at 11:57 PM, Jeff King <peff@peff.net> wrote:
> 
> > +static int exists_in_PATH(const char *file)
> [...]
> 
> I expect that if more post-mortem checking is done, this function is
> going to need a sibling that provides you with the first found entry
> in PATH, so you can do more checks on it.

It should be easy to write it that way. I'm not personally planning on
adding more checks, but I think it's worth considering future additions.

> One of the things I ran into while working on [1] is that quite some
> errors that are produced can also be caused by the interpreter.

Yeah, they can be confusing and hard to track down. I'll leave that
topic out of this round, and you can build on it if you like.

-Peff

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Frans Klaver]

On Thu, 29 Mar 2012 19:15:25 +0200, Jeff King <peff@peff.net> wrote:

> On Thu, Mar 29, 2012 at 01:16:47PM +0200, Frans Klaver wrote:
>
>> > Yes, we can differentiate after the fact. Though I think it ends up
>> > being almost the same code as just implementing execvp in the first
>> > place.
>>
>> It will, but doesn't stock execv*() also provide access to shell
>> builtins? If that's the case then I wouldn't be bothered by the extra
>> bit of code we need to understand what execvp has been doing. I think
>> it would be sane to keep sane_execvp a wrapper instead of a
>> reimplementation.
>
> No, definitely not. Handling builtins is the responsibility of the
> shell, not of execvp. It is responsible for falling back to "/bin/sh
> $file" if execve returns ENOEXEC.
>
> Anyway, I think the last round I posted is good enough. It is
> approaching execvp in complexity, but it is still a little bit simpler.
> And because it's on the error code path, if we are incompatible the
> worst thing we can screw up is the error message, not the actual exec.

Good. In that case I think this last looks good, indeed.

Frans

Re: [PATCH 2/2] git: continue alias lookup on EACCES errors

[Frans Klaver]

On Thu, 29 Mar 2012 19:20:33 +0200, Jeff King <peff@peff.net> wrote:

> On Thu, Mar 29, 2012 at 01:31:09PM +0200, Frans Klaver wrote:
>
>> On Wed, Mar 28, 2012 at 11:57 PM, Jeff King <peff@peff.net> wrote:
>>
>> > +static int exists_in_PATH(const char *file)
>> [...]
>>
>> I expect that if more post-mortem checking is done, this function is
>> going to need a sibling that provides you with the first found entry
>> in PATH, so you can do more checks on it.
>
> It should be easy to write it that way. I'm not personally planning on
> adding more checks, but I think it's worth considering future additions.

I have some similar code lying around. It shouldn't be too hard to rebase  
that on top of this.


>> One of the things I ran into while working on [1] is that quite some
>> errors that are produced can also be caused by the interpreter.
>
> Yeah, they can be confusing and hard to track down. I'll leave that
> topic out of this round, and you can build on it if you like.

I was planning to do that, yea.