Skip to content

Commit

Permalink
Fix race conditions running external commands with job control on
Browse files Browse the repository at this point in the history
When ksh is compiled with SHOPT_SPAWN (the default), which uses
posix_spawn(3) or vfork(2) (via sh_ntfork()) to launch external
commands, at least two race conditions occur when launching
external commands while job control is active. See:
https://bugs.launchpad.net/ubuntu/+source/ksh/+bug/1887863/comments/3
https://www.mail-archive.com/ast-developers@research.att.com/msg00717.html

The basic issue is that this performance optimisation is
incompatible with job control, because it uses a spawning mechanism
that doesn't copy the parent process' memory pages into the child
process, therefore no state that involves memory can be set before
exec-ing the external program. This makes it impossible to
correctly set the terminal's process group ID in the child process,
something that is essential for job control to work.

src/cmd/ksh93/sh/xec.c:
- Use sh_fork() instead of sh_ntfork() if job control is active.
  This uses fork(2), which is 30%-ish slower on most sytems, but
  allows for correctly setting the terminal process group.

src/cmd/ksh93/tests/basic.sh:
- Add regression test for the race condition reported in #79.

src/cmd/INIT/cc.darwin:
- Remove hardcoded flag to disable SHOPT_SPAWN on the Mac.
  It should be safe to use now.

Fixes #79
  • Loading branch information
McDutchie committed Jul 22, 2020
1 parent 4e5f24e commit f207cd5
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 3 deletions.
5 changes: 5 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@ For full details, see the git log at: https://github.com/ksh93/ksh

Any uppercase BUG_* names are modernish shell bug IDs.

2020-07-22:

- Fixed two race conditions when running external commands on
interactive shells with job control active.

2020-07-20:

- If a shell function and a built-in command by the same name exist,
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/INIT/cc.darwin
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ init) echo "cc: arguments expected" >&2
;;
cpp) $CC -E "$@"
;;
cc) $CC -DSHOPT_SPAWN=0 -D_ast_int8_t=int64_t -D_lib_memccpy \
cc) $CC -D_ast_int8_t=int64_t -D_lib_memccpy \
-Wno-unused-value -Wno-parentheses -Wno-macro-redefined "$@"
;;
dll) $CC -Wl,-flat_namespace -dynamiclib -undefined dynamic_lookup "$@"
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 @@ -17,4 +17,4 @@
* David Korn <dgk@research.att.com> *
* *
***********************************************************************/
#define SH_RELEASE "93u+m 2020-07-20"
#define SH_RELEASE "93u+m 2020-07-22"
3 changes: 2 additions & 1 deletion src/cmd/ksh93/sh/xec.c
Original file line number Diff line number Diff line change
Expand Up @@ -1575,7 +1575,7 @@ int sh_exec(register const Shnode_t *t, int flags)
}
#if SHOPT_SPAWN
# ifdef _lib_fork
if(com)
if(com && !job.jobcontrol)
parent = sh_ntfork(shp,t,com,&jobid,ntflag);
else
parent = sh_fork(shp,type,&jobid);
Expand Down Expand Up @@ -3460,6 +3460,7 @@ static void sigreset(Shell_t *shp,int mode)

/*
* A combined fork/exec for systems with slow or non-existent fork()
* Note: Incompatible with job control.
*/
static pid_t sh_ntfork(Shell_t *shp,const Shnode_t *t,char *argv[],int *jobid,int flag)
{
Expand Down
17 changes: 17 additions & 0 deletions src/cmd/ksh93/tests/basic.sh
Original file line number Diff line number Diff line change
Expand Up @@ -598,5 +598,22 @@ eu=$(
)
[[ ${us:1:1} == ${eu:1:1} ]] && err_exit "The time keyword ignores the locale's radix point (both are ${eu:1:1})"
# ======
# Test for bug in ksh binaries that use posix_spawn() while job control is active.
# See discussion at: https://github.com/ksh93/ksh/issues/79
if test -t 1 2>/dev/null 1>/dev/tty # this test only works if we have a tty
then actual=$(
"$SHELL" -i <<-\EOF 2>/dev/tty
printf '%s\n' 1 2 3 4 5 | while read
do ls /dev/null
done 2>&1
exit # suppress extra newline
EOF
)
expect=$'/dev/null\n/dev/null\n/dev/null\n/dev/null\n/dev/null'
[[ $actual == "$expect" ]] || err_exit 'Race condition while launching external commands' \
"(expected $(printf %q "$expect"), got $(printf %q "$actual"))"
fi
# ======
exit $((Errors<125?Errors:125))

0 comments on commit f207cd5

Please sign in to comment.