Skip to contentSkip to conversation

debian patches to exit with code 127 for nonexistent/directory scripts

PATCH In review Jilles Tjoelker 6 participants 31 messages Jun 5, 2010 – Nov 28, 2010

lore
Share & tools
lore

Jilles Tjoelker

author
#0 lore raw

Debian's dash package has some local changes which cause an exit with code 127, as required by POSIX, if a script (passed with dash <filename>) cannot be opened or cannot be read because it is a directory.

Unfortunately, these patches also affect the . builtin (if the pathname contains a slash) and use EXEXIT, which means such errors always cause the shell to exit, even in interactive mode or if the builtin's specialness has been disabled using command.

% dash

$ . ./nonexistent
.: 1: Can't open ./nonexistent
zsh: exit 127   dash
%

% dash -c 'command . ./nonexistent; echo continued'

Note: Do not compare this with bash. Bash deliberately does not follow POSIX XCU 2.8.1 Consequences of Shell Errors if not in POSIX mode, and even in POSIX mode trying to source a nonexistent dot script (without slash in the pathname) fails to abort the shell.

Note 2: POSIX seems unclear about what 'command .' should do, but is very clear that failure to find/read a dot script shall not cause an interactive shell to exit.

Signature · 1 line
Jilles Tjoelker
#1 lore raw
Quoted 4 lines · Jilles Tjoelker

Debian's dash package has some local changes which cause an exit with code 127, as required by POSIX, if a script (passed with dash <filename>) cannot be opened or cannot be read because it is a directory.

Please report this through Debian's bug tracking system.

There is nothing that I can do about this.

Thanks,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#2 lore raw
Quoted 5 lines · Herbert Xu
Quoted 4 lines · Jilles Tjoelker

Debian's dash package has some local changes which cause an exit with code 127, as required by POSIX, if a script (passed with dash <filename>) cannot be opened or cannot be read because it is a directory.

My interpretation of the above is that debian's patched dash is POSIX compliant WRT the named exit code.

Quoted 1 line

Please report this through Debian's bug tracking system.

Why?

Quoted 1 line

There is nothing that I can do about this.

Upstream dash is _not_ POSIX compliant WRT the named exit code, seems to be the meaning of the sentence. And you can certainly _do_ something about it.

On the other hand, I may have got it all wrong (because of some language barrier), in which case please accept my apologies.

Cheers,

Signature · 1 line
Cristian
#3 lore raw
Quoted 10 lines · Cristian Ionescu-Idbohrn
Quoted 5 lines · Herbert Xu
Quoted 4 lines · Jilles Tjoelker

Debian's dash package has some local changes which cause an exit with code 127, as required by POSIX, if a script (passed with dash <filename>) cannot be opened or cannot be read because it is a directory.

My interpretation of the above is that debian's patched dash is POSIX compliant WRT the named exit code.

Well if dash is broken then you should submit a patch.

Thanks,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#4 lore raw

PATCH 1/1 exit 127 if command_file is given but doesn't exist

In reply to Jilles Tjoelker

This commit makes dash exit with return code 127 instead of 2 if started as non-interactive shell with a non-existent command_file specified as argument (or a directory), as documented in
http://www.opengroup.org/onlinepubs/009695399/utilities/sh.html#tag_04_128_14

The wrong exit code was reported by Clint Adams and Jari Aalto through
http://bugs.debian.org/548743
http://bugs.debian.org/548687

Signed-off-by Gerrit Pape <[email protected]>

---

Quoted 10 lines · Jilles Tjoelker

Debian's dash package has some local changes which cause an exit with code 127, as required by POSIX, if a script (passed with dash <filename>) cannot be opened or cannot be read because it is a directory.

Unfortunately, these patches also affect the . builtin (if the pathname
contains a slash) and use EXEXIT, which means such errors always cause the shell to exit, even in interactive mode or if the builtin's specialness has been disabled using command.

Hi Jilles, thanks for the hint, I didn't notice. I hope this patch does better, it replaces these two
http://article.gmane.org/gmane.comp.shells.dash/198
http://article.gmane.org/gmane.comp.shells.dash/199

Regards, Gerrit.

 src/error.h |    1 +
 src/input.c |    7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)
src/error.h +1 -0
index 3162e15..c75cb2c 100644
@@ -69,6 +69,7 @@ extern int exception;
 #define EXSHELLPROC 2	/* execute a shell procedure */
 #define EXEXEC 3	/* command execution failed */
 #define EXEXIT 4	/* exit the shell */
