[BUG] Sticky-sh POSIX_TRAPS are function-local

[BUG] Sticky-sh POSIX_TRAPS are function-local

[Martijn Dekker]

The POSIX_TRAPS option for the EXIT trap does not work if both of the
following conditions apply:

- zsh was launched as zsh, not sh
- the EXIT trap is set from a shell function with sticky sh emulation

Script showing the bug (on zsh-5.2-142-gac5d83b):

	#! /bin/zsh
	echo start program
	emulate sh -c 'testfn() {
	        echo start function
		set -o | grep posixtraps
		trap "echo EXIT TRAP TRIGGERED" EXIT
		echo end function
	}'
	testfn
	echo program continuing
	echo end of program

Actual output:

	start program
	start function
	noposixtraps          off
	end function
	EXIT TRAP TRIGGERED
	program continuing
	end of program

Expected output:

	start program
	start function
	noposixtraps          off
	end function
	program continuing
	end of program
	EXIT TRAP TRIGGERED

Note how the EXIT trap is triggered upon exit from the function that set
it, and not upon exit from the shell, even though 'emulate sh' should
activate POSIX_TRAPS so the function should have set a global trap.

Thanks,

- M.

(How I found this bug: my cross-platform shell library, modernish
<https://github.com/modernish/modernish>, can be used with native zsh
with the command
   emulate sh -c '. modernish'
Sticky sh emulation is fantastic: the library's features mix in
seamlessly with native zsh, even the crazy alias-based ones such as the
new loop constructs. The only thing that is *not* working are the
functions that set EXIT traps, i.e.. 'pushtrap' -- stack-based traps, so
modernish modules can set their own clean-up traps without interfering
with others. On native zsh with modernish in sticky sh emulation, those
get executed instantly upon setting them.)

Re: [BUG] Sticky-sh POSIX_TRAPS are function-local

[Peter Stephenson]

On Mon, 15 Feb 2016 06:11:13 +0100
Martijn Dekker <martijn@inlv.org> wrote:
> The POSIX_TRAPS option for the EXIT trap does not work if both of the
> following conditions apply:

Oh, you mean the POSIXness of the trap is not sticky.  It should be
marked as to be run at the end of the programme based on the emulation
when it was started.  I don't think we've ever claimed it would be,
but it would be clearly be useful to change as the intention in
POSIX emulation is unambiguous.

I think that's probably fixable with a bit of flaggery, but we'll
probably need to compromise and agree that if someone sets an EXIT trap
once the emulation is left, which therefore picks up non-POSIX behaviour
(if that's how the shell was started), it will wipe out the trap
completely.  Otherwise we're in a weird and wonderful world of multiple
parallel EXIT traps.  Presumably that's no big issue, since even in
POSIX mode a new EXIT trap wipes out the one you've just set.

pws

Re: [BUG] Sticky-sh POSIX_TRAPS are function-local

[Peter Stephenson]

This makes the POSIX behaviour of the EXIT trap sticky based on the
setting of POSIX_TRAPS at the point where the EXIT trap was set.  It's
hard to see how this can not be what the script writer intended.  In
other words, it doesn't care about the stickiness of the emulator
surrounding it.

I've pointed out knock-on effects in the README; I suspect anyone
relying on those side effects was on a hiding to nothing anyway.

diff --git a/README b/README
index 8b8ab57..d5343db 100644
--- a/README
+++ b/README
@@ -65,6 +65,20 @@ remainder of the value discarded.  This could lead to different behaviour
 if the argument contains non-numeric characters, or if the argument has
 leading zeroes and the OCTAL_ZEROES option is set.
 
+3) For some time the shell has had a POSIX_TRAPS option which determines
+whether the EXIT trap has POSIX behaviour (the trap is only run at shell
+exit) or traditional zsh behaviour (the trap is run once and discarded
+when the enclosing fuction or shell exits, whichever happens first).
+The use of this option has now been made "sticky" on the EXIT trap ---
+in other words, the setting of the option at the point where the trap is
+set now determines whether the trap has POSIX or traditional zsh
+behaviour.  This means that changing the option after the trap was set
+no longer has any effect.
+
+Other aspects of EXIT trap handling have not changed --- there is still
+only one EXIT trap at any point in a programme, so it is not generally
+useful to combine POSIX and non-POSIX behaviour in the same script.
+
 Incompatibilities between 5.0.8 and 5.2
 ---------------------------------------
 
