Skip to content

Commit

Permalink
Avoid race condition in comsub redirection (re: 8628b6b, 2d4a787)
Browse files Browse the repository at this point in the history
Turns out that running sh_subfork() when redirecting the output
of external commands in command substitutions triggers a race
condition on some systems. So we need to limit the workaround to
built-ins, without reintroducing previous problems.

src/cmd/ksh93/sh/io.c: sh_redirect():
- To limit the workaround, use the sh.redir0 flag set in xec.c --
  it's for an optimisation of the 'read' command (introduced in ksh
  93u+ 2011-12-24), but we can use the fact that xec.c sets it to 1
  for any built-in command right before calling sh_redirect(). This
  covers permanent redirections with 'exec' and 'redirect' as well.
- Check all the redirections for need to fork, in a separate loop.

Thanks to @phidebian for the report.

Resolves: #688
Resolves: #690
  • Loading branch information
McDutchie committed Dec 28, 2023
1 parent 7ad3dfb commit b1e514c
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 15 deletions.
6 changes: 6 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@ 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-12-28:

- Fixed intermittent incorrect behaviour (a race condition), introduced on
2023-09-15, that occurred on some systems when running an external command
with a redirection from a command substitution.

2023-12-25:

- Fixed a regression in the behavior of 'exit' in a trap action, introduced
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.8-beta" /* semantic version number: https://semver.org */
#define SH_RELEASE_DATE "2023-12-25" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_DATE "2023-12-28" /* 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
39 changes: 25 additions & 14 deletions src/cmd/ksh93/sh/io.c
Original file line number Diff line number Diff line change
Expand Up @@ -1135,28 +1135,39 @@ int sh_redirect(struct ionod *iop, int flag)
clexec = 1;
if(iop)
traceon = sh_trace(NULL,0);
for(;iop;iop=iop->ionxt)
{
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)
/*
* 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.
* (Note that sh.redir0 is set to 1 in xec.c immediately before processing
* redirections for any built-in command, including 'exec' and 'redirect'.)
*/
if(sh.subshell && sh.comsub && sh.redir0==1)
{
struct ionod *i;
for(i = iop; i; i = i->ionxt)
{
if((i->iofile & IOUFD) != 1)
continue;
if(!sh.subshare)
{
sh_subfork();
else if(flag==1 || flag==2) /* block stdout perma-redirects: would hang */
break;
}
if(flag==1 || flag==2)
{
errormsg(SH_DICT,ERROR_exit(1),"cannot redirect stdout inside shared-state comsub");
UNREACHABLE();
}
}
}
for(; iop; iop = iop->ionxt)
{
iof = iop->iofile;
fn = (iof & IOUFD);
if(sh.redir0 && fn==0 && !(iof&IOMOV))
sh.redir0 = 2;
io_op[0] = '0'+(iof&IOUFD);
Expand Down

0 comments on commit b1e514c

Please sign in to comment.