Re: local_traps doesn't restore traps set from functions

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Roman Perepelitsa wrote on Wed, 06 May 2020 15:31 +0200:
> Is this working as intended?
> 
>   ❯ zsh -f
>   adam% () { trap '' INT }
>   adam% () { emulate -L zsh; trap 'echo INT' INT }
>   adam% kill -INT $$
>   INT
>   adam%
> 

I'd say it doesn't, since the trap _is_ set before the second function is called —
.
    % () { trap '' INT }
    % kill -INT $$
    % 
.
— and the documentation is quite clear about the semantics in this case:

       LOCAL_TRAPS <K>
              If this option is set when a signal trap is set inside a
              function, then the previous status of the trap for that signal
              will be restored when the function exits.

> It appears that local_traps doesn't restore traps that were originally
> set from functions.

Precisely.

The following patch fixes it:

[[[
diff --git a/Src/signals.c b/Src/signals.c
index 4adf03202..3e4fdf370 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -1154,6 +1154,29 @@ endtrapscope(void)
 	}
     }
 
+    /* Fixup locallevel of signals. */
+    {
+	int i;
+	for (i = 0; i < VSIGCOUNT; ++i) {
+	    int locallevel_of_signal = (sigtrapped[i] >> ZSIG_SHIFT);
+	    if (locallevel_of_signal > locallevel) {
+		DPUTS3(locallevel_of_signal != locallevel + 1,
+		    "BUG: signal %s's locallevel unexpected: %d>>ZSIG_SHIFT seen, v. %d+1 expected",
+		    sigs[i], sigtrapped[i], locallevel);
+
+		/* 
+		 * Decrement the locallevel embedded in sigtrapped[i].
+		 *
+		 * Keep the low ZSIG_SHIFT bits unchanged.
+		 */
+		sigtrapped[i] =
+		    (sigtrapped[i] & ((1u << ZSIG_SHIFT) - 1u))
+		    |
+		    (locallevel << ZSIG_SHIFT);
+	    }
+	}
+    }
+
     if (exittr) {
 	/*
 	 * We already made sure this wasn't set as a POSIX exit trap.
diff --git a/Src/zsh.h b/Src/zsh.h
index 1f2d774a1..71ba6e6e0 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -2940,8 +2940,8 @@ struct heap {
 #define ZSIG_FUNC	(1<<2)	/* Trap is a function, not an eval list */
 /* Mask to get the above flags */
 #define ZSIG_MASK	(ZSIG_TRAPPED|ZSIG_IGNORED|ZSIG_FUNC)
-/* No. of bits to shift local level when storing in sigtrapped */
 #define ZSIG_ALIAS	(1<<3)  /* Trap is stored under an alias */
+/* No. of bits to shift locallevel when storing in sigtrapped */
 #define ZSIG_SHIFT	4
 
 /*
]]]

[[[
% () { trap '' INT }
% () { emulate -L zsh; trap 'echo INT' INT }
% kill -INT $$
% 
]]]

However, I don't know this area of the code well enough to be sure the
patch doesn't break anything.  Reviews would be appreciated.

Also, if someone could write a regression test for this, that'd be great.

Cheers,

Daniel