Bug: reading from tty inside process substitution

Bug: reading from tty inside process substitution

[Mark J. Reed]

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

The below code hangs after reading the line from the terminal:

    read foo < <(read bar </dev/tty; echo $bar)

The actual use case I ran into this involved a select, which also hangs
after item selection:

    read foo < <(select item in a b c; do break; done </dev/tty >/dev/tty
2>&1; echo $item)

Both commands work as intended in both bash and ksh.

If I replace the outer `read` with a command substitution, it works in zsh
as well (it started out as `read` because I was getting multiple values out
of the `select`ed line).

I'm running zsh 5.9, tested on macOS 13.6 and 14.0 and Ubuntu 22.04.3
-- 
Mark J. Reed <markjreed@gmail.com>

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

Re: Bug: reading from tty inside process substitution

[Bart Schaefer]

On Sun, Nov 12, 2023 at 7:59 AM Mark J. Reed <markjreed@gmail.com> wrote:
>
> The below code hangs after reading the line from the terminal:
>
>     read foo < <(read bar </dev/tty; echo $bar)

I believe it actually hangs before reading the line from the terminal,
or more precisely at the point of trying to read the line.  "read bar"
is being stopped by a SIGTTIN signal because it is not in a process
group that "owns" the TTY.

This has to do with the way zsh backgrounds the process substitution.
It actually works fine in a script or even in a subshell (which
explains why it works in command substitution):

% zsh -fc 'read foo < <(read bar </dev/tty; echo $bar); echo $foo'
the line
the line
%  ( read foo < <(read bar </dev/tty; echo $bar); echo $foo )
subshell
subshell
%

In an interactive shell with job control, though, the process
management is different.

% unsetopt monitor
% read foo < <(read bar </dev/tty; echo $bar); echo $foo
no job control
no job control
%

I doubt there's any easy way to change this without breaking something else.

Re: Bug: reading from tty inside process substitution

[Bart Schaefer]

On Sun, Nov 12, 2023 at 10:09 AM Bart Schaefer
<schaefer@brasslantern.com> wrote:
>
> I doubt there's any easy way to change this without breaking something else.

This is even worse:

read foo < <(read bar; echo $bar) </dev/tty

With MULTIOS, that command becomes un-killable, because "read foo"
sees a pipeline consisting of a catenation of both the process
substitution and the tty, but the process substitution is perpetually
TTIN'd and the interactive shell is ignoring interrupts.  Without
MULTIOS, the command substitution simply becomes a stopped zombie and
"read foo" sees only /dev/tty.  Without the </dev/tty, it gets stuck
but interrupt is possible, leaving behind a zombie.

Re: Bug: reading from tty inside process substitution

[Bart Schaefer]

On Sun, Nov 12, 2023 at 10:09 AM Bart Schaefer
<schaefer@brasslantern.com> wrote:
>
> On Sun, Nov 12, 2023 at 7:59 AM Mark J. Reed <markjreed@gmail.com> wrote:
> >
> > The below code hangs after reading the line from the terminal:
> >
> >     read foo < <(read bar </dev/tty; echo $bar)
>
> I believe it actually hangs before reading the line from the terminal,
> or more precisely at the point of trying to read the line.  "read bar"
> is being stopped by a SIGTTIN signal because it is not in a process
> group that "owns" the TTY.
>
> I doubt there's any easy way to change this without breaking something else.

I seem to have proven myself wrong.  Can anyone think of a use-case
for doing job control inside a <<(subshell) ?

On Sun, Nov 12, 2023 at 1:50 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> This is even worse:
>
> read foo < <(read bar; echo $bar) </dev/tty

That still doesn't respond to interrupts, but with the below change
will stop on ctlr-d (EOF) on the tty.


diff --git a/Src/exec.c b/Src/exec.c
index 285d2c5ad..f4a71fd03 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -5096,7 +5096,7 @@ getpipe(char *cmd, int nullexec)
        procsubstpid = pid;
        return pipes[!out];
     }
