patchrun-command.c: Accept EACCES as command not found

> 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,...

> 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

> 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.

> 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

> 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

> +#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;

> +	 * - 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 (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);

> 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?

> 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.

>> +				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?

>> 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))?

> "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?
>

> 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')) {

> +	}
> +	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);

> @@ -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

> 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].

> +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)) {

>
>  - 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.

>
> 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.

> 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.