| Message ID | 20181202145332.snc5z6tvisnulc7q@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v2] eval: Only restore exit status on exit/return | expand |
On 02/12/2018 14:53, Herbert Xu wrote: > On Sat, Dec 01, 2018 at 04:45:39PM +0800, Herbert Xu wrote: >> Hmm, I think this breaks the following case which used to work: >> >> dash -c 'trap "(:; exit) && echo BUG" EXIT; false' >> >> I know this makes no sense but almost every other shell does it >> this way. FWIW, I read this as you saying the behaviour is clearly wrong, and other shells doing it this way is the *only* reason you want to preserve it. This is contradicted by you in your reply to my other message, but I do not see how else to interpret this sentence. > Here is a new patch that fixes the reported issue for both signal > traps and the EXIT trap. This has the benefit of fixing one other test case, a small modification from one of Martijn Dekker's: $SHELL -c 'trap "set -o bad@option" INT; kill -s INT $$' && echo BUG Although the shell exited on the invalid option, it still reset the exit status to 0 because a trap handler was started, so BUG is printed. ksh is the only other shell I found to also print BUG. In this test case, execution never reaches clear_traps() since no subshell is started. Any changes made there therefore cannot have any effect. Some other approach is needed. Distinguishing between the different ways of exiting like you're doing here does manage to get this right. Still feels like it's more complicated than it should be though.
On 04/12/2018 23:57, Harald van Dijk wrote: > This has the benefit of fixing one other test case, a small modification > from one of Martijn Dekker's: > > $SHELL -c 'trap "set -o bad@option" INT; kill -s INT $$' && echo BUG Another test case, one that still fails: trap exit INT trap 'true; kill -s INT $$' EXIT false Here, inside the EXIT handler, "the command that executed immediately preceding the trap action" is `false`, but inside the INT handler, it's either `true` or `kill -s INT $$` (I think the latter, but it doesn't matter). dash treats it as if it were still `false`.
On 06/12/2018 21:35, Harald van Dijk wrote: > On 04/12/2018 23:57, Harald van Dijk wrote: >> This has the benefit of fixing one other test case, a small >> modification from one of Martijn Dekker's: >> >> $SHELL -c 'trap "set -o bad@option" INT; kill -s INT $$' && echo BUG > > Another test case, one that still fails: > > trap exit INT > trap 'true; kill -s INT $$' EXIT > false > > Here, inside the EXIT handler, "the command that executed immediately > preceding the trap action" is `false`, but inside the INT handler, it's > either `true` or `kill -s INT $$` (I think the latter, but it doesn't > matter). dash treats it as if it were still `false`. The attached patch fixes this. In short, it assumes that if the EXIT action raises an exception, exitstatus will have been set appropriately, and modifies exitcmd() to set it. It also simplifies dotrap() by removing the unnecessary special handling of recursive calls. It handles the following test cases, from this thread: exec 2>/dev/null $SHELL -c 'trap "(false) && echo BUG1" INT; kill -s INT $$' $SHELL -c 'trap "(false) && echo BUG2" EXIT' $SHELL -c 'trap "(false); echo \$?" EXIT' $SHELL -c 'trap "(true) || echo BUG3" INT; false; kill -s INT $$' $SHELL -c 'trap "(true) || echo BUG4" EXIT; false' $SHELL -c 'trap "(true); echo \$?" EXIT; false' $SHELL -c 'trap "(! :) && echo BUG5" EXIT' $SHELL -c 'trap "(false) && echo BUG6" EXIT' $SHELL -c 'trap "readonly foo=bar; (foo=baz) && echo BUG7" EXIT' $SHELL -c 'trap "(set -o bad@option) && echo BUG8" EXIT' $SHELL -c 'trap "set -o bad@option" EXIT' && echo BUG9 $SHELL -c 'trap "set -o bad@option" INT; kill -s INT $$' && echo BUG10 $SHELL -c 'trap exit INT; trap "true; kill -s INT $$" EXIT; false' || echo BUG11 It does not change the behaviour for these test cases, also from this thread: $SHELL -c 'trap "(trap \"echo exit\" EXIT; :)" EXIT' $SHELL -c 'trap "(:; exit) && echo exit" EXIT; false' It can be combined with the other patches to also change the behaviour of those two. --- a/src/eval.c +++ b/src/eval.c @@ -116,10 +116,7 @@ INCLUDE "eval.h" EXITRESET { evalskip = 0; loopnest = 0; - if (savestatus >= 0) { - exitstatus = savestatus; - savestatus = -1; - } + savestatus = -1; } #endif --- a/src/main.c +++ b/src/main.c @@ -348,7 +348,9 @@ exitcmd(int argc, char **argv) return 0; if (argc > 1) - savestatus = number(argv[1]); + exitstatus = number(argv[1]); + else if (savestatus >= 0) + exitstatus = savestatus; exraise(EXEXIT); /* NOTREACHED */ --- a/src/trap.c +++ b/src/trap.c @@ -320,17 +320,14 @@ void dotrap(void) char *p; char *q; int i; - int status, last_status; + int status; if (!pending_sig) return; status = savestatus; - last_status = status; - if (likely(status < 0)) { - status = exitstatus; - savestatus = status; - } + savestatus = exitstatus; + pending_sig = 0; barrier(); @@ -350,10 +347,10 @@ void dotrap(void) continue; evalstring(p, 0); if (evalskip != SKIPFUNC) - exitstatus = status; + exitstatus = savestatus; } - savestatus = last_status; + savestatus = status; } @@ -390,8 +387,10 @@ exitshell(void) savestatus = exitstatus; TRACE(("pid %d, exitshell(%d)\n", getpid(), savestatus)); - if (setjmp(loc.loc)) + if (setjmp(loc.loc)) { + savestatus = exitstatus; goto out; + } handler = &loc; if ((p = trap[0])) { trap[0] = NULL;
On Sat, Dec 08, 2018 at 03:45:11PM +0000, Harald van Dijk wrote: > > --- a/src/eval.c > +++ b/src/eval.c > @@ -116,10 +116,7 @@ INCLUDE "eval.h" > EXITRESET { > evalskip = 0; > loopnest = 0; > - if (savestatus >= 0) { > - exitstatus = savestatus; > - savestatus = -1; > - } > + savestatus = -1; > } > #endif The reason this is needed is to support a naked return. With your patch a naked return would either have to always return the saved status or the new status. Neither of which is what we want. Please refer to commit 70c16dd30d4cf824aa895e9f6c095fec741c65a8 Author: Herbert Xu <herbert@gondor.apana.org.au> Date: Mon Oct 6 21:51:26 2014 +0800 [BUILTIN] Return without arguments in a trap should use status outside traps Thanks,
On Thu, Dec 06, 2018 at 09:35:47PM +0000, Harald van Dijk wrote: > On 04/12/2018 23:57, Harald van Dijk wrote: > > This has the benefit of fixing one other test case, a small modification > > from one of Martijn Dekker's: > > > > $SHELL -c 'trap "set -o bad@option" INT; kill -s INT $$' && echo BUG > > Another test case, one that still fails: > > trap exit INT > trap 'true; kill -s INT $$' EXIT > false > > Here, inside the EXIT handler, "the command that executed immediately > preceding the trap action" is `false`, but inside the INT handler, it's > either `true` or `kill -s INT $$` (I think the latter, but it doesn't > matter). dash treats it as if it were still `false`. I think this makes sense. The EXIT trap trumps whatever happens inside it. FWIW ksh/mksh both do the same thing. Cheers,
On 14/12/2018 03:10, Herbert Xu wrote: > On Sat, Dec 08, 2018 at 03:45:11PM +0000, Harald van Dijk wrote: >> >> --- a/src/eval.c >> +++ b/src/eval.c >> @@ -116,10 +116,7 @@ INCLUDE "eval.h" >> EXITRESET { >> evalskip = 0; >> loopnest = 0; >> - if (savestatus >= 0) { >> - exitstatus = savestatus; >> - savestatus = -1; >> - } >> + savestatus = -1; >> } >> #endif > > The reason this is needed is to support a naked return. With > your patch a naked return would either have to always return the > saved status or the new status. Neither of which is what we > want. I actually saw the commit you referenced already and tested that with my patch before sending. This is how I tested it: Returns 1: f() { false; return } f Returns 0: f() { trap "false; return" USR1 kill -USR1 $$ } f The former picks up the status of the false command, the latter of the kill command. This works because although returncmd() only looks at exitstatus, so returns the wrong value, in the no-argument version it sets skip to SKIPFUNCDEF, causing dotrap() to restore the original exitstatus value, whereas in the version that does take arguments, skip is set to SKIPFUNC, causing dotrap() to leave exitstatus alone.
On 14/12/2018 08:02, Harald van Dijk wrote: > On 14/12/2018 03:10, Herbert Xu wrote: >> On Sat, Dec 08, 2018 at 03:45:11PM +0000, Harald van Dijk wrote: >>> >>> --- a/src/eval.c >>> +++ b/src/eval.c >>> @@ -116,10 +116,7 @@ INCLUDE "eval.h" >>> EXITRESET { >>> evalskip = 0; >>> loopnest = 0; >>> - if (savestatus >= 0) { >>> - exitstatus = savestatus; >>> - savestatus = -1; >>> - } >>> + savestatus = -1; >>> } >>> #endif >> >> The reason this is needed is to support a naked return. With >> your patch a naked return would either have to always return the >> saved status or the new status. Neither of which is what we >> want. > > I actually saw the commit you referenced already and tested that with my > patch before sending. This is how I tested it: > > Returns 1: > > f() { > false; return > } > f > > Returns 0: > > f() { > trap "false; return" USR1 > kill -USR1 $$ > } > f > > The former picks up the status of the false command, the latter of the > kill command. This works because although returncmd() only looks at > exitstatus, so returns the wrong value, in the no-argument version it > sets skip to SKIPFUNCDEF, causing dotrap() to restore the original > exitstatus value, whereas in the version that does take arguments, skip > is set to SKIPFUNC, causing dotrap() to leave exitstatus alone. Which is exactly how that particular commit was supposed to work in the first place, so perhaps it's simpler to put it as "in these tests, changes to exitreset() do not cause the test to break because exitreset() is never called at the wrong time". If there's some test that breaks, it would need to be one where the original exitstatus needs to be restored by exitreset(), *and* the original exitstatus is not already restored by dotrap(). The original exitstatus is not restored by dotrap() only if evalskip == SKIPFUNC (i.e. return with argument), in which case it shouldn't be restored, or it exits with an exception. And exitreset() is only called when control gets back to main() via an exception. So you'd need an example where an exception is raised after the return command successfully completes without arguments, yet before the function the return command is in (if any) actually returns, where that exception would then reach main(). I struggle to come up with such an example.
On Fri, Dec 14, 2018 at 08:02:04AM +0000, Harald van Dijk wrote: > On 14/12/2018 03:10, Herbert Xu wrote: > > > The reason this is needed is to support a naked return. With > > your patch a naked return would either have to always return the > > saved status or the new status. Neither of which is what we > > want. > > I actually saw the commit you referenced already and tested that with my > patch before sending. This is how I tested it: > > Returns 1: > > f() { > false; return > } > f That's not a naked return. A naked return is one used outside of a function. I know this is unspecified under POSIX, but for dash this has a specific meaning and that is the return is equivalent to exit. Cheers,
On 14/12/2018 09:16, Herbert Xu wrote: > On Fri, Dec 14, 2018 at 08:02:04AM +0000, Harald van Dijk wrote: >> On 14/12/2018 03:10, Herbert Xu wrote: >> >>> The reason this is needed is to support a naked return. With >>> your patch a naked return would either have to always return the >>> saved status or the new status. Neither of which is what we >>> want. >> >> I actually saw the commit you referenced already and tested that with my >> patch before sending. This is how I tested it: >> >> Returns 1: >> >> f() { >> false; return >> } >> f > > That's not a naked return. A naked return is one used outside of > a function. I know this is unspecified under POSIX, but for dash > this has a specific meaning and that is the return is equivalent > to exit. The commit you referred to specifically mentioned changing the behaviour to conform to POSIX requirements, and now it's actually about an extension? Still, that works as well, doesn't it? The control flow is basically the same so the logic in my other message applies. returncmd() doesn't use exceptions, so you get to dotrap() which already resets exitstatus if necessary. Test cases: Returns 0 (status of previous kill command): trap "false; return" USR1 kill -USR1 $$ Returns 1 (status specified in return command): trap "return 1" USR1 kill -USR1 $$ Kills itself with SIGINT (status of cat command) after printing "bye": trap "echo bye; return" INT cat; : <press Ctrl-C> Or is this about observing return's status in a trap action? Prints status=0 and exits with status 0: trap "echo status=\$?; false; exit" EXIT return Prints status=1 and exits with status 1: trap "echo status=\$?; exit" EXIT return 1
On Fri, Dec 14, 2018 at 09:37:09AM +0000, Harald van Dijk wrote: > > Still, that works as well, doesn't it? The control flow is basically the > same so the logic in my other message applies. returncmd() doesn't use > exceptions, so you get to dotrap() which already resets exitstatus if > necessary. returncmd itself doesn't jump up but it may be part of a subshell which would execute a jump to the top as part of the EV_EXIT optimisation. That's where you'd need to restore the exit status. Cheers,
On 14/12/2018 09:47, Herbert Xu wrote: > On Fri, Dec 14, 2018 at 09:37:09AM +0000, Harald van Dijk wrote: >> >> Still, that works as well, doesn't it? The control flow is basically the >> same so the logic in my other message applies. returncmd() doesn't use >> exceptions, so you get to dotrap() which already resets exitstatus if >> necessary. > > returncmd itself doesn't jump up but it may be part of a subshell > which would execute a jump to the top as part of the EV_EXIT > optimisation. That's where you'd need to restore the exit status. Ah, okay, so for something like trap 'false; (return); echo $?' EXIT you want to remember that it's executed as part of a trap action and print 0, not 1. I actually want that to print 1, so for me it's not a problem. However, my patch can be trivially modified to handle that: just have returncmd() check savestatus the same way exitcmd() does. I think that also allows merging SKIPFUNC and SKIPFUNCDEF, since you can then assume exitstatus was always set appropriately, so it allows for some further reduction in code.
On 14/12/2018 10:07, Harald van Dijk wrote: > On 14/12/2018 09:47, Herbert Xu wrote: >> On Fri, Dec 14, 2018 at 09:37:09AM +0000, Harald van Dijk wrote: >>> >>> Still, that works as well, doesn't it? The control flow is basically the >>> same so the logic in my other message applies. returncmd() doesn't use >>> exceptions, so you get to dotrap() which already resets exitstatus if >>> necessary. >> >> returncmd itself doesn't jump up but it may be part of a subshell >> which would execute a jump to the top as part of the EV_EXIT >> optimisation. That's where you'd need to restore the exit status. > > Ah, okay, so for something like > > trap 'false; (return); echo $?' EXIT > > you want to remember that it's executed as part of a trap action and > print 0, not 1. > > I actually want that to print 1, so for me it's not a problem. However, > my patch can be trivially modified to handle that: just have returncmd() > check savestatus the same way exitcmd() does. I think that also allows > merging SKIPFUNC and SKIPFUNCDEF, since you can then assume exitstatus > was always set appropriately, so it allows for some further reduction in > code. I think this is needed regardless to fix a bug as well: trap 'f() { false; return; }; f; echo $?' EXIT Other shells are evenly divided, but POSIX clearly requires this to print 0 (which is what bash/ksh/mksh/yash do): calling a function does not reset any trap information, so this return statement must be considered to execute in a trap action. It currently prints 1 (also seen in bosh/pdksh/posh/zsh). With my suggested changes, it prints 0.
diff --git a/src/error.h b/src/error.h index 9630b56..94e30a2 100644 --- a/src/error.h +++ b/src/error.h @@ -66,7 +66,8 @@ extern int exception; /* exceptions */ #define EXINT 0 /* SIGINT received */ #define EXERROR 1 /* a generic error */ -#define EXEXIT 4 /* exit the shell */ +#define EXEND 3 /* exit the shell */ +#define EXEXIT 4 /* exit the shell via exitcmd */ /* diff --git a/src/eval.c b/src/eval.c index 546ee1b..3cea2a3 100644 --- a/src/eval.c +++ b/src/eval.c @@ -114,12 +114,13 @@ STATIC const struct builtincmd bltin = { INCLUDE "eval.h" EXITRESET { - evalskip = 0; - loopnest = 0; if (savestatus >= 0) { - exitstatus = savestatus; + if (exception == EXEXIT || evalskip == SKIPFUNCDEF) + exitstatus = savestatus; savestatus = -1; } + evalskip = 0; + loopnest = 0; } #endif @@ -314,7 +315,7 @@ out: if (flags & EV_EXIT) { exexit: - exraise(EXEXIT); + exraise(EXEND); } return exitstatus; diff --git a/src/exec.c b/src/exec.c index 9d0215a..87354d4 100644 --- a/src/exec.c +++ b/src/exec.c @@ -143,7 +143,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(EXEXIT, "%s: %s", argv[0], errmsg(e, E_EXEC)); + exerror(EXEND, "%s: %s", argv[0], errmsg(e, E_EXEC)); /* NOTREACHED */ } diff --git a/src/main.c b/src/main.c index 6d53e00..6b3a090 100644 --- a/src/main.c +++ b/src/main.c @@ -111,7 +111,7 @@ main(int argc, char **argv) e = exception; s = state; - if (e == EXEXIT || s == 0 || iflag == 0 || shlvl) + if (e == EXEND || e == EXEXIT || s == 0 || iflag == 0 || shlvl) exitshell(); reset(); diff --git a/src/trap.c b/src/trap.c index ab0ecd4..64917fc 100644 --- a/src/trap.c +++ b/src/trap.c @@ -397,8 +397,10 @@ exitshell(void) trap[0] = NULL; evalskip = 0; evalstring(p, 0); + evalskip = SKIPFUNCDEF; } out: + exitreset(); /* * Disable job control so that whoever had the foreground before we * started can get it back. @@ -406,7 +408,7 @@ out: if (likely(!setjmp(loc.loc))) setjobctl(0); flushall(); - _exit(savestatus); + _exit(exitstatus); /* NOTREACHED */ }