-    entersubsh(ESUB_ASYNC|ESUB_PGRP, NULL);
+    entersubsh(ESUB_ASYNC|ESUB_PGRP|ESUB_NOMONITOR, NULL);
     redup(pipes[out], out);
     closem(FDT_UNUSED, 0);     /* this closes pipes[!out] as well */
     cmdpush(CS_CMDSUBST);

Re: Bug: reading from tty inside process substitution

[Bart Schaefer]

On Tue, Nov 14, 2023 at 7:33 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> > read foo < <(read bar; echo $bar) </dev/tty
>
> That still doesn't respond to interrupts

So what seems to be going on here is that the shell enters
waitonejob() via waitjobs() from execpline().  At that point it has a
nonzero auxprocs pointer, so it calls zwaitjob() which for the same
reason eventually calls signal_suspend(SIGCHLD), which adds SIGINT to
the ignored signals to allow the presumed foreground child to handle
the signal.

The problem is that by that point the auxiliary "foreground" process
-- the process substitution -- has already exited.  It was promptly
reaped, but zwaitjob() is in a loop waiting with SIGINT still blocked
for the parent's jn->stat to be updated to either 0 or STAT_DONE.
Trouble is, that parent is the multio loop, which is actively skipping
over interrupts and won't quit until EOF.

What about the following?  Is there some simple way to check that the
multio tty is the same tty as SHIN ?  Also, even with this in place,
something is still ignoring TSTP (^z), perhaps because "read foo" is
in the top-level interactive shell, so this can't be suspended.

