local_traps doesn't restore traps set from functions

local_traps doesn't restore traps set from functions

[Roman Perepelitsa]

Is this working as intended?

  ❯ zsh -f
  adam% () { trap '' INT }
  adam% () { emulate -L zsh; trap 'echo INT' INT }
  adam% kill -INT $$
  INT
  adam%

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

If the first trap is set from global scope, it works as expected:

  ❯ zsh -f
  adam% trap '' INT
  adam% () { emulate -L zsh; trap 'echo INT' INT }
  adam% kill -INT $$
  adam%

If the first trap is not set at all, it also works as expected:

  ❯ zsh -f
  adam% () { emulate -L zsh; trap 'echo INT' INT }
  adam% kill -INT $$
  adam%

ZSH_PATCHLEVEL is zsh-5.8-126-gab835f0 (tip of master at the time of writing).

Roman.

Re: local_traps doesn't restore traps set from functions

[Daniel Shahaf]

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

Re: local_traps doesn't restore traps set from functions

[Roman Perepelitsa]

On Thu, May 7, 2020 at 11:21 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> The following patch fixes it:

Thanks for the patch. I confirm that it works. I've tried a handful of
different combinations of traps and all work as expected.

One stylistic suggestion for the patch: replace sigtrapped[i] fixup with this:

    sigtrapped[i] -= 1 << ZSIG_SHIFT;

It'll align the code with the preceding comment to the point of making
the comment superfluous. I also find it easier to understand although
that may be subjective.

Roman.

Re: local_traps doesn't restore traps set from functions

[Daniel Shahaf]

Roman Perepelitsa wrote on Fri, 08 May 2020 08:51 +0200:
> On Thu, May 7, 2020 at 11:21 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >
> > The following patch fixes it:  
> 
> Thanks for the patch. I confirm that it works. I've tried a handful of
> different combinations of traps and all work as expected.
> 

Thanks for testing.

> One stylistic suggestion for the patch: replace sigtrapped[i] fixup with this:
> 
>     sigtrapped[i] -= 1 << ZSIG_SHIFT;
> 
> It'll align the code with the preceding comment to the point of making
> the comment superfluous. I also find it easier to understand although
> that may be subjective.

Good point, thanks.  Perhaps it'd be clearer if I wrapped the first
disjunct by a macro, so it'd have a name and a doc string:

/* ... */
static unsigned int low_N_bits_of(unsigned arg, unsigned char N) { return arg & ((1u << N)-1u); }

(Function, rather than macro, for type safety in case the arguments are
passed in the wrong order.)

Cheers,

Daniel

Re: local_traps doesn't restore traps set from functions

[Peter Stephenson]

> On 07 May 2020 at 22:21 Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Roman Perepelitsa wrote on Wed, 06 May 2020 15:31 +0200:
> > It appears that local_traps doesn't restore traps that were originally
> > set from functions.
> 
> Precisely.
> 
> The following patch fixes it:

OK, so the point here is that in this case the trap we're fiddling with
is not going out of scope, nor having something else restored over it,
it just happens to have been created inside a function.  At this
point, that fact becomes irrelevant, so we simply massage the information
to make it behave as if it had been created at the current (new) function
depth.

That seems fine --- my only comment would be it would probably be sensible
to insert some edited version of the previous paragraph as I for one
am almost certain to wonder what's going on if I encounter this code
in the future.

cheers
pws

Re: local_traps doesn't restore traps set from functions

[Daniel Shahaf]

Peter Stephenson wrote on Sat, 09 May 2020 16:24 +0100:
> > On 07 May 2020 at 22:21 Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > Roman Perepelitsa wrote on Wed, 06 May 2020 15:31 +0200:  
> > > It appears that local_traps doesn't restore traps that were originally
> > > set from functions.  
> > 
> > Precisely.
> > 
> > The following patch fixes it:  
> 
> OK, so the point here is that in this case the trap we're fiddling with
> is not going out of scope, nor having something else restored over it,
> it just happens to have been created inside a function.  At this
> point, that fact becomes irrelevant, so we simply massage the information
> to make it behave as if it had been created at the current (new) function
> depth.
> 
> That seems fine --- my only comment would be it would probably be sensible
> to insert some edited version of the previous paragraph as I for one
> am almost certain to wonder what's going on if I encounter this code
> in the future.

Will do.  Thanks for the review!

Daniel

Re: local_traps doesn't restore traps set from functions