+#define EXIFILE 5	/* inputfile cannot be opened or is a directory */
 
 
 /*
src/input.c +5 -2
index e57ad76..a43a7b9 100644
@@ -54,6 +54,7 @@
 #include "parser.h"
 #include "main.h"
 #include "var.h"
+#include "eval.h"
 #ifndef SMALL
 #include "myhistedit.h"
 #endif

@@ -405,11 +406,13 @@ setinputfile(const char *fname, int flags)
 	int fd;
 
 	INTOFF;
-	if ((fd = open(fname, O_RDONLY)) < 0) {
+	if ((fd = open(fname, O_RDWR)) < 0) {
 		if (flags & INPUT_NOFILE_OK)
 			goto out;
-		sh_error("Can't open %s", fname);
+		exitstatus = 127;
+		exerror(EXIFILE, "Can't open %s", fname);
 	}
+	fcntl(fd, F_SETFL, O_RDONLY);
 	if (fd < 10)
 		fd = savefd(fd, fd);
 	setinputfd(fd, flags & INPUT_PUSH_FILE);
Signature · 1 line
1.6.0.3
#5 lore raw

Re: [PATCH] [INPUT] exit 127 if command_file is given but doesn't exist

src/input.c setinputfile(const char *fname, int flags) · line 409

Quoted 7 lines · Gerrit Pape
src/input.c +1 -1
@@ -405,11 +406,13 @@ setinputfile(const char *fname, int flags)
 	int fd;
 
 	INTOFF;
-	if ((fd = open(fname, O_RDONLY)) < 0) {
+	if ((fd = open(fname, O_RDWR)) < 0) {

Why should the file be writeable?

Please do not mix multiple changes in one patch.

src/input.c setinputfile(const char *fname, int flags) · line 415

Quoted 7 lines
src/input.c +3 -1
 		if (flags & INPUT_NOFILE_OK)
 			goto out;
-		sh_error("Can't open %s", fname);
+		exitstatus = 127;
+		exerror(EXIFILE, "Can't open %s", fname);
 	}
+	fcntl(fd, F_SETFL, O_RDONLY);

This is wrong. You're creating exactly the problem that Jilles was talking about where dot(1) is returning 127. This code should be moved to procargs, which runs only for the "sh script" case.

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#6 lore raw

PATCH 1/1 Use exit status 127 when the script to run does not exist

Currently dash uses exit status 2 to report failure to run a script named on the command line that does not exist:

 $ sh nonexistent
 sh: Can't open nonexistent
 $ echo $?
 2

According to SUSv3 and SUSv4[1], the exit code should be 127.

So check errno when opening a command_file during argument processing and exit(127) when it is ENOENT. This check is in the argument processing code to avoid collateral damage to code paths that read scripts (such as the dot builtin). Other errors (such as EACCESS) while opening the command_file will still result in an exit code of 2.

[1] http://www.opengroup.org/onlinepubs/9699919799/utilities/sh.html#tag_20_117_14

Reported-by Clint Adams <[email protected]>
Fixes http://bugs.debian.org/548743
Signed-off-by Jonathan Nieder <[email protected]>

---

Quoted 3 lines · Herbert Xu

This is wrong. You're creating exactly the problem that Jilles was talking about where dot(1) is returning 127. This code should be moved to procargs, which runs only for the "sh script" case.

Makes sense. How does this look?

It still ignores attempts to run "sh directory"; see the next patch for that.

 src/options.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)
src/options.c +8 -1
index 6f381e6..5667b68 100644
@@ -37,6 +37,7 @@
 #include <stdlib.h>
 
 #include "shell.h"
+#include "main.h"
 #define DEFINE_OPTIONS
 #include "options.h"
 #undef DEFINE_OPTIONS

@@ -156,7 +157,13 @@ procargs(int argc, char **argv)
 		if (*xargv)
 			goto setarg0;
 	} else if (!sflag) {
-		setinputfile(*xargv, 0);
+		if (setinputfile(*xargv, INPUT_NOFILE_OK) < 0) {
+			if (errno == ENOENT) {
+				exitstatus = 127;
+				exerror(EXEXIT, "Can't open %s", *xargv);
+			}
+			sh_error("Can't open %s", *xargv);
+		}
 setarg0:
 		arg0 = *xargv++;
 		commandname = arg0;
Signature · 1 line
1.7.2.3
#7 lore raw

PATCH 1/1 Catch attempts to run a directory as a script

open() succeeds, though read() fails later, when a pathname passed as argv[0] on the command line points to a directory, which somehow results in dash thinking everything is okay:

 $ sh dir/
 $ echo $?
 0

But POSIX makes it clear enough that in "sh command_file", command_file is supposed to be a file, not a directory. So diagnose this with an error message and exit with status 2.

Reported-by Jari Aalto <[email protected]>
Fixes http://bugs.debian.org/548687
Signed-off-by Jonathan Nieder <[email protected]>

---
Applies on top of the previous patch, which is based on master.

Thanks to Gerrit and Krzysztof for your help so far.

 src/input.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)
src/input.c +20 -1
index e57ad76..d68d441 100644
@@ -395,6 +395,19 @@ popstring(void)
 }
 
+/*
+ * Is fd a directory file descriptor?
+ */
+
+static int
+isdir(const char *fname, int fd)
+{
+	struct stat64 statb;
+	if (fstat64(fd, &statb) < 0)
+		sh_error("Can't stat %s", fname);
+	return S_ISDIR(statb.st_mode);
+}
+
 /*
  * Set the input to take input from a file.  If push is set, push the
  * old input onto the stack first.
  */

@@ -405,7 +418,13 @@ setinputfile(const char *fname, int flags)
 	int fd;
 
 	INTOFF;
