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

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

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