Failed pipeline doesn't err_exit/return if complex command last

Failed pipeline doesn't err_exit/return if complex command last

[Johan Grande]

Hi all,

I noticed a strange behavior when using a complex command in a pipeline.

With `set -eo pipefail`, a failing pipeline that ends in a complex
command will have non-zero status but not exit the script (or a function
with err_return):

     false | if true; then true; fi
     echo $?

=> prints "1", exits with 0

However, this only occurs if the `if` command is last. If we add a `|
true` at the end:

     false | if true; then true; fi | true
     echo $?

=> no output, exits with 1

Same with a while loop (I only checked these 2 complex commands):

     false | while read line; do true; done
     echo $?

=> prints "1", exits with 0

     false | while read line; do true; done | true
     echo $?

=> no output, exits with 1

It seems inconsistent to me that a complex command be treated just like 
a simple command in the middle of a pipeline but not at the end, making 
a "list" that has non-zero status but doesn't return/exit despite the 
options.

As a sanity check, I ran the examples with bash and the result is what I 
would expect in all 4 cases: no output, exit with 1.

I'm using zsh 5.8.1 (x86_64-ubuntu-linux-gnu).

-- 
Johan Grande

Re: Failed pipeline doesn't err_exit/return if complex command last

[Johan Grande]

Le 06/07/2023 à 01:18, Johan Grande a écrit :
> With `set -eo pipefail`, a failing pipeline that ends in a complex
> command will have non-zero status but not exit the script (or a function
> with err_return):

Note: This happens even if the complex command is inside a function:

     f() { { true; true } }
     false | f
     echo $?

=> prints "1", exits with 0

     f() { true }
     false | f
     echo $?

=> no output, exits with 1

-- 
Johan

Re: Failed pipeline doesn't err_exit/return if complex command last

[Peter Stephenson]

> On 06/07/2023 00:18 Johan Grande <nahoj@crans.org> wrote:
> I noticed a strange behavior when using a complex command in a pipeline.
> 
> With `set -eo pipefail`, a failing pipeline that ends in a complex
> command will have non-zero status but not exit the script (or a function
> with err_return):

So in a nutshell

fn() {
  emulate -L zsh
  setopt errreturn pipefail
  false | { true }
  print "Shouldn't get here, status $?"
}

The final part of the pipeline is run in the current shell, so the
job control is a bit fiddly.  It looks like the relevant code,
including the function storepipestats(), is run too late in this
case.

pws

Re: Failed pipeline doesn't err_exit/return if complex command last

[Bart Schaefer]

On Thu, Jul 6, 2023 at 2:44 AM Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
>
> The final part of the pipeline is run in the current shell, so the
> job control is a bit fiddly.  It looks like the relevant code,
> including the function storepipestats(), is run too late in this
> case.

Possibly relevant, I'm getting random failures of the expected output
status for E01 "Was testing: PIPE_FAIL option" on MacOS Catalina.  The
test line
  true | false | true
sometimes returns 147 instead of 1.

Re: Failed pipeline doesn't err_exit/return if complex command last

[Peter Stephenson]

On Fri, 2023-07-14 at 20:31 +0200, Johan Grande wrote:
> Le 06/07/2023 à 11:44, Peter Stephenson a écrit :
> > > On 06/07/2023 00:18 Johan Grande <nahoj@crans.org> wrote:
> > > I noticed a strange behavior when using a complex command in a pipeline.
> > So in a nutshell
> 
> I'm curious about the process: whether the bug is likely going to be 
> fixed, and whether it's tracked somewhere public.
> 
> No urgency or pressure in my request, I'm just happy to follow up.

There's nowhere to look, other than this list --- if someone here doesn't
pick it up, no one is going to.

This area of the code has given a number of us sleepless nights over the
years, with many hours spent trying to resolve subtle job control issues,
so unless someone happens to spot some key clue, I doubt it's going anywhere
very fast.  Every now and then there are bursts of activity, however.

Cheers
pws

Re: Failed pipeline doesn't err_exit/return if complex command last