-	if ((fd = open(fname, O_RDONLY)) < 0) {
+	fd = open(fname, O_RDONLY);
+	if (isdir(fname, fd)) {
+		close(fd);
+		fd = -1;
+		errno = EISDIR;
+	}
+	if (fd < 0) {
 		if (flags & INPUT_NOFILE_OK)
 			goto out;
 		sh_error("Can't open %s", fname);
Signature · 1 line
1.7.2.3
#8 lore raw

Re: [PATCH] [INPUT] Catch attempts to run a directory as a script

Quoted 8 lines · Jonathan Nieder

open() succeeds, though read() fails later, when a pathname passed as argv[0] on the command line points to a directory, which somehow results in dash thinking everything is okay:

 $ sh dir/
 $ echo $?
 0
Quoted 7 lines

But POSIX makes it clear enough that in "sh command_file", command_file is supposed to be a file, not a directory. So diagnose this with an error message and exit with status 2.

Reported-by Jari Aalto <[email protected]>
Fixes http://bugs.debian.org/548687
Signed-off-by Jonathan Nieder <[email protected]>

Is this required by POSIX? If not this is simply making dash bigger for no good reason.

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#9 lore raw

Re: [PATCH] [INPUT] Catch attempts to run a directory as a script

Quoted 5 lines · Herbert Xu
Quoted 3 lines · Jonathan Nieder

But POSIX makes it clear enough that in "sh command_file", command_file is supposed to be a file, not a directory. So diagnose this with an error message and exit with status 2.

[...]

Quoted 2 lines

Is this required by POSIX? If not this is simply making dash bigger for no good reason.

Not clear. I suppose POSIX usually doesn't require anything when the caller screws up.

Jari Aalto quoted the DESCRIPTION section[1] (and some similar passages):

Quoted 3 lines

The sh utility is a command language interpreter that shall execute commands read from a command line string, the standard

Quoted 1 line
Quoted 1 line

input, or a specified file.

I don't find that alone very convincing.

Under OPERANDS[2]: if the path contains a slash, all the standard says is "the implementation attempts to read that file". If the path does not contain a slash and the file is not in the working directory, the implementation _may_ perform a search as described in "Command Search and Execution".

During that search, after execve() fails, "if the executable file is not a text file, the shell _may_ bypass this command execution. In this case, it shall write an error message, and shall return an exit status of 126." (emphasis mine).

So this behavior is allowed as an optional subset of an optional behavior. That may have guided the bash implementors:

 $ bash directory
 directory: directory: is a directory
 $ echo $?
 126

It's probably not required.

[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=10;bug=548687
[2] http://www.opengroup.org/onlinepubs/9699919799/utilities/sh.html#tag_20_117_05

#10 lore raw

Re: [PATCH] [INPUT] Catch attempts to run a directory as a script

Quoted 9 lines · Jonathan Nieder
Quoted 3 lines
Quoted 3 lines

But POSIX makes it clear enough that in "sh command_file", command_file is supposed to be a file, not a directory. So diagnose this with an error message and exit with status 2.

[...]

Quoted 2 lines

Is this required by POSIX? If not this is simply making dash bigger for no good reason.

Not clear. I suppose POSIX usually doesn't require anything when the caller screws up.

POSIX requires that input files to bash shall be text files; directories do not qualify for this.
http://www.opengroup.org/onlinepubs/9699919799/utilities/sh.html
"The input file shall be a text file, except that line lengths shall be unlimited. "

However, that is a requirement on the user, not the shell; so running 'sh /' is a constraint violation by the user, and leaves behavior up to the shell.

Quoted 5 lines

Under OPERANDS[2]: if the path contains a slash, all the standard says is "the implementation attempts to read that file". If the path does not contain a slash and the file is not in the working directory, the implementation _may_ perform a search as described in "Command Search and Execution".

It's more than just MAY; it's a requirement:
http://www.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01_01

"If the command name contains at least one <slash>, the shell shall execute the utility in a separate utility environment with actions equivalent to calling the execve() function...

"If the execve() function fails due to an error equivalent to the [ENOEXEC] error, the shell shall execute a command equivalent to having a shell invoked with the command name as its first operand"

Quoted 5 lines

During that search, after execve() fails, "if the executable file is not a text file, the shell _may_ bypass this command execution. In this case, it shall write an error message, and shall return an exit status of 126." (emphasis mine).

But yes, that same section is clear that for both command searches along PATH for a word without slash, and for a direct command with a slash, if execve() fails with ENOEXEC (as it does for directories), then it is optional whether the shell bypasses attempts to read the file because it was not a text file.

On the other hand, in Linux, execve(".",...) fails with EACCES, as permitted by the standard:

http://www.opengroup.org/onlinepubs/9699919799/functions/execve.html
"[EACCES] ...or the new process image file is not a regular file and the implementation does not support execution of files of its type."

And since EACCES is not the same class as ENOEXEC, there is no requirement for the shell to attempt to execute the same file. So, rather than stat()ing the argument in advance and checking for S_ISDIR, it seems like it would be simpler to check after the execve() attempt for EACCES and blindly set $? to 126 in that case (since you already have to check for ENOEXEC).

Quoted 9 lines

So this behavior is allowed as an optional subset of an optional behavior. That may have guided the bash implementors:

  $ bash directory
  directory: directory: is a directory
  $ echo $?
  126

It's probably not required.

Additionally, the standard REQUIRES that 'sh -c "exec /"' shall fail with status 126:

http://www.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#exec
"If command is found, but it is not an executable utility, the exit status shall be 126."

Right now, dash gets this wrong:

dash -c 'exec .'; echo $?
exec: 1: /: Permission denied
2

And since you already have the code in dash to detect failure to 'exec' a directory, you should be able to reuse that code when detecting failure to run a directory as a script, as in 'dash .'.

[Hmm, bash also gets it wrong:
bash -c 'exec .'; echo $?
bash: line 0: exec: .: not found
127
even though . should always be found]

Signature · 2 lines
Eric Blake   [email protected]    +1-801-349-2682
Libvirt virtualization library http://libvirt.org
#11 lore raw

Re: [PATCH] [INPUT] Catch attempts to run a directory as a script

Quoted 10 lines · Eric Blake

Right now, dash gets this wrong:

dash -c 'exec .'; echo $?
exec: 1: /: Permission denied
2

And since you already have the code in dash to detect failure to 'exec' a directory, you should be able to reuse that code when detecting failure to run a directory as a script, as in 'dash .'.

Thanks for the detailed investiagtion!

If someone could send me a patch that does this without bloating dash too much then I can apply it without feeling too much guilt :)

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#12 lore raw

cover Fix exit status for 'exec nonexistent' and 'exec .'

In reply to Eric Blake
Quoted 9 lines · Eric Blake

Additionally, the standard REQUIRES that 'sh -c "exec /"' shall fail with status 126:

http://www.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#exec
"If command is found, but it is not an executable utility, the exit status shall be 126."

Right now, dash gets this wrong:

Quoted 7 lines

dash -c 'exec .'; echo $?
exec: 1: /: Permission denied
2

And since you already have the code in dash to detect failure to 'exec' a directory, you should be able to reuse that code when detecting failure to run a directory as a script, as in 'dash .'.

Summary:

Command Status Expected status
1) exec . 2 126
2) exec nonexistent 2 127
3) sh . 0 126
4) sh nonexistent 2 127