diff --git a/Src/signals.c b/Src/signals.c
index aa0b5aa..32452ae 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -55,6 +55,15 @@ mod_export Eprog siglists[VSIGCOUNT];
 /**/
 mod_export int nsigtrapped;
 
+/*
+ * Flag that exit trap has been set in POSIX mode.
+ * The setter's expectation is therefore that it is run
+ * on programme exit, not function exit.
+ */
+
+/**/
+static int exit_trap_posix;
+
 /* Variables used by signal queueing */
 
 /**/
@@ -755,7 +764,7 @@ killjb(Job jn, int sig)
  * at once, so just use a linked list.
  */
 struct savetrap {
-    int sig, flags, local;
+    int sig, flags, local, posix;
     void *list;
 };
 
@@ -774,6 +783,7 @@ dosavetrap(int sig, int level)
     st = (struct savetrap *)zalloc(sizeof(*st));
     st->sig = sig;
     st->local = level;
+    st->posix = (sig == SIGEXIT) ? exit_trap_posix : 0;
     if ((st->flags = sigtrapped[sig]) & ZSIG_FUNC) {
 	/*
 	 * Get the old function: this assumes we haven't added
@@ -873,6 +883,10 @@ settrap(int sig, Eprog l, int flags)
      * works just the same.
      */
     sigtrapped[sig] |= (locallevel << ZSIG_SHIFT) | flags;
+    if (sig == SIGEXIT) {
+	/* Make POSIX behaviour of EXIT trap sticky */
+	exit_trap_posix = isset(POSIXTRAPS);
+    }
     unqueue_signals();
     return 0;
 }
@@ -908,7 +922,7 @@ removetrap(int sig)
      * already one at the current locallevel we just overwrite it.
      */
     if (!dontsavetrap &&
-	(sig == SIGEXIT ? !isset(POSIXTRAPS) : isset(LOCALTRAPS)) &&
+	(sig == SIGEXIT ? !exit_trap_posix : isset(LOCALTRAPS)) &&
 	locallevel &&
 	(!trapped || locallevel > (sigtrapped[sig] >> ZSIG_SHIFT)))
 	dosavetrap(sig, locallevel);
@@ -935,6 +949,8 @@ removetrap(int sig)
 #endif
              sig != SIGCHLD)
         signal_default(sig);
+    if (sig == SIGEXIT)
+	exit_trap_posix = 0;
 
     /*
      * At this point we free the appropriate structs.  If we don't
@@ -978,7 +994,7 @@ starttrapscope(void)
      * so give it the next higher one. dosavetrap() is called
      * automatically where necessary.
      */
