diff mbox series

eval: Only restore exit status on exit/return

Message ID 20181129113353.kx5ccakbvwnk6ptr@gondor.apana.org.au (mailing list archive)
State Superseded
Delegated to: Herbert Xu
Headers show
Series eval: Only restore exit status on exit/return | expand

Commit Message

Herbert Xu Nov. 29, 2018, 11:33 a.m. UTC
Martijn Dekker <martijn@inlv.org> wrote:
> Big bad bug: it appears that subshells always return status 0 in traps.
> 
> Bug found in dash 0.5.9 and later.
> 
> $ src/dash -c 'trap "(false) && echo BUG" INT; kill -s INT $$'
> BUG
> $ src/dash -c 'trap "(false) && echo BUG" EXIT'
> BUG
> $ src/dash -c 'trap "(false); echo \$?" EXIT'
> 0
> 
> Workaround: if an explicit 'exit' is given, it works as expected.
> 
> $ src/dash -c 'trap "(false; exit \$?); echo \$?" EXIT'
> 1

Thanks for the report.  This patch should fix the problem:

---8<---
We unconditionally restore the saved status in exitreset, which
is incorrect as we only want to do it for exitcmd and returncmd.
This patch fixes the problem by introducing EXEND.

Reported-by: Martijn Dekker <martijn@inlv.org>
Fixes: da30b4b78769 ("[BUILTIN] Exit without arguments in a trap...")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Comments

Martijn Dekker Nov. 29, 2018, 2:27 p.m. UTC | #1
Op 29-11-18 om 12:33 schreef Herbert Xu:
> Thanks for the report.  This patch should fix the problem:

Doesn't work for me, in fact it seems to make no difference.

Here are a few more test cases.

(trap '(! :) && echo BUG1' EXIT)
(trap '(false) && echo BUG2' EXIT)
(trap 'readonly foo=bar; (foo=baz) && echo BUG3' EXIT)
(trap '(set -o bad@option) && echo BUG4' EXIT)

- M.
Herbert Xu Nov. 29, 2018, 2:48 p.m. UTC | #2
On Thu, Nov 29, 2018 at 03:27:31PM +0100, Martijn Dekker wrote:
> Op 29-11-18 om 12:33 schreef Herbert Xu:
> > Thanks for the report.  This patch should fix the problem:
> 
> Doesn't work for me, in fact it seems to make no difference.
> 
> Here are a few more test cases.
> 
> (trap '(! :) && echo BUG1' EXIT)
> (trap '(false) && echo BUG2' EXIT)
> (trap 'readonly foo=bar; (foo=baz) && echo BUG3' EXIT)
> (trap '(set -o bad@option) && echo BUG4' EXIT)

Oops, I only tested the dotrap case, and not exitshell.

The problem is that in evalsubshell we end up in exitshell again
which restores the old exit status.  So we need to come up with
a way to differentiate the exitshell from the original shell vs.
a subshell.

Thanks,
Martijn Dekker Nov. 29, 2018, 3:02 p.m. UTC | #3
Op 29-11-18 om 15:48 schreef Herbert Xu:
> The problem is that in evalsubshell we end up in exitshell again
> which restores the old exit status.  So we need to come up with
> a way to differentiate the exitshell from the original shell vs.
> a subshell.

Isn't it much simpler than that? Upon forking a subshell, traps are 
reset, so it would make sense that any flag that says "we are currently 
in the process of executing a trap" is also reset.

Which, from a look at the source code, seems to be the -1 value of 
savestatus. So this oneliner fixes it for me:

diff --git a/src/trap.c b/src/trap.c
index ab0ecd4..7740955 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -168,6 +168,7 @@ clear_traps(void)
  		}
  	}
  	trapcnt = 0;
+	savestatus = -1;
  	INTON;
  }


Thanks,