(1) and (2) seem to be regressions from about 5 years ago. Reverting the problematic patch fixes it for me; see below.

(3) is not clearly documented in the standard, and there is some disagreement between shells on how to handle it[1]. The current behavior seems fine to me.

(4) is a clear bug, well documented in the EXIT STATUS section of the description of sh in the "Utilities" chapter. See
http://thread.gmane.org/gmane.comp.shells.dash/291/focus=390
for a patch[2].

Thoughts?
Jonathan Nieder (3):
[EXCEPTIONS] Stop documenting EXSHELLPROC
Revert "Eliminated global exerrno."
[EXCEPTIONS] Eliminate global exerrno

 src/TOUR    |   11 +----------
 src/error.h |    5 ++---
 src/eval.c  |    8 ++++----
 3 files changed, 7 insertions(+), 17 deletions(-)

[1]

$ bash .; echo $?
.: .: is a directory
126
$ ksh93 .; echo $?
ksh93: .: cannot open [Is a directory]
126
$ pdksh .; echo $?
0

[2] Unfortunately this does not share code with the fix to (1) and (2) as Eric hoped. Why? The system calls involved are
different: the exec builtin uses execve(), while 'sh <script>' uses open() and read().

#13 lore raw

PATCH 1/3 Stop documenting EXSHELLPROC

At some point between ash 0.3.5-11.0.1 and ash 0.3.8-37, Debian ash stopped using the EXSHELLPROC exception to handle shell scripts without a magic number.

Remove all remaining references to it to avoid confusion.

Signed-off-by Jonathan Nieder <[email protected]>
---
 src/TOUR    |   11 +----------
 src/error.h |    5 ++---
 2 files changed, 3 insertions(+), 13 deletions(-)
src/TOUR +1 -10
index 4baac62..056e79b 100644
@@ -43,10 +43,6 @@ C source files for entries looking like:
                            back to the main command loop */
         }
 
-        SHELLPROC {
-              x = 3;    /* executed when the shell runs a shell procedure */
-        }
-
 It pulls this code out into routines which are when particular
 events occur.  The intent is to improve modularity by isolating
 the information about which modules need to be explicitly

@@ -79,12 +75,7 @@ EXCEPTIONS:  Code for dealing with exceptions appears in
 exceptions.c.  The C language doesn't include exception handling,
 so I implement it using setjmp and longjmp.  The global variable
 exception contains the type of exception.  EXERROR is raised by
-calling error.  EXINT is an interrupt.  EXSHELLPROC is an excep-
-tion which is raised when a shell procedure is invoked.  The pur-
-pose of EXSHELLPROC is to perform the cleanup actions associated
-with other exceptions.  After these cleanup actions, the shell
-can interpret a shell procedure itself without exec'ing a new
-copy of the shell.
+calling error.  EXINT is an interrupt.
 
 INTERRUPTS:  In an interactive shell, an interrupt will cause an
 EXINT exception to return to the main command loop.  (Exception:
src/error.h +2 -3
index 3162e15..15ccbe3 100644
@@ -66,9 +66,8 @@ extern int exception;
 /* exceptions */
 #define EXINT 0		/* SIGINT received */
 #define EXERROR 1	/* a generic error */
-#define EXSHELLPROC 2	/* execute a shell procedure */
-#define EXEXEC 3	/* command execution failed */
-#define EXEXIT 4	/* exit the shell */
+#define EXEXEC 2	/* command execution failed */
+#define EXEXIT 3	/* exit the shell */
 
 
 /*
Signature · 1 line
1.7.2.3
#14 lore raw

Re: [PATCH 1/3] [EXCEPTIONS] Stop documenting EXSHELLPROC

Quoted 6 lines · Jonathan Nieder
src/error.h +0 -0
index 3162e15..15ccbe3 100644

src/error.h extern int exception; · line 70

Quoted 8 lines
src/error.h +2 -3
@@ -66,9 +66,8 @@ extern int exception;
 /* exceptions */
 #define EXINT 0		/* SIGINT received */
 #define EXERROR 1	/* a generic error */
-#define EXSHELLPROC 2	/* execute a shell procedure */
-#define EXEXEC 3	/* command execution failed */
-#define EXEXIT 4	/* exit the shell */
+#define EXEXEC 2	/* command execution failed */
+#define EXEXIT 3	/* exit the shell */

There is no need to renumber things. Just leave a gap.

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#15 lore raw

Re: [PATCH 1/3] [EXCEPTIONS] Stop documenting EXSHELLPROC

Quoted 15 lines · Herbert Xu

src/error.h extern int exception; · line 70

Quoted 11 lines · Jonathan Nieder
src/error.h +2 -3
@@ -66,9 +66,8 @@ extern int exception;
 /* exceptions */
 #define EXINT 0		/* SIGINT received */
 #define EXERROR 1	/* a generic error */
-#define EXSHELLPROC 2	/* execute a shell procedure */
-#define EXEXEC 3	/* command execution failed */
-#define EXEXIT 4	/* exit the shell */
+#define EXEXEC 2	/* command execution failed */
+#define EXEXIT 3	/* exit the shell */

There is no need to renumber things. Just leave a gap.

Ok. Should I fix it up or would you like to?

Thanks,
Jonathan

#16 lore raw

Re: [PATCH 1/3] [EXCEPTIONS] Stop documenting EXSHELLPROC

Quoted 17 lines · Jonathan Nieder
Quoted 15 lines · Herbert Xu

src/error.h extern int exception; · line 70

Quoted 11 lines · Jonathan Nieder
src/error.h +2 -3
@@ -66,9 +66,8 @@ extern int exception;
 /* exceptions */
 #define EXINT 0		/* SIGINT received */
 #define EXERROR 1	/* a generic error */
-#define EXSHELLPROC 2	/* execute a shell procedure */
-#define EXEXEC 3	/* command execution failed */
-#define EXEXIT 4	/* exit the shell */
+#define EXEXEC 2	/* command execution failed */
+#define EXEXIT 3	/* exit the shell */

There is no need to renumber things. Just leave a gap.

Quoted 1 line

Ok. Should I fix it up or would you like to?

Yeah please resend the patch with the fix-up.

Thanks!

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#17 lore raw

PATCH v2 1/1 Stop documenting EXSHELLPROC

At some point between ash 0.3.5-11.0.1 and ash 0.3.8-37, Debian ash stopped using the EXSHELLPROC exception to handle shell scripts without a magic number.

Remove all remaining references to it to avoid confusion.

Signed-off-by Jonathan Nieder <[email protected]>

---

Quoted 1 line · Herbert Xu

Yeah please resend the patch with the fix-up.

Done. (Patch is against master, as before.)

 src/TOUR    |   11 +----------
 src/error.h |    1 -
 2 files changed, 1 insertions(+), 11 deletions(-)
src/TOUR +1 -10
index 4baac62..056e79b 100644
@@ -43,10 +43,6 @@ C source files for entries looking like:
                            back to the main command loop */
         }
 