-    if (sigtrapped[SIGEXIT] && !isset(POSIXTRAPS)) {
+    if (sigtrapped[SIGEXIT] && !exit_trap_posix) {
 	locallevel++;
 	unsettrap(SIGEXIT);
 	locallevel--;
@@ -1005,7 +1021,7 @@ endtrapscope(void)
      * Don't do this inside another trap.
      */
     if (!intrap &&
-	!isset(POSIXTRAPS) && (exittr = sigtrapped[SIGEXIT])) {
+	!exit_trap_posix && (exittr = sigtrapped[SIGEXIT])) {
 	if (exittr & ZSIG_FUNC) {
 	    exitfn = removehashnode(shfunctab, "TRAPEXIT");
 	} else {
@@ -1031,7 +1047,9 @@ endtrapscope(void)
 		if (st->flags & ZSIG_FUNC)
 		    settrap(sig, NULL, ZSIG_FUNC);
 		else
-		    settrap(sig, (Eprog) st->list, 0);
+			settrap(sig, (Eprog) st->list, 0);
+		if (sig == SIGEXIT)
+		    exit_trap_posix = st->posix;
 		dontsavetrap--;
 		/*
 		 * counting of nsigtrapped should presumably be handled
@@ -1042,16 +1060,26 @@ endtrapscope(void)
 		if ((sigtrapped[sig] = st->flags) & ZSIG_FUNC)
 		    shfunctab->addnode(shfunctab, ((Shfunc)st->list)->node.nam,
 				       (Shfunc) st->list);
-	    } else if (sigtrapped[sig])
-		unsettrap(sig);
+	    } else if (sigtrapped[sig]) {
+		/*
+		 * Don't restore the old state if someone has set a
+		 * POSIX-style exit trap --- allow this to propagate.
+		 */
+		if (sig != SIGEXIT || !exit_trap_posix)
+		    unsettrap(sig);
+	    }
 
 	    zfree(st, sizeof(*st));
 	}
     }
 
     if (exittr) {
-	if (!isset(POSIXTRAPS))
-	    dotrapargs(SIGEXIT, &exittr, exitfn);
+	/*
+	 * We already made sure this wasn't set as a POSIX exit trap.
+	 * We respect the user's intention when the trap in question
+	 * was set.
+	 */
+	dotrapargs(SIGEXIT, &exittr, exitfn);
 	if (exittr & ZSIG_FUNC)
 	    shfunctab->freenode((HashNode)exitfn);
 	else
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index 4b2843a..d8183a4 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -399,6 +399,26 @@
 >}
 >No, really exited
 
+   (cd ..; $ZTST_exe -fc 'unsetopt posixtraps;
+   echo start program
+   emulate sh -c '\''testfn() {
+     echo start function
+     set -o | grep posixtraps
+     trap "echo EXIT TRAP TRIGGERED" EXIT
+     echo end function
+   }'\''
+   testfn
+   echo program continuing
+   echo end of program')
+0:POSIX_TRAPS effect on EXIT trap is sticky
+>start program
+>start function
+>noposixtraps          off
+>end function
+>program continuing
+>end of program
+>EXIT TRAP TRIGGERED
+
    (set -e
     printf "a\nb\n" | while read line
     do

Re: [BUG] Sticky-sh POSIX_TRAPS are function-local

[Martijn Dekker]

Peter Stephenson schreef op 16-02-16 om 10:57:
> On Mon, 15 Feb 2016 06:11:13 +0100
> Martijn Dekker <martijn@inlv.org> wrote:
>> The POSIX_TRAPS option for the EXIT trap does not work if both of the
>> following conditions apply:
> 
> Oh, you mean the POSIXness of the trap is not sticky.  It should be
> marked as to be run at the end of the programme based on the emulation
> when it was started.  I don't think we've ever claimed it would be,
> but it would be clearly be useful to change as the intention in
> POSIX emulation is unambiguous.

That's a much better way of putting it.

> I think that's probably fixable with a bit of flaggery, but we'll
> probably need to compromise and agree that if someone sets an EXIT trap
> once the emulation is left, which therefore picks up non-POSIX behaviour
> (if that's how the shell was started), it will wipe out the trap
> completely.  Otherwise we're in a weird and wonderful world of multiple
> parallel EXIT traps.  Presumably that's no big issue, since even in
> POSIX mode a new EXIT trap wipes out the one you've just set.

However, since POSIX traps are inherently global, the expectation that
POSIX traps wipe out previous traps only applies to global traps. Could
you make an exception for function-local traps?

IMO, no one would expect a function-local trap to wipe out a global
trap, whether or not the global trap was set in POSIX mode. For
sticky-POSIX library functions to play nice with a native zsh script, it
would be necessary for native function-local traps to still work as
expected.

In other words, in my view, the following ought to work:

#! Src/zsh -f
emulate sh -c 'trap "echo POSIX exit trap triggered" EXIT'
fn() {
	trap "echo native zsh function-local exit trap triggered" EXIT
	echo entering native zsh function
}
fn

Expected output:

entering program
entering native zsh function
native zsh function-local exit trap triggered
exiting program
POSIX exit trap triggered

With your current patch, the function-local trap wipes out the global
POSIX exit trap, so the last line of expected output doesn't appear.

Thanks,

- M.

Re: [BUG] Sticky-sh POSIX_TRAPS are function-local

[Bart Schaefer]

On Feb 17, 12:38am, Martijn Dekker wrote:
} 
} With your current patch, the function-local trap wipes out the global
} POSIX exit trap, so the last line of expected output doesn't appear.

Hrm.  I think the point is that there are no function-local traps in
POSIX, so the POSIX trap obeys the POSIX rules even when another trap
is set in non-POSIX scope.

I could see an argument either way, though.  I could also see an argument
that the localtraps option should affect this (even though it's not usually
needed for global traps).

Re: [BUG] Sticky-sh POSIX_TRAPS are function-local

[Martijn Dekker]

Bart Schaefer schreef op 17-02-16 om 05:25:
> On Feb 17, 12:38am, Martijn Dekker wrote:
> } 
> } With your current patch, the function-local trap wipes out the global
> } POSIX exit trap, so the last line of expected output doesn't appear.
> 
> Hrm.  I think the point is that there are no function-local traps in
> POSIX, so the POSIX trap obeys the POSIX rules even when another trap
> is set in non-POSIX scope.