[Johan Grande]

Le 16/07/2023 à 16:36, Peter Stephenson a écrit :
> On Fri, 2023-07-14 at 20:31 +0200, Johan Grande wrote:
>> Le 06/07/2023 à 11:44, Peter Stephenson a écrit :
>>>> On 06/07/2023 00:18 Johan Grande <nahoj@crans.org> wrote:
>>>> I noticed a strange behavior when using a complex command in a pipeline.
>>> So in a nutshell
>>
>> I'm curious about the process: whether the bug is likely going to be
>> fixed, and whether it's tracked somewhere public.
>>
>> No urgency or pressure in my request, I'm just happy to follow up.
> 
> There's nowhere to look, other than this list --- if someone here doesn't
> pick it up, no one is going to.
> 
> This area of the code has given a number of us sleepless nights over the
> years, with many hours spent trying to resolve subtle job control issues,
> so unless someone happens to spot some key clue, I doubt it's going anywhere
> very fast.  Every now and then there are bursts of activity, however.

Understood, thank you for your answer.

In the meantime, the problem is easily worked around by suffixing every 
pipeline with || return $?.

-- 
Johan

Re: Failed pipeline doesn't err_exit/return if complex command last

[Bart Schaefer]

On Thu, Jul 6, 2023 at 2:44 AM Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
>
\> fn() {
>   emulate -L zsh
>   setopt errreturn pipefail
>   false | { true }
>   print "Shouldn't get here, status $?"
> }
>
> The final part of the pipeline is run in the current shell, so the
> job control is a bit fiddly.  It looks like the relevant code,
> including the function storepipestats(), is run too late in this
> case.

It's not that it's run too late, it's that it's run too SOON:
pipestatus has already been updated and lastval set, but then we get
here:

4186            if (!errflag) {
4187                int ret = execbuiltin(args, assigns, (Builtin) hn);
4188                /*
4189                 * In case of interruption assume builtin status
4190                 * is less useful than what interrupt set.
4191                 */
4192                if (!(errflag & ERRFLAG_INT))
4193                lastval = ret;
4194            }

and clobber lastval with the result of execbuiltin().  Or that's how
it currently appears from pass through gdb.  Per "fiddly" it's a bit
hard to be sure.

Re: Failed pipeline doesn't err_exit/return if complex command last

[Bart Schaefer]

On Mon, Jul 17, 2023 at 4:34 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> It's not that it's run too late, it's that it's run too SOON:
> pipestatus has already been updated and lastval set, but then

No, I'm wrong.  Sorry for the noise.

Re: Failed pipeline doesn't err_exit/return if complex command last

[Peter Stephenson]

> fn () {
>   emulate -L zsh
>   setopt errreturn pipefail
>   false | {
>     true
>   }
>   print "Shouldn't get here, status $?"
> }

A bit more prodding reveals it's not actually timing that
has this effect --- the problem is this_noerrexit being
set to indicate something like "we're doing that complicated
internal shell stuff and we've discovered the status is OK".
This time it isn['t.

Not sure if I need the second case or not, certainly
the first case is the one we're hitting here (the
STAT_CURSH is the giveaway).  My guess is that it's
simply irrelevant because if there's no STAT_CURSH
we aren't going to hit those cases where this_noerrexit
becomes non-zero.

Obviously, there could be even more complicated cases that
still cause problems, but I think we'll just have to discover
those.  I'll write some tests that might help.

Note I fixed some indentation.

pws

diff --git a/Src/jobs.c b/Src/jobs.c
index dd7bba405..a3b9f667a 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -427,11 +427,17 @@ storepipestats(Job jn, int inforeground, int fixlastval)
     }
 
     if (fixlastval) {
-      if (jn->stat & STAT_CURSH) {
-	if (!lastval && isset(PIPEFAIL))
-	  lastval = pipefail;
-      } else if (isset(PIPEFAIL))
-	lastval = pipefail;
+	if (jn->stat & STAT_CURSH) {
+	    if (!lastval && isset(PIPEFAIL)) {
+		if (inforeground)
+		    this_noerrexit = 0;
+		lastval = pipefail;
+	    }
+	} else if (isset(PIPEFAIL)) {
+	    if (inforeground)
+		this_noerrexit = 0;
+	    lastval = pipefail;
+	}
     }
 }