-        SHELLPROC {
-              x = 3;    /* executed when the shell runs a shell procedure */
-        }
-
 It pulls this code out into routines which are when particular
 events occur.  The intent is to improve modularity by isolating
 the information about which modules need to be explicitly

@@ -79,12 +75,7 @@ EXCEPTIONS:  Code for dealing with exceptions appears in
 exceptions.c.  The C language doesn't include exception handling,
 so I implement it using setjmp and longjmp.  The global variable
 exception contains the type of exception.  EXERROR is raised by
-calling error.  EXINT is an interrupt.  EXSHELLPROC is an excep-
-tion which is raised when a shell procedure is invoked.  The pur-
-pose of EXSHELLPROC is to perform the cleanup actions associated
-with other exceptions.  After these cleanup actions, the shell
-can interpret a shell procedure itself without exec'ing a new
-copy of the shell.
+calling error.  EXINT is an interrupt.
 
 INTERRUPTS:  In an interactive shell, an interrupt will cause an
 EXINT exception to return to the main command loop.  (Exception:
src/error.h +0 -1
index 3162e15..be0eec9 100644
@@ -66,7 +66,6 @@ extern int exception;
 /* exceptions */
 #define EXINT 0		/* SIGINT received */
 #define EXERROR 1	/* a generic error */
-#define EXSHELLPROC 2	/* execute a shell procedure */
 #define EXEXEC 3	/* command execution failed */
 #define EXEXIT 4	/* exit the shell */
Signature · 1 line
1.7.2.3
#18 lore raw

Re: [PATCH v2] [EXCEPTIONS] Stop documenting EXSHELLPROC

Quoted 7 lines · Jonathan Nieder

At some point between ash 0.3.5-11.0.1 and ash 0.3.8-37, Debian ash stopped using the EXSHELLPROC exception to handle shell scripts without a magic number.

Remove all remaining references to it to avoid confusion.

Signed-off-by Jonathan Nieder <[email protected]>

Also applied.

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#19 lore raw

PATCH 2/3 Revert "Eliminated global exerrno."

In reply to Jonathan Nieder

This reverts commit c0e07c010e5abdea1a7d1357edb1d08adac529cb.

Yes, it is nicer to pass the exit status through the exitstatus global, but look at the consequences:

 $ sh -c 'exec nonexistent'; echo $?
 exec: 1: nonexistent: not found
 2
 $ sh -c 'exec .'; echo $?
 exec: 1: .: Permission denied
 2

According to POSIX:
"If command is not found, the exit status shall be 127. If command is found, but it is not an executable utility, the exit status shall be 126."

Make it so again.

Reported-by Eric Blake <[email protected]>
Signed-off-by Jonathan Nieder <[email protected]>
---
 src/error.h |    1 +
 src/exec.c  |    3 +--
 src/main.c  |    2 ++
 3 files changed, 4 insertions(+), 2 deletions(-)
src/error.h +1 -0
index 15ccbe3..1bb6a82 100644
@@ -62,6 +62,7 @@ struct jmploc {
 
 extern struct jmploc *handler;
 extern int exception;
+extern int exerrno;	/* error for EXEXEC */
 
 /* exceptions */
 #define EXINT 0		/* SIGINT received */
src/exec.c +1 -2
index 42299ea..9525a16 100644
@@ -86,6 +86,7 @@ struct tblentry {
 
 STATIC struct tblentry *cmdtable[CMDTABLESIZE];
 STATIC int builtinloc = -1;		/* index in path of %builtin, or -1 */
+int exerrno;				/* Last exec error */
 
 
 STATIC void tryexec(char *, char **, char **);

@@ -108,7 +109,6 @@ shellexec(char **argv, const char *path, int idx)
 	char *cmdname;
 	int e;
 	char **envp;
-	int exerrno;
 
 	envp = environment();
 	if (strchr(argv[0], '/') != NULL) {

@@ -138,7 +138,6 @@ shellexec(char **argv, const char *path, int idx)
 		exerrno = 2;
 		break;
 	}
-	exitstatus = exerrno;
 	TRACE(("shellexec failed for %s, errno %d, suppressint %d\n",
 		argv[0], e, suppressint ));
 	exerror(EXEXEC, "%s: %s", argv[0], errmsg(e, E_EXEC));
src/main.c +2 -0
index 2bff956..5bc1a99 100644
@@ -115,6 +115,8 @@ main(int argc, char **argv)
 		e = exception;
 		if (e == EXERROR)
 			exitstatus = 2;
+		else if (e == EXEXEC)
+			exitstatus = exerrno;
 
 		s = state;
 		if (e == EXEXIT || s == 0 || iflag == 0 || shlvl)
Signature · 1 line
1.7.2.3
#20 lore raw

Re: [PATCH 2/3] Revert "Eliminated global exerrno."

Quoted 17 lines · Jonathan Nieder

This reverts commit c0e07c010e5abdea1a7d1357edb1d08adac529cb.

Yes, it is nicer to pass the exit status through the exitstatus global, but look at the consequences:

 $ sh -c 'exec nonexistent'; echo $?
 exec: 1: nonexistent: not found
 2
 $ sh -c 'exec .'; echo $?
 exec: 1: .: Permission denied
 2

According to POSIX:
"If command is not found, the exit status shall be 127. If command is found, but it is not an executable utility, the exit status shall be 126."

Quoted 4 lines

Make it so again.

Reported-by Eric Blake <[email protected]>
Signed-off-by Jonathan Nieder <[email protected]>

Actually the bug is elsewhere. This patch works for me.

commit 7f684260a2426ac61c06d2e4822429b00437ae24

Author: Herbert Xu <[email protected]>
Date:   Thu Oct 7 10:55:15 2010 +0800

[BUILTIN] Fix EXEXEC status clobbering

evalcommand always clobbers the exit status in case of an EXEXEC which means that exec always fails with exit status 2 regardless of what it actually returns.

This patch adds the missing check for EXEXEC so that the correct exit status is preserved.

Signed-off-by Herbert Xu <[email protected]>
ChangeLog +4 -0
index 1dfe241..2faaedd 100644
@@ -1,3 +1,7 @@
+2010-10-07  Herbert Xu <[email protected]>
+
+	* Fix EXEXEC status clobbering.
+
 2010-09-08  Herbert Xu <[email protected]>
 
 	* Fix ifsfirst/ifslastp leak.
src/eval.c +1 -1
index 5b8d36b..b966749 100644
@@ -854,7 +854,7 @@ bail:
 			int i;
 
 			i = exception;
-			if (i == EXEXIT)
+			if (i == EXEXIT || i == EXEXEC)
 				goto raise;
 
 			status = (i == EXINT) ? SIGINT + 128 : 2;

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#21 lore raw

Re: [PATCH 2/3] Revert "Eliminated global exerrno."

Quoted 1 line · Herbert Xu

Actually the bug is elsewhere.

It does bisect to there. :) But you're right, it would have been simpler to send one patch.

src/eval.c bail: · line 858

Quoted 9 lines
src/eval.c +1 -1
@@ -854,7 +854,7 @@ bail:
 			int i;
 
 			i = exception;
-			if (i == EXEXIT)
+			if (i == EXEXIT || i == EXEXEC)
 				goto raise;

Good call. This is better than my patch because it exits like it ought to for

command exec nonexistent

(as POSIX says:

If command is specified, exec shall not return to the shell

).

Maybe the following would make sense on top?

-- 8< --
Subject: [EXCEPTIONS] Use EXEXIT in place of EXEXEC

The intended semantics of EXEXEC are identical to EXEXIT, so simplify by using EXEXIT directly.

Functional change: in edge cases (exec within a trap handler), this causes the exit status from exec not to be clobbered. For example, without this patch:

 $ sh -c 'trap "exec nonexistent" EXIT'; echo $?
 exec: 1: nonexistent: not found
 0

And with it:

 $ sh -c 'trap "exec nonexistent" EXIT'; echo $?
 exec: 1: nonexistent: not found
 127
Signed-off-by Jonathan Nieder <[email protected]>
---
 src/error.h |    1 -
 src/eval.c  |    2 +-
 src/exec.c  |    2 +-
 3 files changed, 2 insertions(+), 3 deletions(-)
src/error.h +0 -1
index be0eec9..f236d9f 100644
@@ -66,7 +66,6 @@ extern int exception;
 /* exceptions */
 #define EXINT 0		/* SIGINT received */
 #define EXERROR 1	/* a generic error */
-#define EXEXEC 3	/* command execution failed */
 #define EXEXIT 4	/* exit the shell */
src/eval.c +1 -1
index b966749..5b8d36b 100644
@@ -854,7 +854,7 @@ bail:
 			int i;
 
 			i = exception;
-			if (i == EXEXIT || i == EXEXEC)
+			if (i == EXEXIT)
 				goto raise;
 
 			status = (i == EXINT) ? SIGINT + 128 : 2;
src/exec.c +1 -1
index 42299ea..b273420 100644
@@ -141,7 +141,7 @@ shellexec(char **argv, const char *path, int idx)
 	exitstatus = exerrno;
 	TRACE(("shellexec failed for %s, errno %d, suppressint %d\n",
 		argv[0], e, suppressint ));
-	exerror(EXEXEC, "%s: %s", argv[0], errmsg(e, E_EXEC));
+	exerror(EXEXIT, "%s: %s", argv[0], errmsg(e, E_EXEC));
 	/* NOTREACHED */
 }
Signature · 1 line
1.7.2.3
#22 lore raw

Re: [PATCH 2/3] Revert "Eliminated global exerrno."

Quoted 22 lines · Jonathan Nieder
Quoted 1 line · Herbert Xu

Actually the bug is elsewhere.

It does bisect to there. :) But you're right, it would have been simpler to send one patch.

