| Message ID | 20190303135750.hztabicgnifqarut@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v2] eval: Reset handler when entering a subshell | expand |
On 03/03/2019 13:57, Herbert Xu wrote: > On Sat, Mar 02, 2019 at 01:28:44PM +0000, Harald van Dijk wrote: >> >> That is *a* real bug here, not *the* real bug. This leaves the buggy >> behaviour of the "command" built-in intact in the test case I included in >> the message you replied to. > > I don't quite understand. Could you explain what is still buggy > after the following patch? I gave this example in my previous message: command . /dev/stdin <<EOF set -o invalid echo a EOF echo b Ordinarily, "set -o invalid" is a "special built-in utility error", for which the consequence is that a non-interactive shell "shall exit". If it weren't a special built-in utility, it would be an "other utility (not a special built-in) error", for which a non-interactive shell "shall not exit". The effect of the command built-in is that "if the command_name is the same as the name of one of the special built-in utilities, the special properties in the enumerated list at the beginning of Special Built-In Utilities shall not occur." This is ambiguous as to whether it is just about the special properties associated with the . command, or whether it includes those associated with the set command called indirectly, but it must be one or the other. If it includes the set command, then the shell shall not exit, and the correct output is a followed by b. If it does not include the set command, then the shell shall exit, and the correct output is nothing. dash outputs b, which is wrong in either case. Cheers, Harald van Dijk
On Sun, Mar 03, 2019 at 04:43:09PM +0000, Harald van Dijk wrote: > On 03/03/2019 13:57, Herbert Xu wrote: > > On Sat, Mar 02, 2019 at 01:28:44PM +0000, Harald van Dijk wrote: > > > That is *a* real bug here, not *the* real bug. This leaves the buggy > > > behaviour of the "command" built-in intact in the test case I included in > > > the message you replied to. > > I don't quite understand. Could you explain what is still buggy > > after the following patch? > I gave this example in my previous message: > command . /dev/stdin <<EOF > set -o invalid > echo a > EOF > echo b > Ordinarily, "set -o invalid" is a "special built-in utility error", for > which the consequence is that a non-interactive shell "shall exit". If it > weren't a special built-in utility, it would be an "other utility (not a > special built-in) error", for which a non-interactive shell "shall not > exit". > The effect of the command built-in is that "if the command_name is the same > as the name of one of the special built-in utilities, the special properties > in the enumerated list at the beginning of Special Built-In Utilities shall > not occur." This is ambiguous as to whether it is just about the special > properties associated with the . command, or whether it includes those > associated with the set command called indirectly, but it must be one or the > other. If it includes the set command, then the shell shall not exit, and > the correct output is a followed by b. If it does not include the set > command, then the shell shall exit, and the correct output is nothing. dash > outputs b, which is wrong in either case. The way I interpret this is that any error while parsing or executing the . or eval command(s) is a "special built-in utility error" which can be "caught" using "command". Therefore, the correct output is an error message about the invalid option (to stderr) followed by b. This is what happens in FreeBSD sh (for quite a few years), ksh93 (20120801_2) and mksh (56c). Given that the text is somewhat ambiguous, I think bash's POSIX mode behaviour of exiting may also be valid. I don't like disabling the special properties of the inner set command since this creates a different kind of script for 'command .' versus '.'. Normally (though not in #!/bin/bash scripts), if I write something like 'set -o pipefail', I can safely assume that it will take if the shell continues to the next command; if the shell does not support the option, it exits. The fairly old dash v0.5.9.1 I tried works even differently, writing an error message (to stderr) followed by a. So it first continues after the error and then aborts when '.' ends. This seems clearly broken (but I think things have changed since back then).
On 03/03/2019 17:39, Jilles Tjoelker wrote: > On Sun, Mar 03, 2019 at 04:43:09PM +0000, Harald van Dijk wrote: >> The effect of the command built-in is that "if the command_name is the same >> as the name of one of the special built-in utilities, the special properties >> in the enumerated list at the beginning of Special Built-In Utilities shall >> not occur." This is ambiguous as to whether it is just about the special >> properties associated with the . command, or whether it includes those >> associated with the set command called indirectly, but it must be one or the >> other. If it includes the set command, then the shell shall not exit, and >> the correct output is a followed by b. If it does not include the set >> command, then the shell shall exit, and the correct output is nothing. dash >> outputs b, which is wrong in either case. > > The way I interpret this is that any error while parsing or executing > the . or eval command(s) is a "special built-in utility error" which can > be "caught" using "command". Therefore, the correct output is an error > message about the invalid option (to stderr) followed by b. This is what > happens in FreeBSD sh (for quite a few years), ksh93 (20120801_2) and > mksh (56c). That's how dash implements it too, but it plainly doesn't make sense based on POSIX's requirements. If the effect of "shall exit" does not occur, the result isn't "shall start exiting and then abort the exit", it's "shall not exit". In order to allow dash/FreeBSD sh's interpretation, POSIX would need to say something like "the special properties [...] shall be suppressed" rather than "[...] shall not occur". Cheers, Harald van Dijk
diff --git a/src/eval.c b/src/eval.c index 1aad31a..6ee2e1a 100644 --- a/src/eval.c +++ b/src/eval.c @@ -41,6 +41,7 @@ * Evaluate a command. */ +#include "main.h" #include "shell.h" #include "nodes.h" #include "syntax.h" @@ -492,6 +493,7 @@ evalsubshell(union node *n, int flags) if (backgnd) flags &=~ EV_TESTED; nofork: + reset_handler(); redirect(n->nredir.redirect, 0); evaltreenr(n->nredir.n, flags); /* never returns */ @@ -574,6 +576,7 @@ evalpipe(union node *n, int flags) } } if (forkshell(jp, lp->n, n->npipe.backgnd) == 0) { + reset_handler(); INTON; if (pip[1] >= 0) { close(pip[0]); @@ -630,6 +633,7 @@ evalbackcmd(union node *n, struct backcmd *result) sh_error("Pipe call failed"); jp = makejob(n, 1); if (forkshell(jp, n, FORK_NOJOB) == 0) { + reset_handler(); FORCEINTON; close(pip[0]); if (pip[1] != 1) { diff --git a/src/main.c b/src/main.c index 6b3a090..b2712cb 100644 --- a/src/main.c +++ b/src/main.c @@ -71,6 +71,7 @@ int *dash_errno; short profile_buf[16384]; extern int etext(); #endif +static struct jmploc main_handler; STATIC void read_profile(const char *); STATIC char *find_dot_file(char *); @@ -90,7 +91,6 @@ main(int argc, char **argv) { char *shinit; volatile int state; - struct jmploc jmploc; struct stackmark smark; int login; @@ -102,7 +102,7 @@ main(int argc, char **argv) monitor(4, etext, profile_buf, sizeof profile_buf, 50); #endif state = 0; - if (unlikely(setjmp(jmploc.loc))) { + if (unlikely(setjmp(main_handler.loc))) { int e; int s; @@ -137,7 +137,7 @@ main(int argc, char **argv) else goto state4; } - handler = &jmploc; + handler = &main_handler; #ifdef DEBUG opentrace(); trputs("Shell args: "); trargs(argv); @@ -353,3 +353,8 @@ exitcmd(int argc, char **argv) exraise(EXEXIT); /* NOTREACHED */ } + +void reset_handler(void) +{ + handler = &main_handler; +} diff --git a/src/main.h b/src/main.h index 19e4983..51f1604 100644 --- a/src/main.h +++ b/src/main.h @@ -52,3 +52,4 @@ extern int *dash_errno; void readcmdfile(char *); int dotcmd(int, char **); int exitcmd(int, char **); +void reset_handler(void);