But POSIX is irrelevant to function-local traps because they are not
defined in POSIX, so, once emulation mode is exited, there is nothing
that compels you to make them wipe out a global POSIX trap.

I think it would actually be more consistent with the intention of the
POSIX trap if it survived a function-local native zsh trap; the
intention of the POSIX trap was for it to be executed on program exit,
so having it wiped out by a temporary function-local trap doesn't seem
helpful.

> I could see an argument either way, though.  I could also see an argument
> that the localtraps option should affect this (even though it's not usually
> needed for global traps).

How do you think that option should affect this?

Thanks,

- M.

Re: [BUG] Sticky-sh POSIX_TRAPS are function-local

[Bart Schaefer]

On Feb 17,  6:24am, Martijn Dekker wrote:
}
} But POSIX is irrelevant to function-local traps because they are not
} defined in POSIX, so, once emulation mode is exited, there is nothing
} that compels you to make them wipe out a global POSIX trap.

That's sort of the point -- there's nothing that compels us, hence ...

} > I could see an argument either way

... and your argument is a pretty good one.

} > I could also see an argument
} > that the localtraps option should affect this
} 
} How do you think that option should affect this?

To be pedantic, I didn't say I think that; I said I could understand how
one might argue that position.  To try to frame it -- the implication is
that a trap created under LOCAL_TRAPS should *never* alter traps in any
outer scope.  This ought to include the global scope, regardless of the
context in which the global trap was created.

I.e. given "setopt POSIX_TRAPS LOCAL_TRAPS" this argues that LOCAL_TRAPS
should "win".  With PWS's patch, POSIX_TRAPS prevails.

Re: [BUG] Sticky-sh POSIX_TRAPS are function-local

[Peter Stephenson]

On Wed, 17 Feb 2016 00:38:15 +0100
Martijn Dekker <martijn@inlv.org> wrote:
> However, since POSIX traps are inherently global, the expectation that
> POSIX traps wipe out previous traps only applies to global traps. Could
> you make an exception for function-local traps?

Quite possibly; I'll commit what I've got and look at tweaks when I get
a moment.

pws

Re: [BUG] Sticky-sh POSIX_TRAPS are function-local

[Peter Stephenson]

On Wed, 17 Feb 2016 00:38:15 +0100
Martijn Dekker <martijn@inlv.org> wrote:
> However, since POSIX traps are inherently global, the expectation that
> POSIX traps wipe out previous traps only applies to global traps. Could
> you make an exception for function-local traps?

I think this makes nested native mode EXIT traps work while preserving
a global POSIX mode EXIT trap.

pws

diff --git a/Src/signals.c b/Src/signals.c
index 32452ae..1344395 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -920,9 +920,14 @@ removetrap(int sig)
      * Note that we save the trap here even if there isn't an existing
      * one, to aid in removing this one.  However, if there's
      * already one at the current locallevel we just overwrite it.
+     *
+     * Note we save EXIT traps based on the *current* setting of
+     * POSIXTRAPS --- so if there is POSIX EXIT trap set but
+     * we are in native mode it can be saved, replaced by a function
+     * trap, and then restored.
      */
     if (!dontsavetrap &&
-	(sig == SIGEXIT ? !exit_trap_posix : isset(LOCALTRAPS)) &&
+	(sig == SIGEXIT ? !isset(POSIXTRAPS) : isset(LOCALTRAPS)) &&
 	locallevel &&
 	(!trapped || locallevel > (sigtrapped[sig] >> ZSIG_SHIFT)))
 	dosavetrap(sig, locallevel);
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index d8183a4..f4466b5 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -419,6 +419,23 @@
 >end of program
 >EXIT TRAP TRIGGERED
 
