Re: errexit and (Z)ERR trap regression

[Philippe Altherr <philippe.altherr@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4937 bytes --]

>
> This doesn't seem necessary to me.  For one thing, it's equivalent to:


>   noerrexit = oldnoerrexit;
>   if (isandor || isnot)
>     noerrexit |= NOERREXIT_EXIT | NOERREXIT_RETURN;


I agree that it's equivalent but I disagree that it (or something similar)
isn't necessary ;-)

But assigning (noerrexit = oldnoerrexit) presumes that noerrexit was
> (improperly?) cleared at or after the point where olderrexit was
> recorded.


Which is indeed the problem. To see it, let's take a wider view of the code
at hand:

    int oldnoerrexit = noerrexit;
    // ...
    while (wc_code(code) == WC_SUBLIST) {
        // ...
*        noerrexit = oldnoerrexit;*
*        if (isandor || isnot)*
*            noerrexit |= NOERREXIT_EXIT | NOERREXIT_RETURN;*
        switch (WC_SUBLIST_TYPE(code)) {
            case WC_SUBLIST_END: // ...
            case WC_SUBLIST_AND: // ...
            case WC_SUBLIST_OR: // ...
        }
        // ...
    }
    sublist_done:
    noerrexit = oldnoerrexit;

It's true that in the first iteration of the while loop, the noerrexit =
oldnoerrexit is always a nop. However, that isn't the case for subsequent
iterations and in particular for the last iteration where it's important to
always restore noerrexit to its original value (i.e., oldnoerrexit) even if
that value isn't equal to 0.

Now that I see this code under this angle, I think that a better version is
one where noerrexit = oldnoerrexit is placed after the switch statement.
This way it's easier to grasp why it's necessary (or why it's not a nop).
Ideally we would then also drop the noerrexit = oldnoerrexit after the
sublist_done label. Unfortunately, that's not possible because the switch
statement contains multiple goto statements that jump to the label and that
bypass any noerrexit = oldnoerrexit after the switch statement.

> Here are two other examples fixed by this patch:
> >
> >     zsh -c 'trap "echo Trapped!" ERR; true && if true; then false; fi'
> >     zsh -c 'trap "echo Trapped!" ERR; true && {false} always {true}'


> These two cases also pass with my patch from workers/52973.  Do you
> have an example where my patch doesn't work?


Yes, these two cases were just meant to highlight slightly different code
paths that also trigger the bug. And yes, I managed to build an example
that fails with your patch. I think that the following is a minimal
example, i.e., both && and the braces around the false are needed to
trigger the bug:

zsh -c 'set -o ERR_RETURN; f() { true && { false }; echo "NOT REACHED";  };
f && true'

In this example "f" in "f && true" is evaluated with
"errnoexit = NOERREXIT_EXIT | NOERREXIT_RETURN". The call to "f" clears
the NOERREXIT_RETURN bit. Thus "true && { false }" is evaluated
with "errnoexit = NOERREXIT_EXIT" and "olderrnoexit" is initialized to that
value in "execlist". The evaluation of "true" sets "errnoexit" to
"NOERREXIT_EXIT | NOERREXIT_RETURN". Now, if "errnoexit" is left unchanged,
as is the case in version 5.9, or set to 0 only if "olderrnoexit" is equal
to 0, then "{ false }" is evaluated with "errnoexit = NOERREXIT_EXIT |
NOERREXIT_RETURN" and fails to trigger ERR_RETURN. With my path "errnoexit"
is restored to "NOERREXIT_EXIT", which enables "{ false }" to trigger
ERR_RETURN.

Below is an updated patch with the cleaner and simpler fix described above
and a new test case for the ERR_RETURN example.

Philippe


On Wed, Jun 26, 2024 at 11:43 PM Bart Schaefer <schaefer@brasslantern.com>
wrote:

> On Wed, Jun 26, 2024 at 5:43 AM Philippe Altherr
> <philippe.altherr@gmail.com> wrote:
> >
> > I think that the correct fix is the following:
> >
> >     if (isandor || isnot)
> >         noerrexit = oldnoerrexit | NOERREXIT_EXIT | NOERREXIT_RETURN;
> >     else
> >         noerrexit = oldnoerrexit;
>
> This doesn't seem necessary to me.  For one thing, it's equivalent to:
>
>   noerrexit = oldnoerrexit;
>   if (isandor || isnot)
>     noerrexit |= NOERREXIT_EXIT | NOERREXIT_RETURN;
>
> But assigning (noerrexit = oldnoerrexit) presumes that noerrexit was
> (improperly?) cleared at or after the point where olderrexit was
> recorded.  If that were the situation, then --
>
> > For reminder, here is the current code:
> >
> >     if (isandor || isnot)
> >         noerrexit |= NOERREXIT_EXIT | NOERREXIT_RETURN;
>
> -- would be insufficient for the existing test cases, I think.  That
> is, either (olderrexit == noerrexit), or it's not necessary to
> OR-together olderrexit with the new values.
>
> > Here are two other examples fixed by this patch:
> >
> >     zsh -c 'trap "echo Trapped!" ERR; true && if true; then false; fi'
> >     zsh -c 'trap "echo Trapped!" ERR; true && {false} always {true}'
>
> These two cases also pass with my patch from workers/52973.  Do you
> have an example where my patch doesn't work?
>

[-- Attachment #1.2: Type: text/html, Size: 7320 bytes --]

[-- Attachment #2: noerrexit-list-subcommands.txt --]
[-- Type: text/plain, Size: 1658 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index e955e85df..a473938ec 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1568,6 +1568,7 @@ execlist(Estate state, int dont_change_job, int exiting)
 	    }
 	    state->pc = next;
 	    code = *state->pc++;
+	    noerrexit = oldnoerrexit;
 	}
 	state->pc--;
 sublist_done:
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index de57765a0..87b7fd1f7 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -995,6 +995,33 @@ F:Must be tested with a top-level script rather than source or function
 ?loop 0
 ?loop 1
 
+  ( set -e; true && {false; echo NOT REACHED} )
+  ( trap "print Trapped!" ERR; true && {false} )
+  ( trap "print Trapped!" ERR; true && if true; then false; fi )
+  ( trap "print Trapped!" ERR; true && {false} always {true} )
+  ( true && (set -e; false; echo NOT REACHED) )
+  ( true && (trap "print Trapped!" ERR; false) )
+  ( true && { set -e; false; echo NOT REACHED } )
+  ( true && { trap "print Trapped!" ERR; false } )
+  ( set -e; true && (false; echo one) || echo two )
+  ( set -e; true && { false; echo one; } || echo two )
+0:ERR_EXIT is triggered by last command in an AND-OR list
+>Trapped!
+>Trapped!
+>Trapped!
+>Trapped!
+>Trapped!
+>one
+>one
+
+  ( set -o ERR_RETURN; f() { false; echo NOT REACHED;  }; f || true; echo OK )
+  ( set -o ERR_RETURN; f() { true && false; echo NOT REACHED;  }; f || true; echo OK )
+  ( set -o ERR_RETURN; f() { true && { false }; echo NOT REACHED;  }; f || true; echo OK )
+0:ERR_RETURN is triggered in function calls on the left of an AND-OR
+>OK
+>OK
+>OK
+
   if zmodload zsh/system 2>/dev/null; then
   (
     trap 'echo TERM; exit 2' TERM