src/eval.c bail: · line 858

Quoted 9 lines
src/eval.c +1 -1
@@ -854,7 +854,7 @@ bail:
 			int i;
 
 			i = exception;
-			if (i == EXEXIT)
+			if (i == EXEXIT || i == EXEXEC)
 				goto raise;

Good call. This is better than my patch because it exits like it ought to for

command exec nonexistent

Quoted 7 lines

(as POSIX says:

If command is specified, exec shall not return to the shell

).

Maybe the following would make sense on top?

Nice! I like where you are going with this :)

Let me run it through the tests and apply it.

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#23 lore raw

Re: [PATCH 2/3] Revert "Eliminated global exerrno."

Quoted 2 lines · Herbert Xu

Let me run it through the tests and apply it.

There is one problem with this and that is changing the exit status in a trap:

sh -c 'trap "exec nosuchfile" EXIT; exit 3'; echo $?

This makes dash exit with 127 instead of 3 as it does now.

AFAIK the wording of POSIX is such that we should return 3, however, it does seem that every other shell exits with 127.

So unless anyone objects, I will apply your patch and adopt this new behaviour.

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#24 lore raw

Re: [PATCH 2/3] Revert "Eliminated global exerrno."

Quoted 6 lines · Herbert Xu

sh -c 'trap "exec nosuchfile" EXIT; exit 3'; echo $?

