clear_traps: reset savestatus

Commit Message

Martijn Dekker Nov. 29, 2018, 3:45 p.m. UTC

Op 27-11-18 om 17:24 schreef Martijn Dekker:
> Big bad bug: it appears that subshells always return status 0 in traps.

As posted elsewhere, looks like the problem is simply that savestatus 
("/* exit status of last command outside traps */") isn't reset to -1 
upon resetting traps when forking a subshell.

Reposting patch for Patchwork purposes:


Thanks,

- M.

Comments

Harald van Dijk Nov. 29, 2018, 7:56 p.m. UTC | #1

On 29/11/2018 15:45, Martijn Dekker wrote:
> Op 27-11-18 om 17:24 schreef Martijn Dekker:
>> Big bad bug: it appears that subshells always return status 0 in traps.
> 
> As posted elsewhere, looks like the problem is simply that savestatus 
> ("/* exit status of last command outside traps */") isn't reset to -1 
> upon resetting traps when forking a subshell.

That's part of it, but not the whole story. Herbert Xu's comment about 
exitshell() was right, that is still a problem. A testcase for this is

   trap '(true) || echo bug' EXIT

Your patch can be extended to handle that though, by skipping 
exitshell()'s handler if savestatus got reset to -1.

Cheers,
Harald van Dijk
diff --git a/src/trap.c b/src/trap.c
index ab0ecd46..eef44f1d 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -169,6 +169,7 @@ clear_traps(void)
 	}
 	trapcnt = 0;
 	INTON;
+	savestatus = -1;
 }
 
 
@@ -387,11 +388,18 @@ exitshell(void)
 {
 	struct jmploc loc;
 	char *p;
+	struct jmploc *volatile savehandler;
 
 	savestatus = exitstatus;
 	TRACE(("pid %d, exitshell(%d)\n", getpid(), savestatus));
-	if (setjmp(loc.loc))
+	savehandler = handler;
+	if (setjmp(loc.loc)) {
+		if (savestatus == -1) {
+			handler = savehandler;
+			longjmp(handler->loc, 1);
+		}
 		goto out;
+	}
 	handler = &loc;
 	if ((p = trap[0])) {
 		trap[0] = NULL;

Martijn Dekker Nov. 29, 2018, 8:28 p.m. UTC | #2

Op 29-11-18 om 20:56 schreef Harald van Dijk:
> That's part of it, but not the whole story. Herbert Xu's comment about 
> exitshell() was right, that is still a problem. A testcase for this is
> 
>    trap '(true) || echo bug' EXIT

Yes. Thanks. I hadn't thought about that.

The test case above is not quite complete. It needs to make sure the 
exit status is non-zero before executing the trap:

$ src/dash -c "trap '(true) || echo bug' EXIT; false"
bug

- Martijn

Harald van Dijk Nov. 29, 2018, 8:39 p.m. UTC | #3

On 29/11/2018 20:28, Martijn Dekker wrote:
> Op 29-11-18 om 20:56 schreef Harald van Dijk:
>> That's part of it, but not the whole story. Herbert Xu's comment about 
>> exitshell() was right, that is still a problem. A testcase for this is
>>
>>    trap '(true) || echo bug' EXIT
> 
> Yes. Thanks. I hadn't thought about that.
> 
> The test case above is not quite complete. It needs to make sure the 
> exit status is non-zero before executing the trap:
> 
> $ src/dash -c "trap '(true) || echo bug' EXIT; false"
> bug

I had intended it as a testcase for dash with your change applied, in 
which case it fails even without making sure of that since you hit 
_exit(savestatus) with savestatus reset to -1, but I like that it's so 
easy to modify to also fail on unpatched dash, so thanks for that.

By the way, my change has an unintended but possibly acceptable side effect:

   trap '(trap "echo exit" EXIT; :)' EXIT

This prints nothing with current dash, but prints "exit" with my change. 
It also prints "exit" in ksh, mksh, posh, and bosh.

> - Martijn
>

Martijn Dekker Nov. 29, 2018, 11:21 p.m. UTC | #4

Op 29-11-18 om 21:39 schreef Harald van Dijk:
> By the way, my change has an unintended but possibly acceptable side 
> effect:
> 
>    trap '(trap "echo exit" EXIT; :)' EXIT
> 
> This prints nothing with current dash, but prints "exit" with my change. 
> It also prints "exit" in ksh, mksh, posh, and bosh.

IMHO, that's not a side effect, but evidence that the bug was properly 
fixed. A subshell is defined as a duplicate of the original shell 
process except with its traps reset[*], so setting a trap should work as 
normal within it.

It doesn't work in bash, yash or zsh, which I believe is a bug that I 
should report to them.

- M.

[*] "A subshell environment shall be created as a duplicate of the shell 
environment, except that signal traps that are not being ignored shall 
be set to the default action."
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_12
(I'm going to assume that "signal traps" here is meant to include the 
EXIT trap, because not clearing it for subshells would be insane.)

Herbert Xu Dec. 2, 2018, 3:06 p.m. UTC | #5

Harald van Dijk <[e-mail]> wrote:
>
> I had intended it as a testcase for dash with your change applied, in 
> which case it fails even without making sure of that since you hit 
> _exit(savestatus) with savestatus reset to -1, but I like that it's so 
> easy to modify to also fail on unpatched dash, so thanks for that.
> 
> By the way, my change has an unintended but possibly acceptable side effect:
> 
>   trap '(trap "echo exit" EXIT; :)' EXIT
> 
> This prints nothing with current dash, but prints "exit" with my change. 
> It also prints "exit" in ksh, mksh, posh, and bosh.

I'm happy to allow exit traps to be processed here.  But we should
be able to achieve the same result without jumping back to main,
by simply jumping back to the start of exitshell instead.

Thanks,

Patch

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;
  }