SIGPIPE (Re: ZSH history not saved anymore)

SIGPIPE (Re: ZSH history not saved anymore)

[Bart Schaefer]

On Sep 27, 10:05am, Tassilo Horn wrote:
} Subject: Re: ZSH history not saved anymore
}
} > Obviously you can work around the problem by telling zsh to trap that
} > signal:
} >
} > TRAPPIPE() {
} >     exit
} > }
} 
} Yes, that works.  So I'll keep that in my ~/.zshrc for the time being.

I'm a bit hesitant to change this after all these years, but perhaps an
interactive shell should exit on SIGPIPE if the terminal is not still open?

I'm probably missing something having to do with subshells receiving the
PIPE signal.  There are regrettably many code paths covering all the ways
an interactive shell might fork.

diff --git a/Src/init.c b/Src/init.c
index 40f46a8..6d005dc 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -1134,6 +1134,7 @@ init_signals(void)
     winch_block();	/* See utils.c:preprompt() */
 #endif
     if (interact) {
+	install_handler(SIGPIPE);
 	install_handler(SIGALRM);
 	signal_ignore(SIGTERM);
     }
diff --git a/Src/signals.c b/Src/signals.c
index cb2b581..094b4b0 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -594,6 +594,17 @@ zhandler(int sig)
 	wait_for_processes();
         break;
  
+    case SIGPIPE:
+	if (!handletrap(SIGPIPE)) {
+	    if (!interact)
+		_exit(SIGPIPE);
+	    else if (!isatty(SHTTY)) {
+		stopmsg = 1;
+		zexit(SIGPIPE, 1);
+	    }
+	}
+	break;
+
     case SIGHUP:
         if (!handletrap(SIGHUP)) {
             stopmsg = 1;

Re: SIGPIPE (Re: ZSH history not saved anymore)

[Peter Stephenson]

On Sat, 27 Sep 2014 10:53:01 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> I'm a bit hesitant to change this after all these years, but perhaps an
> interactive shell should exit on SIGPIPE if the terminal is not still open?

It's hard to see how can that be wrong if we exit on EOF on the terminal.
 
> I'm probably missing something having to do with subshells receiving the
> PIPE signal.  There are regrettably many code paths covering all the ways
> an interactive shell might fork.

I don't know what it is that stops it running zexit() and having the
same effect in a subshell, hence writing out history incorrectly, if
that's what you mean, but you may be thinking of something more subtle.

pws

Re: SIGPIPE (Re: ZSH history not saved anymore)

[Bart Schaefer]

On Sep 27,  9:40pm, Peter Stephenson wrote:
} Subject: Re: SIGPIPE (Re: ZSH history not saved anymore)
}
} On Sat, 27 Sep 2014 10:53:01 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > I'm a bit hesitant to change this after all these years, but perhaps an
} > interactive shell should exit on SIGPIPE if the terminal is not still open?
} 
} It's hard to see how can that be wrong if we exit on EOF on the terminal.

Ideally we'd know what descriptor caused the SIGPIPE and only exit if it
was the terminal.  isatty(SHTTY) is a crude approximation.
 
} > I'm probably missing something having to do with subshells receiving the
} > PIPE signal.
} 
} I don't know what it is that stops it running zexit() and having the
} same effect in a subshell, hence writing out history incorrectly, if
} that's what you mean, but you may be thinking of something more subtle.

Usually a SIGPIPE is generated by a write on a descriptor whose "other
end" is closed.  So I'm wondering if there are cases where a subshell
might get a SIGPIPE on write, in which not only should it not zexit()
but it shouldn't exit at all?

Re: SIGPIPE (Re: ZSH history not saved anymore)

[Bart Schaefer]

On Sep 27,  4:55pm, Bart Schaefer wrote:
}
} Usually a SIGPIPE is generated by a write on a descriptor whose "other
} end" is closed.  So I'm wondering if there are cases where a subshell
} might get a SIGPIPE on write, in which not only should it not zexit()
} but it shouldn't exit at all?