+   (cd ..; $ZTST_exe -fc '
+     echo entering program
+     emulate sh -c '\''trap "echo POSIX exit trap triggered" EXIT'\''
+     fn() {
+        trap "echo native zsh function-local exit trap triggered" EXIT
+        echo entering native zsh function
+     }
+     fn
+     echo exiting program
+   ')
+0:POSX EXIT trap can have nested native mode EXIT trap
+>entering program
+>entering native zsh function
+>native zsh function-local exit trap triggered
+>exiting program
+>POSIX exit trap triggered
+
    (set -e
     printf "a\nb\n" | while read line
     do

Re: [BUG] Sticky-sh POSIX_TRAPS are function-local

[Martijn Dekker]

Peter Stephenson schreef op 25-02-16 om 12:53:
> I think this makes nested native mode EXIT traps work while preserving
> a global POSIX mode EXIT trap.

It's nearly there. The new test case in C03traps.ztst succeeds.

But if a function defined with sticky emulation sets a POSIX trap, and
that function is called from native zsh, then the old behaviour returns
and a subsequent function-local trap wipes out the global POSIX trap. So
this still kills my POSIX library functions that set a trap.

New test script that does not succeed yet:

#! Src/zsh -f
echo entering program
emulate sh -c 'spt() { trap "echo POSIX exit trap triggered" EXIT; }'
fn() {
   trap "echo native zsh function-local exit trap triggered" EXIT
   echo entering native zsh function
}
spt
fn
echo exiting program

Expected output is identical to that of the C03traps.ztst test case.

Many thanks,

- M.

Re: [BUG] Sticky-sh POSIX_TRAPS are function-local

[Peter Stephenson]

On Thu, 25 Feb 2016 14:52:24 +0100
Martijn Dekker <martijn@inlv.org> wrote:
> New test script that does not succeed yet:

I'll have a separate look at that at some point.

pws

Re: [BUG] Sticky-sh POSIX_TRAPS are function-local

[Peter Stephenson]

On Thu, 25 Feb 2016 14:52:24 +0100
Martijn Dekker <martijn@inlv.org> wrote:
> But if a function defined with sticky emulation sets a POSIX trap, and
> that function is called from native zsh, then the old behaviour returns
> and a subsequent function-local trap wipes out the global POSIX trap. So
> this still kills my POSIX library functions that set a trap.

It looks like I've figured this out.

pws

diff --git a/Src/signals.c b/Src/signals.c
index 1344395..2eefc07 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -877,16 +877,21 @@ settrap(int sig, Eprog l, int flags)
             sig != SIGCHLD)
             install_handler(sig);
     }
+    sigtrapped[sig] |= flags;
     /*
      * Note that introducing the locallevel does not affect whether
      * sigtrapped[sig] is zero or not, i.e. a test without a mask
      * works just the same.
      */
-    sigtrapped[sig] |= (locallevel << ZSIG_SHIFT) | flags;
     if (sig == SIGEXIT) {
 	/* Make POSIX behaviour of EXIT trap sticky */
 	exit_trap_posix = isset(POSIXTRAPS);
+	/* POSIX exit traps are not local. */
+	if (!exit_trap_posix)
+	    sigtrapped[sig] |= (locallevel << ZSIG_SHIFT);
     }
+    else
+	sigtrapped[sig] |= (locallevel << ZSIG_SHIFT);
     unqueue_signals();
     return 0;
 }
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -429,14 +429,32 @@
      fn
      echo exiting program
    ')
-0:POSX EXIT trap can have nested native mode EXIT trap
+0:POSIX EXIT trap can have nested native mode EXIT trap
 >entering program
 >entering native zsh function
 >native zsh function-local exit trap triggered
 >exiting program
 >POSIX exit trap triggered
 
-   (set -e
+   (cd ..; $ZTST_exe -fc '
+     echo entering program
+     emulate sh -c '\''spt() { trap "echo POSIX exit trap triggered" EXIT; }'\''
+     fn() {
+	trap "echo native zsh function-local exit trap triggered" EXIT
+	echo entering native zsh function
+     }
+     spt
+     fn
+     echo exiting program
+   ')
+0:POSIX EXIT trap not replaced if defined within function
+>entering program
+>entering native zsh function
+>native zsh function-local exit trap triggered
+>exiting program
+>POSIX exit trap triggered
+
+    (set -e
     printf "a\nb\n" | while read line
     do
        [[ $line = a* ]] || continue