From a2196f9434746996c2f3e9c34013c7e233186d22 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Fri, 13 Aug 2021 08:01:46 +0200 Subject: [PATCH] Fix `backtick` comsubs by making them act like $(modern) ones ksh93 currently has three command substitution mechanisms: - type 1: old-style backtick comsubs that use a pipe; - type 3: $(modern) comsubs that use a temp file, currently with fallback to a pipe if a temp file cannot be created; - type 2: ${ shared-state; } comsubs; same as type 3, but shares state with parent environment. Type 1 is buggy. There are at least two reproducers that make it hang. The Red Hat patch applied in 4ce486a7 fixed a hang in backtick comsubs but reintroduced another hang that was fixed in ksh 93v-. So far, no one has succeeded in making pipe-based comsubs work properly. But, modern (type 3) comsubs use temp files. How does it make any sense to have two different command substitution mechanisms at the execution level? The specified functionality between backtick and modern command substitutions is exactly the same; the difference *should* be purely syntactic. So this commit removes the type 1 comsub code at the execution level, treating them all like type 3 (or 2). As a result, the related bugs vanish while the regression tests all pass. The only side effect that I can find is that the behaviour of bug https://github.com/ksh93/ksh/issues/124 changes for backtick comsubs. But it's broken either way, so that's neutral. So this commit can now be added to my growing list of ksh93 issues fixed by simply removing code. src/cmd/ksh93/sh/xec.c: - Remove special code for type 1 comsubs from iousepipe(), sh_iounpipe(), sh_exec() and _sh_fork(). src/cmd/ksh93/include/defs.h, src/cmd/ksh93/sh/subshell.c: - Remove pipe support from sh_subtmpfile(). This also removes the use of a pipe as a fallback for $(modern) comsubs. Instead, panic and error out if temp file creation fails. If the shell cannot create a temporary file, there are fatal system problems anyway and a script should not continue. - No longer pass comsub type to sh_subtmpfile(). All other changes: - Update sh_subtmpfile() calls. src/cmd/ksh93/tests/subshell.sh: - Add two regression tests based on reproducers from bug reports. Resolves: https://github.com/ksh93/ksh/issues/305 Resolves: https://github.com/ksh93/ksh/issues/316 --- NEWS | 5 ++ src/cmd/ksh93/include/defs.h | 2 +- src/cmd/ksh93/include/jobs.h | 1 - src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/sh/args.c | 2 +- src/cmd/ksh93/sh/io.c | 4 +- src/cmd/ksh93/sh/subshell.c | 55 +++++----------- src/cmd/ksh93/sh/xec.c | 107 +++++--------------------------- src/cmd/ksh93/tests/subshell.sh | 35 +++++++++++ 9 files changed, 74 insertions(+), 139 deletions(-) diff --git a/NEWS b/NEWS index b6a7282ee..cd493ddf3 100644 --- a/NEWS +++ b/NEWS @@ -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. +2021-08-13: + +- An issue was fixed that could cause old-style `backtick` command + substitutions to hang in certain cases. + 2021-06-03: - Fixed a bug in the [[ compound command: the '!' logical negation operator diff --git a/src/cmd/ksh93/include/defs.h b/src/cmd/ksh93/include/defs.h index 50def36b3..0e7698e24 100644 --- a/src/cmd/ksh93/include/defs.h +++ b/src/cmd/ksh93/include/defs.h @@ -381,7 +381,7 @@ extern Dt_t *sh_subtracktree(int); extern Dt_t *sh_subfuntree(int); extern void sh_subjobcheck(pid_t); extern int sh_subsavefd(int); -extern void sh_subtmpfile(char); +extern void sh_subtmpfile(Shell_t*); extern char *sh_substitute(const char*,const char*,char*); extern void sh_timetraps(Shell_t*); extern const char *_sh_translate(const char*); diff --git a/src/cmd/ksh93/include/jobs.h b/src/cmd/ksh93/include/jobs.h index c2ed67715..b9dabac35 100644 --- a/src/cmd/ksh93/include/jobs.h +++ b/src/cmd/ksh93/include/jobs.h @@ -100,7 +100,6 @@ struct jobs char jobcontrol; /* turned on for real job control */ char waitsafe; /* wait will not block */ char waitall; /* wait for all jobs in pipe */ - char bktick_waitall; /* wait state for `backtick comsubs` */ char toclear; /* job table needs clearing */ unsigned char *freejobs; /* free jobs numbers */ }; diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index ff36c716e..c6a458881 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -21,7 +21,7 @@ #define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */ #define SH_RELEASE_SVER "1.0.0-beta.2" /* semantic version number: https://semver.org */ -#define SH_RELEASE_DATE "2021-06-03" /* must be in this format for $((.sh.version)) */ +#define SH_RELEASE_DATE "2021-08-13" /* must be in this format for $((.sh.version)) */ #define SH_RELEASE_CPYR "(c) 2020-2021 Contributors to ksh " SH_RELEASE_FORK /* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */ diff --git a/src/cmd/ksh93/sh/args.c b/src/cmd/ksh93/sh/args.c index 507dc82ec..a0a28aefe 100644 --- a/src/cmd/ksh93/sh/args.c +++ b/src/cmd/ksh93/sh/args.c @@ -735,7 +735,7 @@ struct argnod *sh_argprocsub(Shell_t *shp,struct argnod *argp) ap->argflag &= ~ARG_RAW; fd = argp->argflag&ARG_RAW; if(fd==0 && shp->subshell) - sh_subtmpfile(shp->comsub); + sh_subtmpfile(shp); #if SHOPT_DEVFD sfwrite(shp->stk,e_devfdNN,8); pv[2] = 0; diff --git a/src/cmd/ksh93/sh/io.c b/src/cmd/ksh93/sh/io.c index 30b687d73..47fb9d63e 100644 --- a/src/cmd/ksh93/sh/io.c +++ b/src/cmd/ksh93/sh/io.c @@ -1194,7 +1194,7 @@ int sh_redirect(Shell_t *shp,struct ionod *iop, int flag) if(iof&IOPUT) ap->argflag = ARG_RAW; else if(shp->subshell) - sh_subtmpfile(shp->comsub); + sh_subtmpfile(shp); ap->argchn.ap = (struct argnod*)fname; ap = sh_argprocsub(shp,ap); fname = ap->argval; @@ -1262,7 +1262,7 @@ int sh_redirect(Shell_t *shp,struct ionod *iop, int flag) if(shp->subshell && dupfd==1) { if(sfset(sfstdout,0,0)&SF_STRING) - sh_subtmpfile(shp->comsub); + sh_subtmpfile(shp); if(shp->comsub==1) shp->subdup |= 1<jmplist; register struct subshell *sp = subshell_data->pipe; @@ -136,40 +134,21 @@ void sh_subtmpfile(char comsub_flag) UNREACHABLE(); } /* popping a discipline forces a /tmp file create */ - if(comsub_flag != 1) - sfdisc(sfstdout,SF_POPDISC); + sfdisc(sfstdout,SF_POPDISC); if((fd=sffileno(sfstdout))<0) { - /* unable to create the /tmp file so use a pipe */ - int fds[3]; - Sfoff_t off; - fds[2] = 0; - sh_pipe(fds); - sp->pipefd = fds[0]; - sh_fcntl(sp->pipefd,F_SETFD,FD_CLOEXEC); - /* write the data to the pipe */ - if(off = sftell(sfstdout)) - write(fds[1],sfsetbuf(sfstdout,(Void_t*)sfstdout,0),(size_t)off); - sfclose(sfstdout); - if((sh_fcntl(fds[1],F_DUPFD, 1)) != 1) - { - errormsg(SH_DICT,ERROR_system(1),e_file+4); - UNREACHABLE(); - } - sh_close(fds[1]); + errormsg(SH_DICT,ERROR_SYSTEM|ERROR_PANIC,"could not create temp file"); + UNREACHABLE(); } + shp->fdstatus[fd] = IOREAD|IOWRITE; + sfsync(sfstdout); + if(fd==1) + fcntl(1,F_SETFD,0); else { - shp->fdstatus[fd] = IOREAD|IOWRITE; - sfsync(sfstdout); - if(fd==1) - fcntl(1,F_SETFD,0); - else - { - sfsetfd(sfstdout,1); - shp->fdstatus[1] = shp->fdstatus[fd]; - shp->fdstatus[fd] = IOCLOSE; - } + sfsetfd(sfstdout,1); + shp->fdstatus[1] = shp->fdstatus[fd]; + shp->fdstatus[fd] = IOCLOSE; } sh_iostream(shp,1); sfset(sfstdout,SF_SHARE|SF_PUBLIC,1); @@ -197,7 +176,7 @@ void sh_subfork(void) trap = sh_strdup(trap); /* see whether inside $(...) */ if(sp->pipe) - sh_subtmpfile(shp->comsub); + sh_subtmpfile(shp); shp->curenv = 0; shp->savesig = -1; if(pid = sh_fork(shp,FSHOWME,NIL(int*))) @@ -537,10 +516,7 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) if(!shp->subshare) sp->pathlist = path_dup((Pathcomp_t*)shp->pathlist); if(comsub) - { shp->comsub = comsub; - job.bktick_waitall = (comsub==1); - } if(!shp->subshare) { struct subshell *xp; @@ -704,8 +680,7 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) } else { - job.bktick_waitall = 0; - if(comsub!=1 && shp->spid) + if(shp->spid) { int e = shp->exitval; job_wait(shp->spid); @@ -900,7 +875,7 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) if(sp->subpid) { job_wait(sp->subpid); - if(comsub>1) + if(comsub) sh_iounpipe(shp); } shp->comsub = sp->comsub; diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c index c3df0bee4..1a9898156 100644 --- a/src/cmd/ksh93/sh/xec.c +++ b/src/cmd/ksh93/sh/xec.c @@ -184,29 +184,8 @@ static int iousepipe(Shell_t *shp) if(sh_rpipe(subpipe) < 0) return(0); usepipe++; - if(shp->comsub!=1) - { - subpipe[2] = sh_fcntl(subpipe[1],F_DUPFD,10); - sh_close(subpipe[1]); - return(1); - } - subpipe[2] = sh_fcntl(fd,F_dupfd_cloexec,10); - sh_iovalidfd(shp,subpipe[2]); - shp->fdstatus[subpipe[2]] = shp->fdstatus[1]; - while(close(fd)<0 && errno==EINTR) - errno = err; - fcntl(subpipe[1],F_DUPFD,fd); - shp->fdstatus[1] = shp->fdstatus[subpipe[1]]&~IOCLEX; + subpipe[2] = sh_fcntl(subpipe[1],F_DUPFD,10); sh_close(subpipe[1]); - if(subdup=shp->subdup) for(i=0; i < 10; i++) - { - if(subdup&(1<fdstatus[i] = shp->fdstatus[1]; - } - } return(1); } @@ -217,48 +196,9 @@ void sh_iounpipe(Shell_t *shp) if(!usepipe) return; --usepipe; - if(shp->comsub>1) - { - sh_close(subpipe[2]); - while(read(subpipe[0],buff,sizeof(buff))>0); - goto done; - } - while(close(fd)<0 && errno==EINTR) - errno = err; - fcntl(subpipe[2], F_DUPFD, fd); - shp->fdstatus[1] = shp->fdstatus[subpipe[2]]; - if(subdup) for(n=0; n < 10; n++) - { - if(subdup&(1<fdstatus[n] = shp->fdstatus[1]; - } - } - shp->subdup = 0; sh_close(subpipe[2]); - if(usepipe==0) while(1) - { - while(job.waitsafe && job.savesig==SIGCHLD) - { - if(!vmbusy()) - { - job.in_critical++; - job_reap(SIGCHLD); - job.in_critical--; - break; - } - sh_delay(1,0); - } - if((n = read(subpipe[0],buff,sizeof(buff)))==0) - break; - if(n>0) - sfwrite(sfstdout,buff,n); - else if(errno!=EINTR) - break; - } -done: + while(read(subpipe[0],buff,sizeof(buff))>0) + ; sh_close(subpipe[0]); subpipe[0] = -1; tsetio = 0; @@ -1399,7 +1339,7 @@ int sh_exec(register const Shnode_t *t, int flags) bp->notify = 0; bp->flags = (OPTIMIZE!=0); if(shp->subshell && nv_isattr(np,BLT_NOSFIO)) - sh_subtmpfile(shp->comsub); + sh_subtmpfile(shp); if(execflg && !shp->subshell && !shp->st.trapcom[0] && !shp->st.trap[SH_ERRTRAP] && shp->fn_depth==0 && !nv_isattr(np,BLT_ENV)) { @@ -1616,12 +1556,14 @@ int sh_exec(register const Shnode_t *t, int flags) int pipes[3]; if(shp->subshell) { - sh_subtmpfile(2); - if((shp->comsub && (type&(FAMP|TFORK))==(FAMP|TFORK) || shp->comsub==1) && - !(shp->fdstatus[1]&IONOSEEK)) - unpipe = iousepipe(shp); - if((type&(FAMP|TFORK))==(FAMP|TFORK) && !shp->subshare) - sh_subfork(); + sh_subtmpfile(shp); + if((type&(FAMP|TFORK))==(FAMP|TFORK)) + { + if(shp->comsub && !(shp->fdstatus[1]&IONOSEEK)) + unpipe = iousepipe(shp); + if(!shp->subshare) + sh_subfork(); + } } no_fork = !ntflag && !(type&(FAMP|FPOU)) && !shp->subshell && !(shp->st.trapcom[SIGINT] && *shp->st.trapcom[SIGINT]) && @@ -1675,8 +1617,6 @@ int sh_exec(register const Shnode_t *t, int flags) { if(shp->topfd > topfd) sh_iorestore(shp,topfd,0); /* prevent FD leak from 'not found' */ - if(shp->comsub==1 && usepipe && unpipe) - sh_iounpipe(shp); break; } } @@ -1908,8 +1848,6 @@ int sh_exec(register const Shnode_t *t, int flags) jmpval = sigsetjmp(buffp->buff,0); if(jmpval==0) { - if(shp->comsub==1) - tsetio = 1; if(execflg && !check_exec_optimization(t->fork.forkio)) { execflg = 0; @@ -1942,8 +1880,6 @@ int sh_exec(register const Shnode_t *t, int flags) if(type || !sh_isoption(SH_PIPEFAIL)) shp->exitval = type; } - if(shp->comsub==1 && usepipe) - sh_iounpipe(shp); shp->pipepid = 0; shp->st.ioset = 0; } @@ -2028,11 +1964,7 @@ int sh_exec(register const Shnode_t *t, int flags) job.exitval = 0; job.curjobid = 0; if(shp->subshell) - { - sh_subtmpfile(2); - if(shp->comsub==1 && !(shp->fdstatus[1]&IONOSEEK)) - iousepipe(shp); - } + sh_subtmpfile(shp); shp->inpipe = pvo; shp->outpipe = pvn; pvo[1] = -1; @@ -2047,7 +1979,7 @@ int sh_exec(register const Shnode_t *t, int flags) memset(exitval,0,job.waitall*sizeof(int)); } else - job.waitall |= (job.bktick_waitall || !pipejob && sh_isstate(SH_MONITOR)); + job.waitall |= (!pipejob && sh_isstate(SH_MONITOR)); job_lock(); nlock++; do @@ -2460,8 +2392,6 @@ int sh_exec(register const Shnode_t *t, int flags) } if(t->par.partre) { - if(shp->subshell && shp->comsub==1) - sh_subfork(); long timer_on = sh_isstate(SH_TIMING); #ifdef timeofday /* must be run after forking a subshell */ @@ -3015,15 +2945,6 @@ pid_t _sh_fork(Shell_t *shp,register pid_t parent,int flags,int *jobid) job.curpgid = curpgid; if(jobid) *jobid = myjob; - if(shp->comsub==1 && usepipe) - { - if(!tsetio || !subdup) - { - if(shp->topfd > restorefd) - sh_iorestore(shp,restorefd,0); - sh_iounpipe(shp); - } - } return(parent); } #if !_std_malloc diff --git a/src/cmd/ksh93/tests/subshell.sh b/src/cmd/ksh93/tests/subshell.sh index 559443ea6..14c2d906e 100755 --- a/src/cmd/ksh93/tests/subshell.sh +++ b/src/cmd/ksh93/tests/subshell.sh @@ -1048,5 +1048,40 @@ got=$(/dev/null +if ((! $?)) +then kill -9 $tpid + err_exit 'read hangs after background job in backtick command sub' +fi + +# https://github.com/ksh93/ksh/issues/316 +sleep 2 & spid=$! +( + for ((i=1; i<=2048; i++)) + do eval "z$i=" + done + eval 'v=`set 2>&1`' + kill $spid +) & +tpid=$! +wait $spid 2>/dev/null +if ((! $?)) +then kill -9 $tpid + err_exit 'backtick command substitution hangs on reproducer from issue 316' +fi + # ====== exit $((Errors<125?Errors:125))