Behavior before patch:

torch% (sleep 5; echo hello; print -u2 continued after PIPE) | (exit)
torch% 

Behavior after patch in 33257:

torch% (sleep 5; echo hello; print -u2 continued after PIPE) | (exit)
echo: write error: broken pipe
zsh: write error: broken pipe
continued after PIPE
torch% 

No history saved in either case, so at least that part is OK, but the
patch obviously breaks something in subshells.

Re: SIGPIPE (Re: ZSH history not saved anymore)

[Peter Stephenson]

On Sun, 28 Sep 2014 11:04:38 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Behavior before patch:
> 
> torch% (sleep 5; echo hello; print -u2 continued after PIPE) | (exit)
> torch% 
> 
> Behavior after patch in 33257:
> 
> torch% (sleep 5; echo hello; print -u2 continued after PIPE) | (exit)
> echo: write error: broken pipe
> zsh: write error: broken pipe
> continued after PIPE
> torch% 
> 
> No history saved in either case, so at least that part is OK, but the
> patch obviously breaks something in subshells.

So in the first case I presume we're exiting (silently) on SIGPIPE.  That
should be just a question of checking if SIGPIPE is trapped and if it isn't
setting the handler to the default in entersubsh().  There's some partial
prior art for this.

pws

Re: SIGPIPE (Re: ZSH history not saved anymore)

[Bart Schaefer]

On Sep 28,  7:18pm, Peter Stephenson wrote:
}
} So in the first case I presume we're exiting (silently) on SIGPIPE.  That
} should be just a question of checking if SIGPIPE is trapped and if it isn't
} setting the handler to the default in entersubsh().  There's some partial
} prior art for this.

It gets a little stranger - this is again before the patch:

torch% TRAPPIPE() { : }                                              
torch% (sleep 5; echo hello; print -u2 $sysparams[pid] continued after PIPE) |
(exit)
TRAPPIPE: write error: broken pipe
echo: write error: broken pipe
zsh: write error: broken pipe
3579 continued after PIPE


Why did TRAPPIPE() receive a SIGPIPE?  And then:

torch% TRAPPIPE() { print -u2 $sysparams[pid] }
torch% (sleep 5; echo hello; print -u2 $sysparams[pid] continued after PIPE) |
(exit)
3659
TRAPPIPE: write error: broken pipe
3659
echo: write error: broken pipe
zsh: write error: broken pipe
3659 continued after PIPE


Note all calls to TRAPPIPE were in the subshell, and that it was called
more than once.

I think the following patch now covers keeping all this weird behavior
the same, while still zexit()'ng if SHTTY is lost.  There may remain
cases where a top-level non-interactive shell now calls _exit(SIGPIPE)
where it did not before, but I'm not sure how to test for those.  Just
doing 'kill -PIPE' of 'zsh -fc "sleep 10"' exits 141 both before and
after the patch below.  TRAPEXIT() has never been called on SIGPIPE.

I'm not entirely happy about examining forklevel in the very last hunk,
but it's the only value passed out of entersubsh() that seems to make
sense for this.  That hunk is restoring zsh's handler when a user-defined
trap is removed; without it, defining and then removing TRAPPIPE would
instead restore the OS default SIGPIPE handling.

I'd be grateful if somebody else could invent a regression test for this,
perhaps in Test/C03traps.ztst ?

diff --git a/Src/exec.c b/Src/exec.c
index fb2739a..fbd3095 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1005,6 +1005,8 @@ entersubsh(int flags)
 	signal_default(SIGTERM);
 	if (!(sigtrapped[SIGINT] & ZSIG_IGNORED))
 	    signal_default(SIGINT);
+	if (!(sigtrapped[SIGPIPE]))
+	    signal_default(SIGPIPE);
     }
     if (!(sigtrapped[SIGQUIT] & ZSIG_IGNORED))
 	signal_default(SIGQUIT);