- M.
Herbert Xu Dec. 1, 2018, 8:45 a.m. UTC | #4
On Thu, Nov 29, 2018 at 04:02:32PM +0100, Martijn Dekker wrote:
>
> Isn't it much simpler than that? Upon forking a subshell, traps are reset,
> so it would make sense that any flag that says "we are currently in the
> process of executing a trap" is also reset.
> 
> Which, from a look at the source code, seems to be the -1 value of
> savestatus. So this oneliner fixes it for me:
> 
> diff --git a/src/trap.c b/src/trap.c
> index ab0ecd4..7740955 100644
> --- a/src/trap.c
> +++ b/src/trap.c
> @@ -168,6 +168,7 @@ clear_traps(void)
>  		}
>  	}
>  	trapcnt = 0;
> +	savestatus = -1;
>  	INTON;
>  }

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.

Thanks,
Harald van Dijk Dec. 1, 2018, 10:39 a.m. UTC | #5
On 01/12/2018 08:45, 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.

"Almost every other shell"? In the shells I tested, other than dash, 
it's evenly divided: four print nothing (bash, ksh, mksh, yash), four 
print BUG (bosh, pdksh, posh, zsh). Which ones are you testing?

To me, the standard is unclear on what the output should be: if at the 
time exit is executed, traps are reset, can what's being executed still 
be considered a trap action?

It's a nice test case whatever the correct result.
Martijn Dekker Dec. 1, 2018, 11:43 a.m. UTC | #6
Op 01-12-18 om 11:39 schreef Harald van Dijk:
> On 01/12/2018 08:45, 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.
> 
> "Almost every other shell"? In the shells I tested, other than dash, 
> it's evenly divided: four print nothing (bash, ksh, mksh, yash), four 
> print BUG (bosh, pdksh, posh, zsh). Which ones are you testing?
> 
> To me, the standard is unclear on what the output should be: if at the 
> time exit is executed, traps are reset, can what's being executed still 
> be considered a trap action?

The word 'reset' is not ambiguous. Resetting traps in a subshell means 
that subshells forget all about the parent shell's traps, so the fact 
that the parent shell that forked that subshell happens to be executing 
a trap action is irrelevant to the behaviour of the subshell. Thus, the 
'BUG' output is actually the correct one here.

As Herbert acknowledged, the alternative makes no sense. I think the 
standard *is* clear. But even (or especially) if the standard is 
unclear, blatantly illogical behaviour should be considered a bug.

> It's a nice test case whatever the correct result.

Yes.

- M.
Herbert Xu Dec. 2, 2018, 3:04 p.m. UTC | #7
Harald van Dijk <harald@gigawatt.nl> wrote:
>
> "Almost every other shell"? In the shells I tested, other than dash, 
> it's evenly divided: four print nothing (bash, ksh, mksh, yash), four 
> print BUG (bosh, pdksh, posh, zsh). Which ones are you testing?
> 
> To me, the standard is unclear on what the output should be: if at the 
> time exit is executed, traps are reset, can what's being executed still 
> be considered a trap action?
> 
> It's a nice test case whatever the correct result.

I don't disagree with your reasoning.  However, I'm not going
to change existing behaviour unless it is clearly wrong.

Cheers,
Harald van Dijk Dec. 8, 2018, 6:01 p.m. UTC | #8
On 29/11/2018 15:02, Martijn Dekker wrote:
> Op 29-11-18 om 15:48 schreef Herbert Xu:
>> The problem is that in evalsubshell we end up in exitshell again
>> which restores the old exit status.  So we need to come up with
>> a way to differentiate the exitshell from the original shell vs.
>> a subshell.
> 
> Isn't it much simpler than that? Upon forking a subshell, traps are 
> reset, so it would make sense that any flag that says "we are currently 
> in the process of executing a trap" is also reset.

There's more than just trap status that should be reset in subshells.

   for x in a b
   do
     (
       for y in c d
       do
         break 2
       done
       echo $x
     )
   done

I expect this to print "a" and "b", possibly accompanied by some error 
messages from break. Instead, dash (like ksh, bosh and zsh) remembers 
there's an outer loop and tries to break out of that from within the 
subshell, which of course doesn't work. It doesn't print anything.

It looks like the child process should call exitreset(), which fixes 
this and will also set savestatus to -1 -- except for exitreset()'s 
(correct) behaviour of also closing redirections.
diff mbox series

Patch

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