This makes dash exit with 127 instead of 3 as it does now.

AFAIK the wording of POSIX is such that we should return 3, however, it does seem that every other shell exits with 127.

On 'exec':

"If exec is specified with command, it shall replace the shell with
command without creating a new process." (1)

"If command is specified, exec shall not return to the shell; rather, the exit status of the process shall be the exit status of the program implementing command, which overlaid the shell. If command is not found, the exit status shall be 127. If command is found, but it is not an executable utility, the exit status shall be 126. If a redirection error occurs (see Consequences of Shell Errors ), the shell shall exit with a value in the range 1-125. Otherwise, exec
shall return a zero exit status." (2)

On 'exit':

"...
A trap on EXIT shall be executed before the shell terminates, except when the exit utility is invoked in that trap itself, in which case
the shell shall exit immediately." (3)

"The exit status [of the exit utility -jrn] shall be n, if specified. Otherwise, the value shall be the exit value of the last command executed, or zero if no command was executed. When exit is executed in a trap action, the last command is considered to be the command that
executed immediately preceding the trap action." (4)

The passage (3) seems to mean that "exit" triggers an EXIT trap (except, to avoid having to deal with recursion, when the "exit" occurs within an EXIT trap). Then the exec builtin is invoked, resulting (according to (2)) in an exit(127) without returning to the shell.

So I think the proposed behavior fits the spec.

Regards,
Jonathan

#25 lore raw

Re: [PATCH 2/3] Revert "Eliminated global exerrno."

In reply to Jonathan Nieder
Quoted 5 lines · Jonathan Nieder

Maybe the following would make sense on top?

-- 8< --
Subject: [EXCEPTIONS] Use EXEXIT in place of EXEXEC

Patch applied. Thanks a lot!

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#26 lore raw

PATCH 3/3 Eliminate global exerrno

In reply to Jonathan Nieder

Pass the exit status from the exec builtin through the exitstatus global. This just eliminates a global variable; no noticeable change is intended.

Based on v0.5.3~57 (Eliminated global exerrno, 2005-02-25).

Signed-off-by Jonathan Nieder <[email protected]>

---
That's the end of the series. Thanks for reading.

 src/error.h |    1 -
 src/eval.c  |    8 ++++----
 src/exec.c  |    3 ++-
 src/main.c  |    2 --
 4 files changed, 6 insertions(+), 8 deletions(-)