diff --git a/Src/init.c b/Src/init.c
index 40f46a8..6d005dc 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -1134,6 +1134,7 @@ init_signals(void)
     winch_block();	/* See utils.c:preprompt() */
 #endif
     if (interact) {
+	install_handler(SIGPIPE);
 	install_handler(SIGALRM);
 	signal_ignore(SIGTERM);
     }
diff --git a/Src/signals.c b/Src/signals.c
index cb2b581..84a2609 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -594,6 +594,17 @@ zhandler(int sig)
 	wait_for_processes();
         break;
  
+    case SIGPIPE:
+	if (!handletrap(SIGPIPE)) {
+	    if (!interact)
+		_exit(SIGPIPE);
+	    else if (!isatty(SHTTY)) {
+		stopmsg = 1;
+		zexit(SIGPIPE, 1);
+	    }
+	}
+	break;
+
     case SIGHUP:
         if (!handletrap(SIGHUP)) {
             stopmsg = 1;
@@ -897,6 +908,8 @@ removetrap(int sig)
 	noholdintr();
     } else if (sig == SIGHUP)
         install_handler(sig);
+    else if (sig == SIGPIPE && interact && !forklevel)
+        install_handler(sig);
     else if (sig && sig <= SIGCOUNT &&
 #ifdef SIGWINCH
              sig != SIGWINCH &&

Re: SIGPIPE (Re: ZSH history not saved anymore)

[Peter Stephenson]

On Sun, 28 Sep 2014 13:16:27 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Sep 28,  7:18pm, Peter Stephenson wrote:
> }
> } So in the first case I presume we're exiting (silently) on SIGPIPE.  That
> } should be just a question of checking if SIGPIPE is trapped and if it isn't
> } setting the handler to the default in entersubsh().  There's some partial
> } prior art for this.
> 
> It gets a little stranger - this is again before the patch:
> 
> torch% TRAPPIPE() { : }                                              
> torch% (sleep 5; echo hello; print -u2 $sysparams[pid] continued after PIPE) |
> (exit)
> TRAPPIPE: write error: broken pipe
> echo: write error: broken pipe
> zsh: write error: broken pipe
> 3579 continued after PIPE
> 
> 
> Why did TRAPPIPE() receive a SIGPIPE?  And then:

I think we're looking at this in execcmd() or similar chunks.

		/* It's a builtin */
		if (forked)
		    closem(FDT_INTERNAL);
		lastval = execbuiltin(args, (Builtin) hn);
		fflush(stdout);
		if (save[1] == -2) {
		    if (ferror(stdout)) {
			zwarn("write error: %e", errno);
			clearerr(stdout);
		    }
		} else
		    clearerr(stdout);

so it doesn't necessarily mean there's a signal, just that there's a
broken pipe.  Maybe the fflush() is enough to trigger setting errno
again if it failed to write the first time?

pws

Re: SIGPIPE (Re: ZSH history not saved anymore)

[Bart Schaefer]

On Sep 29,  9:45am, Peter Stephenson wrote:
}
} > Why did TRAPPIPE() receive a SIGPIPE?
} 
} I think we're looking at this in execcmd() or similar chunks.
} 
} 		/* It's a builtin */
} 		if (forked)
} 		    closem(FDT_INTERNAL);
} 		lastval = execbuiltin(args, (Builtin) hn);
} 		fflush(stdout);
} 		if (save[1] == -2) {
} 		    if (ferror(stdout)) {
} 			zwarn("write error: %e", errno);
} 			clearerr(stdout);
} 		    }
} 		} else
} 		    clearerr(stdout);
} 
} so it doesn't necessarily mean there's a signal, just that there's a
} broken pipe.

The zwarn() explains why the output appears more than once, but there
definitely are two calls to TRAPPIPE, so I think that does mean that
there was a signal 2 of the 3 times?

} Maybe the fflush() is enough to trigger setting errno
} again if it failed to write the first time?

The fflush is probably enough to trigger a second SIGPIPE if the shell
didn't exit on the first one.