Skip to content

Commit

Permalink
Fix hang in comsub on redirecting stdout & more (re: 2d4a787)
Browse files Browse the repository at this point in the history
The referenced commit introduced a hang (writes infinite '\0' or
garbage output) upon the following combination of a comsub and
redirections, including a redirection of standard output:

	{ v=$(redirect 2>&1 1>&9); } 9>&1

See the commits referenced by the referenced commit (and so on) for
more background info.

This is a rarely triggered but serious bug. One thing that was
definitely affected is the sys/base/seq module in the modernish
shell library.

src/cmd/ksh93/sh/xec.c,
src/cmd/ksh93/sh/io.c:
- Move the comsub forking workaround back from sh_exec() to
  sh_redirect() so it applies to all commands, not just builtins.
  Also apply it for both local and permanent redirects. Seems it's
  the only way to be safe.

src/cmd/ksh93/tests/subshell.sh:
- Introducing the forking workaround caused what I believe to be a
  spurious failure in a test inherited from AT&T; the test itself
  works just fine. I think this may expose an issue with the 'kill
  -0' built-in command. Running 'sleep' as an external instead of
  built-in command works around the problem. This is for
  investigating another time.
- Add new test for the bug fixed here.
  • Loading branch information
McDutchie committed Sep 15, 2023
1 parent 3eedd07 commit 8628b6b
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 21 deletions.
7 changes: 7 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@ 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.

2023-09-15:

- Fixed a hang in command substitutions (introduced in 93u+m/1.0.0) that was
triggered when redirecting standard output within a command substitution,
in combination with other factors. E.g., the following no longer hangs:
{ v=$(redirect 2>&1 1>&9); } 9>&1

2023-09-13:

- Fixed a crash on trying to append an indexed array value to an unset name
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.7-alpha" /* semantic version number: https://semver.org */
#define SH_RELEASE_DATE "2023-09-13" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_DATE "2023-09-15" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_CPYR "(c) 2020-2023 Contributors to ksh " SH_RELEASE_FORK

/* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */
Expand Down
18 changes: 18 additions & 0 deletions src/cmd/ksh93/sh/io.c
Original file line number Diff line number Diff line change
Expand Up @@ -1139,6 +1139,24 @@ int sh_redirect(struct ionod *iop, int flag)
{
iof=iop->iofile;
fn = (iof&IOUFD);
/*
* A command substitution will hang on exit, writing infinite '\0', if,
* within it, standard output (FD 1) is redirected for a built-in command
* that calls sh_subfork(), or redirected permanently using 'exec' or
* 'redirect'. This forking workaround is necessary to avoid that bug.
* For shared-state comsubs, forking is incorrect, so error out then.
* TODO: actually fix the bug and remove this workaround.
*/
if(fn==1 && sh.subshell && sh.comsub)
{
if(!sh.subshare)
sh_subfork();
else if(flag==1 || flag==2) /* block stdout perma-redirects: would hang */
{
errormsg(SH_DICT,ERROR_exit(1),"cannot redirect stdout inside shared-state comsub");
UNREACHABLE();
}
}
if(sh.redir0 && fn==0 && !(iof&IOMOV))
sh.redir0 = 2;
io_op[0] = '0'+(iof&IOUFD);
Expand Down
19 changes: 0 additions & 19 deletions src/cmd/ksh93/sh/xec.c
Original file line number Diff line number Diff line change
Expand Up @@ -1182,25 +1182,6 @@ int sh_exec(const Shnode_t *t, int flags)
}
else
type = (execflg && !sh.subshell && !sh.st.trapcom[0]);
/*
* A command substitution will hang on exit, writing infinite '\0', if,
* within it, standard output (FD 1) is redirected for a built-in command
* that calls sh_subfork(), or redirected permanently using 'exec' or
* 'redirect'. This forking workaround is necessary to avoid that bug.
* For shared-state comsubs, forking is incorrect, so error out then.
* TODO: actually fix the bug and remove this workaround.
*/
if((io->iofile & IOUFD)==1 && sh.subshell && sh.comsub)
{
if(!sh.subshare)
sh_subfork();
else if(type==2) /* block stdout perma-redirects: would hang */
{
errormsg(SH_DICT,ERROR_exit(1),"cannot redirect stdout"
" inside shared-state comsub");
UNREACHABLE();
}
}
sh.redir0 = 1;
sh_redirect(io,type);
for(item=buffp->olist;item;item=item->next)
Expand Down
14 changes: 13 additions & 1 deletion src/cmd/ksh93/tests/subshell.sh
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ trap - USR1 ERR
dot=$(cat <<-EOF
$(ls -d .)
EOF
) ) & sleep 1
) ) & "$binsleep" .1
if kill -0 $! 2> /dev/null
then err_exit 'command substitution containing here-doc with command substitution fails'
fi
Expand Down Expand Up @@ -1183,5 +1183,17 @@ exp='some output'
[[ $got == "$exp" ]] || err_exit 'command substitution did not catch output' \
"(expected $(printf %q "$exp"), got $(printf %q "$got"))"

# ======
# Command substitution hangs when redirecting standard output in combination with other redirections
# Bug introduced in 93u+m/1.0.0-beta.2
"$SHELL" -c '{ v=$(redirect 2>&1 1>&9); } 9>&1' &
test_pid=$!
(sleep 2; kill -s KILL "$test_pid" 2>/dev/null) &
sleep_pid=$!
{ wait "$test_pid"; } 2>/dev/null
((!(e = $?))) || err_exit "comsub hangs on redirecting stdout & more" \
"(got status $e$( ((e>128)) && print -n /SIG && kill -l "$e"))"
kill "$sleep_pid" 2>/dev/null

# ======
exit $((Errors<125?Errors:125))

0 comments on commit 8628b6b

Please sign in to comment.