src/error.h +0 -1
index 1bb6a82..15ccbe3 100644
@@ -62,7 +62,6 @@ struct jmploc {
 
 extern struct jmploc *handler;
 extern int exception;
-extern int exerrno;	/* error for EXEXEC */
 
 /* exceptions */
 #define EXINT 0		/* SIGINT received */
src/eval.c +4 -4
index 5b8d36b..0406571 100644
@@ -850,15 +850,15 @@ bail:
 				listsetvar(varlist.list, VEXPORT);
 		}
 		if (evalbltin(cmdentry.u.cmd, argc, argv, flags)) {
-			int status;
 			int i;
 
 			i = exception;
 			if (i == EXEXIT)
 				goto raise;
-
-			status = (i == EXINT) ? SIGINT + 128 : 2;
-			exitstatus = status;
+			if (i == EXINT)
+				exitstatus = SIGINT + 128;
+			if (i == EXERROR)
+				exitstatus = 2;
 
 			if (i == EXINT || spclbltin > 0) {
 raise:
src/exec.c +2 -1
index 9525a16..42299ea 100644
@@ -86,7 +86,6 @@ struct tblentry {
 
 STATIC struct tblentry *cmdtable[CMDTABLESIZE];
 STATIC int builtinloc = -1;		/* index in path of %builtin, or -1 */
-int exerrno;				/* Last exec error */
 
 
 STATIC void tryexec(char *, char **, char **);

@@ -109,6 +108,7 @@ shellexec(char **argv, const char *path, int idx)
 	char *cmdname;
 	int e;
 	char **envp;
+	int exerrno;
 
 	envp = environment();
 	if (strchr(argv[0], '/') != NULL) {

@@ -138,6 +138,7 @@ shellexec(char **argv, const char *path, int idx)
 		exerrno = 2;
 		break;
 	}
+	exitstatus = exerrno;
 	TRACE(("shellexec failed for %s, errno %d, suppressint %d\n",
 		argv[0], e, suppressint ));
 	exerror(EXEXEC, "%s: %s", argv[0], errmsg(e, E_EXEC));
src/main.c +0 -2
index 5bc1a99..2bff956 100644
@@ -115,8 +115,6 @@ main(int argc, char **argv)
 		e = exception;
 		if (e == EXERROR)
 			exitstatus = 2;
-		else if (e == EXEXEC)
-			exitstatus = exerrno;
 
 		s = state;
 		if (e == EXEXIT || s == 0 || iflag == 0 || shlvl)
Signature · 1 line
1.7.2.3
#27 lore raw

Re: [PATCH 3/3] [EXCEPTIONS] Eliminate global exerrno

Quoted 7 lines · Jonathan Nieder

Pass the exit status from the exec builtin through the exitstatus global. This just eliminates a global variable; no noticeable change is intended.

Based on v0.5.3~57 (Eliminated global exerrno, 2005-02-25).

Signed-off-by Jonathan Nieder <[email protected]>

Heh, had you sent 2/3 and 3/3 as one patch I would've seen the problem and just applied it :)

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#28 lore raw

Re: [PATCH] [OPTIONS] Use exit status 127 when the script to run does not exist

In reply to Jonathan Nieder
Quoted 11 lines · Jonathan Nieder
Quoted 3 lines · Herbert Xu

This is wrong. You're creating exactly the problem that Jilles was talking about where dot(1) is returning 127. This code should be moved to procargs, which runs only for the "sh script" case.

Makes sense. How does this look?

It still ignores attempts to run "sh directory"; see the next patch for that.

Actually I was wrong. I had misunderstood Jilles' report and thought that the problem is with 127 coming from dotcmd. His problem is actually with dotcmd bailing out which your (or was it Gerrit's?) EXFILE patch already addressed.

However, instead of adding an ad-hoc exception for EXFILE, I'm simply going to move the exit status setting for EXERROR out of main.c and into error.c which should let you get what you want.

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#29 lore raw

Re: [PATCH] [OPTIONS] Use exit status 127 when the script to run does not exist

Quoted 4 lines · Herbert Xu

However, instead of adding an ad-hoc exception for EXFILE, I'm simply going to move the exit status setting for EXERROR out of main.c and into error.c which should let you get what you want.

Something like this:

commit a42317f10233cf2fcff033cc705fd14315188374

Author: Herbert Xu <[email protected]>
Date:   Sun Nov 28 20:22:00 2010 +0800

[ERROR] Allow the originator of EXERROR to set the exit status

Some errors have exit status values specified by POSIX and it is therefore desirable to be able to set the exit status at the EXERROR source rather than in main.c.

Signed-off-by Herbert Xu <[email protected]>
ChangeLog +1 -0
index 76cbeb4..e5479a8 100644
@@ -5,6 +5,7 @@
 2010-11-28  Herbert Xu <[email protected]>
 
 	* Fixed trap/return regression due to SKIPEVAL removal.
+	* Allow the originator of EXERROR to set the exit status.
 
 2010-10-18  Herbert Xu <[email protected]>
src/error.c +2 -0
index e304d3d..f1a358d 100644
@@ -163,6 +163,8 @@ sh_error(const char *msg, ...)
 {
 	va_list ap;
 
+	exitstatus = 2;
+
 	va_start(ap, msg);
 	exverror(EXERROR, msg, ap);
 	/* NOTREACHED */
src/eval.c +4 -2
index ea4afc6..013a8dd 100644
@@ -823,11 +823,13 @@ evalcommand(union node *cmd, int flags)
 	}
 
 	if (status) {
+bail:
+		exitstatus = status;
+
 		/* We have a redirection error. */
 		if (spclbltin > 0)
 			exraise(EXERROR);
-bail:
-		exitstatus = status;
+
 		goto out;
 	}
src/main.c +0 -2
index 2bff956..1735c67 100644
@@ -113,8 +113,6 @@ main(int argc, char **argv)
 		reset();
 
 		e = exception;
-		if (e == EXERROR)
-			exitstatus = 2;
 
 		s = state;
 		if (e == EXEXIT || s == 0 || iflag == 0 || shlvl)

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#30 lore raw

Re: [PATCH] [OPTIONS] Use exit status 127 when the script to run does not exist

Quoted 7 lines · Herbert Xu
Quoted 4 lines · Herbert Xu

However, instead of adding an ad-hoc exception for EXFILE, I'm simply going to move the exit status setting for EXERROR out of main.c and into error.c which should let you get what you want.

Something like this:

And here is the final patch that I applied for this:

commit 34f60a3781ea8f17f3d2a1a743a9aab9b31a296f

Author: Gerrit Pape <[email protected]>
Date:   Sun Nov 28 20:32:00 2010 +0800

[INPUT] Use exit status 127 when the script to run does not exist

This commit makes dash exit with return code 127 instead of 2 if started as non-interactive shell with a non-existent command_file specified as argument (or a directory), as documented in
http://www.opengroup.org/onlinepubs/009695399/utilities/sh.html#tag_04_128_14

The wrong exit code was reported by Clint Adams and Jari Aalto through
http://bugs.debian.org/548743
http://bugs.debian.org/548687

Signed-off-by Gerrit Pape <[email protected]>
Signed-off-by Herbert Xu <[email protected]>
ChangeLog +4 -0
index e5479a8..a18838c 100644
@@ -1,3 +1,7 @@
+2010-11-28  Gerrit Pape <[email protected]>
+
+	* Use exit status 127 when the script to run does not exist.
+
 2010-11-28  Philipp Weis <[email protected]>
 
 	* Document optional open parenthesis for case patterns.
src/input.c +3 -1
index e57ad76..bd3a9a2 100644
@@ -42,6 +42,7 @@
  * This file implements the input routines used by the parser.
  */
 
+#include "eval.h"
 #include "shell.h"
 #include "redir.h"
 #include "syntax.h"

@@ -408,7 +409,8 @@ setinputfile(const char *fname, int flags)
 	if ((fd = open(fname, O_RDONLY)) < 0) {
 		if (flags & INPUT_NOFILE_OK)
 			goto out;
-		sh_error("Can't open %s", fname);
+		exitstatus = 127;
+		exerror(EXERROR, "Can't open %s", fname);
 	}
 	if (fd < 10)
 		fd = savefd(fd, fd);

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt