[Patch] Fix ERR_RETURN behavior in and/or statements

[Patch] Fix ERR_RETURN behavior in and/or statements

[Philippe Altherr]

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

ATTENTION: This patch depends on the following patches described here
<https://inbox.vuxu.org/zsh-workers/CAGdYchufLwUgpKVU6b5eOiF15p0njFr59H1q9XizypwNjzqKzw@mail.gmail.com/>
:
- patch-01-revert-mistaken-errexit-patches.txt
- patch-03-fix-negation-statement.txt

I have found another issue in execlist, which affects the behavior of
ERR_RETURN in convoluted cases that involve at least two &&/|| commands and
a function call. The following example exhibits the issue.

set -o err_return
> fn() { { false; echo fn1 } && echo fn2 }
> fn; echo done
> fn && echo done


The execution of "fn; echo done" prints "fn1", "fn2", and "done". The
execution of "fn && echo done" should print the same but currently prints
nothing.

The problem is that this test
<https://github.com/zsh-users/zsh/blob/291940bae6cc0471c35c73498e873bc58dae9a95/Src/exec.c#L1429>
in "execlist" is too conservative. Inside "fn", "oldnoerrexit" is equal to
"NOERREXIT_EXIT". Indeed the "&&" in "fn && echo done"  first sets
"noerrexit" to  "NOERREXIT_EXIT | NOERREXIT_RETURN". The call to "fn" then
removes "NOERREXIT_RETURN". Thus, at the beginning of "execlist" in the
call to "fn", "noerrexit" is equal to "NOERREXIT_EXIT" and "oldnoerrexit"
is initialized to the same value. Since "oldnoerrexit" is non-zero, the test
<https://github.com/zsh-users/zsh/blob/291940bae6cc0471c35c73498e873bc58dae9a95/Src/exec.c#L1429>
fails
to readd "NOERREXIT_RETURN" for the evaluation of "{ false; echo fn1 }".
Therefore the evaluation of "false" triggers an early return of "fn" wîth
exit status 1, which in turn triggers an early return of the toplevel and
thus nothing is printed.

The solution is to unconditionally set "noerrexit" to "NOERREXIT_EXIT |
NOERREXIT_RETURN" for any command on the left of "&&" and "||" (and for any
command on the right of "!"). The patch does exactly that (and adds a
regression test).

Philippe

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

