[PATCH] return status 126 for execution failures other than 'not found'

[PATCH] return status 126 for execution failures other than 'not found'

[Martijn Dekker]

[-- Attachment #1: Type: text/plain, Size: 1677 bytes --]

When the argument list to an external command is too long (exceeds 
ARG_MAX), zsh returns exit status 127. This is incorrect because status 
127 means the command was not found.

POSIX says in "2.8.2 Exit Status for Commands":
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_02
| If a command is not found, the exit status shall be 127. If the
| command name is found, but it is not an executable utility, the exit
| status shall be 126.

Now, the phrasing "it is not an executable utility" is a little vague, 
but the current versions of all other shells return 126 upon any failure 
to execute a utility that was found, so they seem to interpret that 
phrase as meaning "it could not be executed for whatever reason (other 
than 'not found')". In any case, 126 is better than any alternative, as 
all other exit codes potentially conflict with other meanings.

Currently, the execute() function in Src/exec.c only returns status 126 
if the error code is EACCES (permission denied) or ENOEXEC (exec format 
error). In all other cases, 127 is returned.

That logic is not right, because 127 is the specific case: it is only to 
be used if the command was not found. Any failure to execute after the 
command is found should yield status 126.

The attached patch changes that logic to return status 127 if there is 
no error number (which happens if a PATH search does not find a command) 
or if the error number is ENOENT (no such file or directory). In all 
other cases it now returns 126.

I've also added a test for the "argument list too long" case.

- Martijn

-- 
modernish -- harness the shell
https://github.com/modernish/modernish

[-- Attachment #2: status126.patch --]
[-- Type: text/plain, Size: 1466 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index 2b8e2167f..be949049f 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -726,7 +726,7 @@ execute(LinkList args, int flags, int defpath)
 		(arg0[0] == '.' && (arg0 + 1 == s ||
 				    (arg0[1] == '.' && arg0 + 2 == s)))) {
 		zerr("%e: %s", lerrno, arg0);
-		_exit((lerrno == EACCES || lerrno == ENOEXEC) ? 126 : 127);
+		_exit((lerrno == 0 || lerrno == ENOENT) ? 127 : 126);
 	    }
 	    break;
 	}
@@ -805,7 +805,7 @@ execute(LinkList args, int flags, int defpath)
 	_realexit();
     else
 	zerr("command not found: %s", arg0);
-    _exit((eno == EACCES || eno == ENOEXEC) ? 126 : 127);
+    _exit((eno == 0 || eno == ENOENT) ? 127 : 126);
 }
 
 #define RET_IF_COM(X) { if (iscom(X)) return docopy ? dupstring(X) : arg0; }
diff --git a/Test/A05execution.ztst b/Test/A05execution.ztst
index edc561582..64fdd8288 100644
--- a/Test/A05execution.ztst
+++ b/Test/A05execution.ztst
@@ -394,3 +394,13 @@ F:anonymous function, and a descriptor leak when backgrounding a pipeline
 >127
 # TBD: the 0 above is believed to be bogus and should also be turned
 # into 127 when the ccorresponding bug is fixed in the main shell.
+
+# Test for exit status 126 when an external command's argument list is too long.
+  v=foo_bar_baz_quux_lorem_ipsum_dolor_sit_amet
+  while :; do
+    v=$v$v$v$v$v$v$v$v
+    env true $v || { print $?; break; }
+  done
+0:exit status when argument list too long
+>126
+?(eval):4: argument list too long: env

Re: [PATCH] return status 126 for execution failures other than 'not found'

[Bart Schaefer]

On Fri, May 8, 2020 at 1:37 PM Martijn Dekker <martijn@inlv.org> wrote:
>
> Currently, the execute() function in Src/exec.c only returns status 126
> if the error code is EACCES (permission denied) or ENOEXEC (exec format
> error). In all other cases, 127 is returned.
>
> That logic is not right, because 127 is the specific case: it is only to
> be used if the command was not found. Any failure to execute after the
> command is found should yield status 126.

Aside from the "zsh is not POSIX unless invoked as sh" standpoint ...

> The attached patch changes that logic to return status 127 if there is
> no error number (which happens if a PATH search does not find a command)
> or if the error number is ENOENT (no such file or directory). In all
> other cases it now returns 126.

... shouldn't EPERM also be in that list?  A file that's not
executable or otherwise not reachable seems to fall into the "not
found" category.

Re: [PATCH] return status 126 for execution failures other than 'not found'

[Daniel Shahaf]

Martijn Dekker wrote on Fri, 08 May 2020 21:36 +0100:
> When the argument list to an external command is too long (exceeds 
> ARG_MAX), zsh returns exit status 127. This is incorrect because status 
> 127 means the command was not found.
> 
> POSIX says in "2.8.2 Exit Status for Commands":
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_02
> | If a command is not found, the exit status shall be 127. If the
> | command name is found, but it is not an executable utility, the exit
> | status shall be 126.
> 

In the test case you added, I think the answers to "Was «env» found?"
and "Is «env» an executable utility?" are both "yes".  Thus, neither of
the sentence quoted from §2.8.2 applies: the exit status in that case is
neither required to be 127 nor required to be 126.

That would appear to be a lacuna: the standard does not specify what the
exit status shall be when the command _is_ found and _is_ an "executable
utility", but execution fails for some other reason, such as an argument
being too long.

Furthermore, the standard doesn't _forbid_ the exit status of the test
you added from being either 126 or 127.  (That's the plain meaning of
quoted sentences: they say "if A then B", not "A iff B".)

It _would_ be useful for error codes to only be caused by specific
failure modes, in order to enable scripts to handle error codes
selectively.  However, the incumbent code has this property too:
status 126 has only two specific causes.

We could still make the change you propose, of course, but it's not
a black and white case of the incumbent behaviour being "incorrect".
Therefore, in particular, we should consider not only whether we'd be
compatible with other shells, but also whether the proposed change might
constitute a regression for people upgrading from older zsh to newer
zsh.  For example, might some zsh users be relying on the incumbent
"status 126 can only be caused by EACCES and ENOEXEC" semantics?

To be clear, I'm not objecting to the change; I'm only trying to ensure
all viewpoints are taken into account.

Thanks, Martijn.

Daniel

> Now, the phrasing "it is not an executable utility" is a little vague, 
> but the current versions of all other shells return 126 upon any failure 
> to execute a utility that was found, so they seem to interpret that 
> phrase as meaning "it could not be executed for whatever reason (other 
> than 'not found')". In any case, 126 is better than any alternative, as 
> all other exit codes potentially conflict with other meanings.
> 
> Currently, the execute() function in Src/exec.c only returns status 126 
> if the error code is EACCES (permission denied) or ENOEXEC (exec format 
> error). In all other cases, 127 is returned.
> 
> That logic is not right, because 127 is the specific case: it is only to 
> be used if the command was not found. Any failure to execute after the 
> command is found should yield status 126.
> 
> The attached patch changes that logic to return status 127 if there is 
> no error number (which happens if a PATH search does not find a command) 
> or if the error number is ENOENT (no such file or directory). In all 
> other cases it now returns 126.
> 
> I've also added a test for the "argument list too long" case.