[Daniel Shahaf]

Daniel Shahaf wrote on Sat, 09 May 2020 15:42 +0000:
> Peter Stephenson wrote on Sat, 09 May 2020 16:24 +0100:
> > > On 07 May 2020 at 22:21 Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > > Roman Perepelitsa wrote on Wed, 06 May 2020 15:31 +0200:    
> > > > It appears that local_traps doesn't restore traps that were originally
> > > > set from functions.    
> > > 
> > > Precisely.
> > > 
> > > The following patch fixes it:    
> > 
> > OK, so the point here is that in this case the trap we're fiddling with
> > is not going out of scope, nor having something else restored over it,
> > it just happens to have been created inside a function.  At this
> > point, that fact becomes irrelevant, so we simply massage the information
> > to make it behave as if it had been created at the current (new) function
> > depth.
> > 
> > That seems fine --- my only comment would be it would probably be sensible
> > to insert some edited version of the previous paragraph as I for one
> > am almost certain to wonder what's going on if I encounter this code
> > in the future.  
> 
> Will do.  Thanks for the review!

Here's an interdiff, to be applied on top of 45790.

Test will follow separately.

diff --git a/Src/signals.c b/Src/signals.c
index 5d0bae7f5..56b3071de 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -30,8 +30,16 @@
 #include "zsh.mdh"
 #include "signals.pro"
  
-/* Array describing the state of each signal: an element contains *
- * 0 for the default action or some ZSIG_* flags ored together.   */
+/* 
+ * Array describing the state of each signal: an element contains
+ * 0 for the default action or some ZSIG_* flags ored together.
+ *
+ * The high bits (see ZSIG_SHIFT) identify the locallevel that owns the trap.
+ * Whenever execution returns to that locallevel (which is never larger/deeper
+ * than the current locallevel) from deeper levels, any function-scoped traps
+ * that shadow this trap will have gone out of scope.  See the LOCALTRAPS
+ * condition in removetrap().
+ */
 
 /**/
 mod_export int sigtrapped[VSIGCOUNT];
@@ -997,9 +1005,12 @@ removetrap(int sig)
     queue_signals();
     trapped = sigtrapped[sig];
     /*
+     * Save the trap if we should restore it at the end of the current function.
+     *
      * 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.
+     * one, to aid in removing this one.  We also save the trap if it's owned
+     * by someone up the callstack.  However, if the trap is owned by 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
@@ -1062,6 +1073,16 @@ removetrap(int sig)
     return NULL;
 }
 
+/*
+ * Return the value of ARG after setting all but the N least significant bits
+ * to zero.
+ */
+static unsigned int
+low_N_bits_of(unsigned int arg, unsigned char N)
+{
+    return arg & ((1u << N)-1u);
+}
+
 /**/
 void
 starttrapscope(void)
@@ -1114,6 +1135,9 @@ endtrapscope(void)
 	sigtrapped[SIGEXIT] = 0;
     }
 
+    /*
+     * Restore traps from savetraps to sigtrapped.
+     */
     if (savetraps) {
 	while ((ln = firstnode(savetraps)) &&
 	       (st = (struct savetrap *) ln->dat) &&
@@ -1122,6 +1146,8 @@ endtrapscope(void)
 
 	    remnode(savetraps, ln);
 
+	    /* ### This doesn't mask ZSIG_IGNORED out of the boolean check,
+	     * ### presumably because checking st->list is sufficient? */
 	    if (st->flags && (st->list != NULL)) {
 		/* prevent settrap from saving this */
 		dontsavetrap++;
@@ -1154,7 +1180,11 @@ endtrapscope(void)
 	}
     }
 
-    /* Fixup locallevel of signals. */
+    /*
+     * If the function that's in the process of returning set a global trap,
+     * that trap is now owned by that function's caller.  Update sigtrapped
+     * accordingly.
+     */
     {
 	int i;
 	for (i = 0; i < VSIGCOUNT; ++i) {
@@ -1170,7 +1200,7 @@ endtrapscope(void)
 		 * Keep the low ZSIG_SHIFT bits unchanged.
 		 */
 		sigtrapped[i] =
-		    (sigtrapped[i] & ((1u << ZSIG_SHIFT) - 1u))
+		    low_N_bits_of(sigtrapped[i], ZSIG_SHIFT)
 		    |
 		    (locallevel << ZSIG_SHIFT);
 	    }