Skip to content

Commit

Permalink
Fix two issues with $TMOUT
Browse files Browse the repository at this point in the history
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
  • Loading branch information
McDutchie committed Nov 2, 2024
1 parent 83ee566 commit d917f3e
Show file tree
Hide file tree
Showing 7 changed files with 49 additions and 10 deletions.
10 changes: 10 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,16 @@ This documents significant changes in the 1.0 branch of ksh 93u+m.
For full details, see the git log at: https://github.com/ksh93/ksh/tree/1.0
Uppercase BUG_* IDs are shell bug IDs as used by the Modernish shell library.

2024-11-02:

- Fixed: assigning to TMOUT in a virtual subshell was rendering further
assignments to TMOUT outside of that subshell ineffective.

- Fixed: the 'read' built-in was timing out according to the value of TMOUT
even when it was not reading from a terminal. The 'read' command will now
ignore $TMOUT when it is reading from a pipe, as was always documented.
(Note that read's -t option is not affected by this change.)

2024-10-31:

- Fixed a regression that caused a standard output redirection within a block
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/bltins/read.c
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/edit/edit.c
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,10 @@ int tty_check(int fd)
Edit_t *ep = (Edit_t*)(sh.ed_context);
struct termios tty;
Sfio_t *sp;
ep->e_savefd = -1;
if(fd < 0 || fd > sh.lim.open_max || sh.fdstatus[fd] == IOCLOSE
|| (sp = sh.sftable[fd]) && (sfset(sp,0,0) & SFIO_STRING))
return 0;
ep->e_savefd = -1;
return tty_get(fd,&tty)==0;
}

Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/include/version.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

#define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */
#define SH_RELEASE_SVER "1.0.11-beta" /* semantic version number: https://semver.org */
#define SH_RELEASE_DATE "2024-10-31" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_DATE "2024-11-02" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_CPYR "(c) 2020-2024 Contributors to ksh " SH_RELEASE_FORK

/* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */
Expand Down
3 changes: 1 addition & 2 deletions src/cmd/ksh93/sh.1
Original file line number Diff line number Diff line change
Expand Up @@ -9038,10 +9038,9 @@ Unsetting
.BR OPTIND ,
.BR RANDOM ,
.BR SECONDS ,
.BR TMOUT ,
and
.SM
.B _
.B TMOUT
removes their special meaning even if they are
subsequently assigned to.
.TP
Expand Down
3 changes: 3 additions & 0 deletions src/cmd/ksh93/sh/subshell.c
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,9 @@ void sh_assignok(Namval_t *np,int add)
save = sh.subshell;
sh.subshell = 0;
mp->nvname = np->nvname;
/* Copy value pointers for variables whose values are pointers into the static scope, sh.st */
if(!nv_isnonptr(np) && np->nvalue.cp >= (char*)&sh.st && np->nvalue.cp < (char*)&sh.st + sizeof(struct sh_scoped))
mp->nvalue = np->nvalue;
if(nv_isattr(np,NV_NOFREE))
nv_onattr(mp,NV_IDENT);
nv_clone(np,mp,(add?(nv_isnull(np)?0:NV_NOFREE)|NV_ARRAY:NV_MOVE));
Expand Down
37 changes: 32 additions & 5 deletions src/cmd/ksh93/tests/variables.sh
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,28 @@
. "${SHTESTS_COMMON:-${0%/*}/_common}"
((!.sh.level))||err_exit ".sh.level should be 0 after dot script, is ${.sh.level}"

# ======
# The following tests are run in parallel because they are slow; they are checked at the end

# setting TMOUT in a virtual subshell removes its special meaning
# https://github.com/ksh93/ksh/issues/782
(
typeset -i s=SECONDS
"$SHELL" -c 'TMOUT=2; (TMOUT=3); TMOUT=1; read v' </dev/tty
((SECONDS < s + 2))
) &
parallel_1=$!

# TMOUT applies to 'read' from a non-terminal
# https://github.com/ksh93/ksh/issues/783
(
typeset -F s=SECONDS
"$SHELL" -c 'TMOUT=1; { sleep 1.1; print; } | read a'
(($? == 0 || SECONDS > s + 1.05))
) &
parallel_2=$!

# ======
[[ ${.sh.version} == "$KSH_VERSION" ]] || err_exit '.sh.version != KSH_VERSION'
unset ss
[[ ${@ss} ]] && err_exit '${@ss} should be empty string when ss is unset'
Expand Down Expand Up @@ -332,11 +354,11 @@ fi
unset CDPATH
cd "${tmp#/}" >/dev/null 2>&1 && err_exit "CDPATH not deactivated after unset"
cd "$tmp" || exit
TMOUT=100
(TMOUT=20)
if (( TMOUT !=100 ))

if "$SHELL" -c 'TMOUT=100; (TMOUT=20); (( TMOUT != 100 ))'
then err_exit 'setting TMOUT in subshell affects parent'
fi

unset y
function setdisc # var
{
Expand Down Expand Up @@ -1180,8 +1202,8 @@ $SHELL -c '
# ${.sh.pid} should be the PID of the running job
echo ${.sh.pid} > "$tmp/jobpid" &
wait
[[ $(cat "$tmp/jobpid") == ${.sh.pid} ]] && err_exit "\${.sh.pid} is not set to a job's PID (expected $!, got $(cat "$tmp/jobpid"))"
wait "$!"
[[ $(<$tmp/jobpid) == $! ]] || err_exit "\${.sh.pid} is not set to a job's PID (expected $!, got $(<$tmp/jobpid))"
# ${.sh.pid} should be the same as $$ in the parent shell
[[ $$ == ${.sh.pid} ]] || err_exit "\${.sh.pid} and \$$ differ in the parent shell (expected $$, got ${.sh.pid})"
Expand Down Expand Up @@ -1639,5 +1661,10 @@ EOF
(((e=$?)==0)) || err_exit "crash after unsetting SHLVL" \
"(expected status 0, got status $e$( ((e>128)) && print -n /SIG && kill -l "$e"))"
# ======
# checks for tests run in parallel (see top)
wait "$parallel_1" || err_exit 'setting TMOUT in a virtual subshell removes its special meaning'
wait "$parallel_2" || err_exit "TMOUT applies to 'read' from a non-terminal"
# ======
exit $((Errors<125?Errors:125))

0 comments on commit d917f3e

Please sign in to comment.