ksh93 / ksh

ksh 93u+m: KornShell lives! | Latest release: https://github.com/ksh93/ksh/releases
Eclipse Public License 2.0
192 stars 32 forks source link

TMOUT applies to 'read' from a non-terminal #783

Closed sckendall2 closed 3 weeks ago

sckendall2 commented 2 months ago

The ksh man page says that TMOUT applies to "the read built-in command... when input is from a terminal." In context, it is fairly clear that TMOUT should not apply to read when input is not from a terminal.

The following command is reading from a non-terminal, and so should ignore TMOUT. It should take 5 seconds and print 0:

ksh -c 'TMOUT=1; { sleep 5; print; } | read a; print $?'

Instead it takes 1 second and prints 1. I am assuming that ksh is the shell under test.

ksh93 has apparently always behaved this way. I've reproduced this behavior in "Version M 93t+ 2009-05-01" in AIX 7.2, and in "Version AJM 93u+m/1.0.6 2023-06-13", RPM package ksh-1.0.6-3.el9.x86_64 on RHEL 9.3.

One could argue that the bug is in the man page. The remedy would then be to fix the man page to match reality. After all--so this argument goes--it's always behaved this way. And we don't want to break existing scripts that might intentionally use TMOUT to terminate long-running pipelines.

On the other hand, this bug came to my attention because in a customer's environment, TMOUT was 900. Their sysadmins must have put in that setting so that interactive shells would log out when left alone. Instead, deep in a shell script in one of our products was something like the following:

very_long_running_command | { ...; read var; ...; }

The read timed out, of course, leading to a puzzling failure. Yuck. I'd rather TMOUT not affect read from pipes.

Another argument against timeout of read from pipes: buffering in a pipeline can lead to latency and thus to surprising timeouts. I must admit, though, that it took some doing to reproduce this kind of effect. cat and other pipeline-y utilities are surprisingly eager to flush lines that are ready for output.

Note: if you run experiments with TMOUT, run each experiment in a separate, non-interactive shell. This prevents unwanted interaction with https://github.com/ksh93/ksh/issues/782 and with the correct-but-annoying effects of TMOUT in an interactive shell.

McDutchie commented 3 weeks ago

I agree that TMOUT should only apply to read when reading from a terminal, as documented. That's also how bash behaves.

diff --git a/src/cmd/ksh93/bltins/read.c b/src/cmd/ksh93/bltins/read.c
index 3af8ec3fc..30ad63f46 100644
--- a/src/cmd/ksh93/bltins/read.c
+++ b/src/cmd/ksh93/bltins/read.c
@@ -349,6 +349,9 @@ int sh_readline(char **names, volatile int fd, int flags, ssize_t size, Sflong_t
    was_write = (sfset(iop,SFIO_WRITE,0)&SFIO_WRITE)!=0;
    if(fd==0)
        was_share = (sfset(iop,SFIO_SHARE,sh.redir0!=2)&SFIO_SHARE)!=0;
+   /* only time out if standard input is on a terminal */
+   if(timeout && !tty_check(0))
+       timeout = 0;
    if(timeout || (sh.fdstatus[fd]&(IOTTY|IONOSEEK)))
    {
        sh_pushcontext(&buff,1);
McDutchie commented 3 weeks ago

That patch is not correct; it breaks read -t, which should apply both to reading from a terminal and reading from a pipe. E.g., sleep 10 | read -t 1 foo takes 10 seconds to time out but should take one second.

McDutchie commented 3 weeks ago

Patch version two: instead of patching the central sh_readline() function, this patches the default timeout value in b_read(), the builtin command itself. If standard input is not on a terminal, the default timeout is always 0.

diff --git a/src/cmd/ksh93/bltins/read.c b/src/cmd/ksh93/bltins/read.c
index 3af8ec3fc..41f9a13a5 100644
--- a/src/cmd/ksh93/bltins/read.c
+++ b/src/cmd/ksh93/bltins/read.c
@@ -66,7 +66,7 @@ int   b_read(int argc,char *argv[], Shbltin_t *context)
    const char *msg = e_file+4;
    int r, flags=0, fd=0;
    ssize_t len=0;
-   Sflong_t timeout = 1000*(Sflong_t)sh.st.tmout;
+   Sflong_t timeout = sh.st.tmout && tty_check(0) ? 1000*(Sflong_t)sh.st.tmout : 0;
    int save_prompt, fixargs=context->invariant;
    struct read_save *rp;
    static char default_prompt[3] = {ESC,ESC};