Zsh Mailing List Archive
Messages sorted by: Reverse Date, Date, Thread, Author

Re: errexit and (Z)ERR trap regression

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@xxxxxxxxxxxxxxxx> wrote:
On Wed, Jun 26, 2024 at 5:43 AM Philippe Altherr
<philippe.altherr@xxxxxxxxx> 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?
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
Messages sorted by: Reverse Date, Date, Thread, Author