diff --git a/Src/exec.c b/Src/exec.c
index 285d2c5ad..97823760f 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2309,7 +2309,7 @@ closemn(struct multio **mfds, int fd, int type)
         for (i = 0; i < mn->ct; i++)
         while ((len = read(mn->fds[i], buf, TCBUFSIZE)) != 0) {
             if (len < 0) {
-            if (errno == EINTR)
+            if (errno == EINTR && !isatty(mn->fds[i]))
                 continue;
             else
                 break;

(This patch may not apply cleanly due to gmail ... I'll prepare a
proper one if deemed OK.)

Re: Bug: reading from tty inside process substitution

[Bart Schaefer]

A couple of other things about this ...

On Tue, Nov 14, 2023 at 8:42 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> On Tue, Nov 14, 2023 at 7:33 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
> >
> > > read foo < <(read bar; echo $bar) </dev/tty

The input of the process substitution is closed, so "read bar" does
nothing and "echo $bar" just prints a newline, which satisfies "read
foo".

However, the multio loop is still reading /dev/tty and won't discover
until its I/O buffers fill up that "read foo" isn't consuming any more
input.

> something is still ignoring TSTP (^z), perhaps because "read foo" is
> in the top-level interactive shell, so this can't be suspended.

The use of ESUB_NOMONITOR in the previous patch causes the multio loop
to ignore TSTP, so maybe there's another tweak to be done there, but I
think the reason the whole thing is not stop-able may be because "read
foo" has already finished.

[PATCH] reading from tty inside process substitution

[Bart Schaefer]

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

On Tue, Nov 14, 2023 at 8:42 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> (This patch may not apply cleanly due to gmail ... I'll prepare a
> proper one if deemed OK.)

Taking silence as assent ...

[-- Attachment #2: procsubs-tty.txt --]
[-- Type: text/plain, Size: 737 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index 285d2c5ad..97823760f 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2309,7 +2309,7 @@ closemn(struct multio **mfds, int fd, int type)
 	    for (i = 0; i < mn->ct; i++)
 		while ((len = read(mn->fds[i], buf, TCBUFSIZE)) != 0) {
 		    if (len < 0) {
-			if (errno == EINTR)
+			if (errno == EINTR && !isatty(mn->fds[i]))
 			    continue;
 			else
 			    break;
@@ -5096,7 +5096,7 @@ getpipe(char *cmd, int nullexec)
 	procsubstpid = pid;
 	return pipes[!out];
     }
-    entersubsh(ESUB_ASYNC|ESUB_PGRP, NULL);
+    entersubsh(ESUB_ASYNC|ESUB_PGRP|ESUB_NOMONITOR, NULL);
     redup(pipes[out], out);
     closem(FDT_UNUSED, 0);	/* this closes pipes[!out] as well */
     cmdpush(CS_CMDSUBST);

Re: Bug: reading from tty inside process substitution

[Bart Schaefer]

I've committed the patch so far, but another case occurred to me.

On Tue, Nov 14, 2023 at 9:00 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> > On Tue, Nov 14, 2023 at 7:33 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
> > >
> > > > read foo < <(read bar; echo $bar) </dev/tty
>
> ... the multio loop is still reading /dev/tty and won't discover
> until its I/O buffers fill up that "read foo" isn't consuming any more
> input.

Replace /dev/tty with /dev/zero and we're back to the unkillable loop.

It's been this way since workers/12222 in July 2000.  The bug report
to which that was a response had to do with a write loop (emulating
"tee" to multiple outputs).  Do we actually need to discard EINTR on
the read loop, or did that just get put in for symmetry?

[PATCH] reading from large or "infinite" source in multio

[Bart Schaefer]

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

On Wed, Nov 15, 2023 at 8:58 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> > > > > read foo < <(read bar; echo $bar) </dev/tty
> >
> > ... the multio loop is still reading /dev/tty and won't discover
> > until its I/O buffers fill up that "read foo" isn't consuming any more
> > input.

I fact it never notices then, either.

> Replace /dev/tty with /dev/zero and we're back to the unkillable loop.

Why not this?  Any reason that errors from write_loop() should be ignored?

[-- Attachment #2: procsubs-write_loop.txt --]
[-- Type: text/plain, Size: 588 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index 97823760f..1bbefd985 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2302,7 +2302,8 @@ closemn(struct multio **mfds, int fd, int type)
 			break;
 		}
 		for (i = 0; i < mn->ct; i++)
-		    write_loop(mn->fds[i], buf, len);
+		    if (write_loop(mn->fds[i], buf, len) < 0)
+			break;
 	    }
 	} else {
 	    /* cat process */
@@ -2314,7 +2315,8 @@ closemn(struct multio **mfds, int fd, int type)
 			else
 			    break;
 		    }
-		    write_loop(mn->pipe, buf, len);
+		    if (write_loop(mn->pipe, buf, len) < 0)
+			break;
 		}
 	}
 	_exit(0);

Re: [PATCH] reading from large or "infinite" source in multio

[Bart Schaefer]

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

On Thu, Nov 16, 2023 at 5:27 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> Why not this?  Any reason that errors from write_loop() should be ignored?

Small addition to that patch:  There's no good reason that the tee/cat
loops should act like interactive shells for purposes of signal
handling etc.

The error checks on the return from write_loop() appear less necessary
once dont_queue_signals() is asserted, but seem prudent anyway.

[-- Attachment #2: procsubs-write_loop.txt --]
[-- Type: text/plain, Size: 800 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index 97823760f..7d8135266 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2290,6 +2290,8 @@ closemn(struct multio **mfds, int fd, int type)
 	    return;
 	}
 	/* pid == 0 */
+	opts[INTERACTIVE] = 0;
+	dont_queue_signals();
 	child_unblock();
 	closeallelse(mn);
 	if (mn->rflag) {
@@ -2302,7 +2304,8 @@ closemn(struct multio **mfds, int fd, int type)
 			break;
 		}
 		for (i = 0; i < mn->ct; i++)
-		    write_loop(mn->fds[i], buf, len);
+		    if (write_loop(mn->fds[i], buf, len) < 0)
+			break;
 	    }
 	} else {
 	    /* cat process */
@@ -2314,7 +2317,8 @@ closemn(struct multio **mfds, int fd, int type)
 			else
 			    break;
 		    }
-		    write_loop(mn->pipe, buf, len);
+		    if (write_loop(mn->pipe, buf, len) < 0)
+			break;
 		}
 	}
 	_exit(0);