[-- Attachment #2: patch-05-fix-and-or-statement.txt --]
[-- Type: text/plain, Size: 1955 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index 43df8211a..711d8f374 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1427,14 +1427,12 @@ execlist(Estate state, int dont_change_job, int exiting)
 	    goto sublist_done;
 	}
 	while (wc_code(code) == WC_SUBLIST) {
-	    int isend = (WC_SUBLIST_TYPE(code) == WC_SUBLIST_END);
+	    int isandor = WC_SUBLIST_TYPE(code) != WC_SUBLIST_END;
+	    int isnot = WC_SUBLIST_FLAGS(code) & WC_SUBLIST_NOT;
 	    next = state->pc + WC_SUBLIST_SKIP(code);
-	    if (!oldnoerrexit)
-		noerrexit = isend ? 0 : NOERREXIT_EXIT | NOERREXIT_RETURN;
-	    if (WC_SUBLIST_FLAGS(code) & WC_SUBLIST_NOT) {
-		/* suppress errexit for the commands in ! <list-of-commands> */
+	    /* suppress errexit for commands before && and || and after ! */
+	    if (isandor || isnot)
 		noerrexit = NOERREXIT_EXIT | NOERREXIT_RETURN;
-	    }
 	    switch (WC_SUBLIST_TYPE(code)) {
 	    case WC_SUBLIST_END:
 		/* End of sublist; just execute, ignoring status. */
@@ -1444,7 +1442,7 @@ execlist(Estate state, int dont_change_job, int exiting)
 		    execpline(state, code, ltype, (ltype & Z_END) && exiting);
 		state->pc = next;
 		/* suppress errexit for the command "! ..." */
-		if (WC_SUBLIST_FLAGS(code) & WC_SUBLIST_NOT)
+		if (isnot)
 		  this_noerrexit = 1;
 		goto sublist_done;
 		break;
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index a8880673f..b7132da81 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -670,6 +670,22 @@ F:Must be tested with a top-level script rather than source or function
 >before-out
 >before-in
 
+  (set -o err_return
+    fn() {
+      print before-in
+      { false; true } && true
+      print after-in
+    }
+    print before-out
+    fn && true
+    print after-out
+  )
+0:ERR_RETURN not triggered on LHS of "&&" in function on LHS of "&&" (regression test)
+>before-out
+>before-in
+>after-in
+>after-out
+
   mkdir -p zdotdir
   print >zdotdir/.zshenv '
   setopt norcs errreturn

Re: [Patch] Fix ERR_RETURN behavior in and/or statements

[Bart Schaefer]

On Fri, Nov 25, 2022 at 9:45 AM Philippe Altherr
<philippe.altherr@gmail.com> wrote:
>
> ATTENTION: This patch depends on the following patches described here:
> - patch-01-revert-mistaken-errexit-patches.txt
> - patch-03-fix-negation-statement.txt

I'll take a look at all of this in a couple of days.

Re: [Patch] Fix ERR_RETURN behavior in and/or statements

[Philippe Altherr]

[-- Attachment #1: Type: text/plain, Size: 546 bytes --]

>
> I'll take a look at all of this in a couple of days.
>

Sounds good

I just sent a 6th patch. With that, all the errexit issues I'm aware of are
fixed.

I still have on my todo list to write a NEWS item that documents the
changes/fixes. So there will be a 7th patch but that one will only touch
the NEWS file.

There is one last thing that I don't understand in the errexit code, namely
the NOERREXIT_UNTIL_EXEC bit used in execif. I will spend some time
studying it in the next few days. Hopefully, I don't discover new issues :-)

Philippe

[-- Attachment #2: Type: text/html, Size: 884 bytes --]

Re: [Patch] Fix ERR_RETURN behavior in and/or statements

[Philippe Altherr]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

Hi Bart,

With my last patch (patch-08-use-or-assignments-to-set-noerrexit-bits), I
think that I'm done with code changes. The best way to understand all of my
changes might be to first apply all my 8 patches and have a look at all the
places that deal with noerrexit (just grep for noerrexit in all files). The
noerrexit logic is now quite a bit smaller and also more regular. For
example all places that deal with conditions (if condition, while
condition, left of &&/||, and right of !) now behave in very similar ways.
Same for all compound commands. You can then have a look at the individual
patches.

Philippe


On Sat, Nov 26, 2022 at 4:47 AM Philippe Altherr <philippe.altherr@gmail.com>
wrote:

> I'll take a look at all of this in a couple of days.
>>
>
> Sounds good
>
> I just sent a 6th patch. With that, all the errexit issues I'm aware of
> are fixed.
>
> I still have on my todo list to write a NEWS item that documents the
> changes/fixes. So there will be a 7th patch but that one will only touch
> the NEWS file.
>
> There is one last thing that I don't understand in the errexit code,
> namely the NOERREXIT_UNTIL_EXEC bit used in execif. I will spend some time
> studying it in the next few days. Hopefully, I don't discover new issues :-)
>
> Philippe
>
>

[-- Attachment #2: Type: text/html, Size: 1950 bytes --]

Re: [Patch] Fix ERR_RETURN behavior in and/or statements

[Philippe Altherr]

[-- Attachment #1: Type: text/plain, Size: 1566 bytes --]

Fyi: my 2 messages about my 7th patch
(patch-07-remove-noerrexit-until-exec.txt) are currently stuck in the
moderation queue. No clue why.

Philippe


On Sat, Dec 3, 2022 at 2:31 AM Philippe Altherr <philippe.altherr@gmail.com>
wrote:

> Hi Bart,
>
> With my last patch (patch-08-use-or-assignments-to-set-noerrexit-bits), I
> think that I'm done with code changes. The best way to understand all of my
> changes might be to first apply all my 8 patches and have a look at all the
> places that deal with noerrexit (just grep for noerrexit in all files). The
> noerrexit logic is now quite a bit smaller and also more regular. For
> example all places that deal with conditions (if condition, while
> condition, left of &&/||, and right of !) now behave in very similar ways.
> Same for all compound commands. You can then have a look at the individual
> patches.
>
> Philippe
>
>
> On Sat, Nov 26, 2022 at 4:47 AM Philippe Altherr <
> philippe.altherr@gmail.com> wrote:
>
>> I'll take a look at all of this in a couple of days.
>>>
>>
>> Sounds good
>>
>> I just sent a 6th patch. With that, all the errexit issues I'm aware of
>> are fixed.
>>
>> I still have on my todo list to write a NEWS item that documents the
>> changes/fixes. So there will be a 7th patch but that one will only touch
>> the NEWS file.
>>
>> There is one last thing that I don't understand in the errexit code,
>> namely the NOERREXIT_UNTIL_EXEC bit used in execif. I will spend some time
>> studying it in the next few days. Hopefully, I don't discover new issues :-)
>>
>> Philippe
>>
>>

[-- Attachment #2: Type: text/html, Size: 2526 bytes --]

Re: [Patch] Fix ERR_RETURN behavior in and/or statements

[Lawrence Velázquez]

On Fri, Dec 2, 2022, at 8:36 PM, Philippe Altherr wrote:
> Fyi: my 2 messages about my 7th patch 
> (patch-07-remove-noerrexit-until-exec.txt) are currently stuck in the 
> moderation queue. No clue why.

Because Sympa thought they were spam.  I just released them.

-- 
vq