Re: Failed pipeline doesn't err_exit/return if complex command last

[Peter Stephenson]

> On 18/07/2023 14:57 Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> > fn () {
> >   emulate -L zsh
> >   setopt errreturn pipefail
> >   false | {
> >     true
> >   }
> >   print "Shouldn't get here, status $?"
> > }
> 
> A bit more prodding reveals it's not actually timing that
> has this effect --- the problem is this_noerrexit being
> set to indicate something like "we're doing that complicated
> internal shell stuff and we've discovered the status is OK".
> This time it isn['t.
>
> Obviously, there could be even more complicated cases that
> still cause problems, but I think we'll just have to discover
> those.  I'll write some tests that might help.

Here's the patch with some tests.

pws


diff --git a/Src/jobs.c b/Src/jobs.c
index dd7bba405..a3b9f667a 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -427,11 +427,17 @@ storepipestats(Job jn, int inforeground, int fixlastval)
     }
 
     if (fixlastval) {
-      if (jn->stat & STAT_CURSH) {
-	if (!lastval && isset(PIPEFAIL))
-	  lastval = pipefail;
-      } else if (isset(PIPEFAIL))
-	lastval = pipefail;
+	if (jn->stat & STAT_CURSH) {
+	    if (!lastval && isset(PIPEFAIL)) {
+		if (inforeground)
+		    this_noerrexit = 0;
+		lastval = pipefail;
+	    }
+	} else if (isset(PIPEFAIL)) {
+	    if (inforeground)
+		this_noerrexit = 0;
+	    lastval = pipefail;
+	}
     }
 }
 
diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index 533e08773..83f0371a1 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -1379,6 +1379,64 @@ F:Regression test for workers/41811
 >1
 >2
 
+  pipefailfn1() {
+     emulate -L zsh
+     setopt errreturn pipefail
+     false | { true; }
+     print "Shouldn't get here, status $?"
+  }
+  pipefailfn1
+1:PIPE_FAIL causes ERR_RETURN with complex end of pipeline: braces
+
+  pipefailfn2() {
+     emulate -L zsh
+     setopt errreturn pipefail
+     false | if true; then true; fi
+     print "Shouldn't get here, status $?"
+  }
+  pipefailfn2 || print Function failed, as expected
+0:PIPE_FAIL causes ERR_RETURN with complex end of pipeline: if
+>Function failed, as expected
+
+  pipefailfn3() {
+     emulate -L zsh
+     setopt errreturn pipefail
+     false | while true; do break; done
+     print "Shouldn't get here, status $?"
+  }
+  pipefailfn3 || print Function failed, as expected
+0:PIPE_FAIL causes ERR_RETURN with complex end of pipeline: while
+>Function failed, as expected
+
+  pipefailfn4() {
+      emulate -L zsh
+      setopt errreturn pipefail
+      false | true
+      print "Shouldn't get here, status $?"
+  }
+  pipefailfn4
+1:PIPE_FAIL causes ERR_RETURN in simple case
+
+  pipefailfn5() {
+      emulate -L zsh
+      setopt errreturn pipefail
+      false | { true | true; }
+      print "Shouldn't get here, status $?"
+  }
+  pipefailfn5 || print Function failed as expected
+0:PIPE_FAIL causes ERR_RETURN with nested successful pipe
+>Function failed as expected
+
+  pipefailfn6() {
+      emulate -L zsh
+      setopt errreturn pipefail
+      false | { false | true; }
+      print "Shouldn't get here, status $?"
+  }
+  pipefailfn6 || print Function failed as expected
+0:PIPE_FAIL causes ERR_RETURN with nested failed pipe
+>Function failed as expected
+
   for (( i = 0; i < 10; i++ )); do
      () {
         print $i