Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
sckendall2 opened this issue Sep 11, 2024 · 3 comments
Closed

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

sckendall2 opened this issue Sep 11, 2024 · 3 comments
Labels
bug Something is not working

Comments

@sckendall2
Copy link

sckendall2 commented Sep 11, 2024

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 #782 and with the correct-but-annoying effects of TMOUT in an interactive shell.

@McDutchie McDutchie added the bug Something is not working label Nov 1, 2024
@McDutchie
Copy link

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
Copy link

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
Copy link

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};

McDutchie added a commit that referenced this issue Nov 2, 2024
Reproducer 1:

   ksh -c '( TMOUT=0 ); TMOUT=1; read v; print $?'

Actual behaviour: waits for input indefinitely.
Expected behaviour: one second delay, then output of exit status 1.

Analysis: The value of TMOUT is initialised as a pointer into the
static scope struct, sh.st (see init.c line 1918). This struct is
saved and restored for virtual subshells and ksh functions (so the
effect of TMOUT is always local to a ksh function). However, an
assignment in a virtual subshell changes the value pointer to a
newly allocated value node, so even when the value is restored at
the end of the subshell, assigning to it will no longer modify
sh.st.tmout.

Reproducer 2:

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

Actual behaviour: one second delay, then output of exit status 1.
Expected behaviour: since 'read' is reading from a pipe, TMOUT
should be ignored, so the reproducer should take 5 seconds and
print exit status 0. The manual page has always stated that TMOUT
applies to the 'read' command "if a line is not entered within the
prescribed number of seconds while reading from a terminal." The
Bolsky & Korn book is silent on the matter.

While 'read' in ksh has always timed out according to TMOUT even
when reading from a pipe, the documentation has also always
specified that it only times out when reading from a terminal. The
behaviour described in the documentation makes a lot more sense,
because an unexpected TMOUT value inherited from the environment
may easily break scripts that use 'read' to read from slow commands
via a pipe. (This in fact happened, as per @sckendall2's report).

It is also possible that there are some scripts that depend on the
current behaviour, but my estimation is that fixing this will solve
more problems than it will cause.

src/cmd/ksh93/sh/subshell.c: sh_assignok():
- When cloning a node for the subshell, check if the address of its
  value pointer is within the sh.st struct. If so, copy the value
  pointer to the cloned node, preserving the special meaning of the
  variable. (Note that the sh.st struct is integreally saved and
  restored for virtual subshells by sh_subshell(), so this change
  does not introduce a subshell leak.)

src/cmd/ksh93/bltins/read.c: b_read():
- Only honour $TMOUT (sh.st.tmout) if standard input (0) is on a
  terminal (tty_check(0)==1). This fixes reproducer 2.

src/cmd/ksh93/sh.1:
- Fix documentation: unsetting '_' does not actually remove the
  special meaning of $_. Thanks to @sckendall2 for noticing.

src/cmd/ksh93/edit/edit.c: tty_check():
- Minor tweak. We should only set e_savefd to -1 before calling
  tty_get(), to force it to call tcgetattr(3). If we return without
  calling tty_get(), keep the saved fd. (re: 53f4bc6)

Thanks to @sckendall2 for the reports.
Resolves: #782
Resolves: #783
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working
Projects
None yet
Development

No branches or pull requests

2 participants