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

[Jeff King <peff@peff.net>]

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