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
2025-02-13 03:32:24 +00:00 · 2021-08-13 08:01:46 +02:00 · 2021-08-13 08:01:46 +02:00 · a2196f9434
commit a2196f9434
parent 6952d444ae
9 changed files with 74 additions and 139 deletions
--- a/5
+++ b/5
@ -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
--- 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*);
--- 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 */
 };
--- 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. */
--- 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;
--- 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<<fn;
 						dupfd = sffileno(sfstdout);
--- a/src/cmd/ksh93/sh/subshell.c
+++ b/src/cmd/ksh93/sh/subshell.c
@ -112,14 +112,12 @@ static unsigned int subenv;


 /*
- * This routine will turn the sftmp() file into a real /tmp file or pipe
- * if the /tmp file create fails
+ * This routine will turn the sftmp() file into a real temporary file
 */
-void	sh_subtmpfile(char comsub_flag)
+void	sh_subtmpfile(Shell_t *shp)
 {
 	if(sfset(sfstdout,0,0)&SF_STRING)
 	{
-		Shell_t *shp = sh_getinterp();
 		register int fd;
 		register struct checkpt	*pp = (struct checkpt*)shp->jmplist;
 		register struct subshell *sp = subshell_data->pipe;
@ -136,30 +134,12 @@ void	sh_subtmpfile(char comsub_flag)
 			UNREACHABLE();
 		}
 		/* popping a discipline forces a /tmp file create */
-		if(comsub_flag != 1)
 		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);
+			errormsg(SH_DICT,ERROR_SYSTEM|ERROR_PANIC,"could not create temp file");
 			UNREACHABLE();
 		}
-			sh_close(fds[1]);
-		}
-		else
-		{
 		shp->fdstatus[fd] = IOREAD|IOWRITE;
 		sfsync(sfstdout);
 		if(fd==1)
@ -170,7 +150,6 @@ void	sh_subtmpfile(char comsub_flag)
 			shp->fdstatus[1] = shp->fdstatus[fd];
 			shp->fdstatus[fd] = IOCLOSE;
 		}
-		}
 		sh_iostream(shp,1);
 		sfset(sfstdout,SF_SHARE|SF_PUBLIC,1);
 		sfpool(sfstdout,shp->outpool,SF_WRITE);
@ -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;
--- a/src/cmd/ksh93/sh/xec.c
+++ b/src/cmd/ksh93/sh/xec.c
@ -184,31 +184,10 @@ 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;
-	sh_close(subpipe[1]);
-	if(subdup=shp->subdup) for(i=0; i < 10; i++)
-	{
-		if(subdup&(1<<i))
-		{
-			sh_close(i);
-			fcntl(1,F_DUPFD,i);
-			shp->fdstatus[i] = shp->fdstatus[1];
-		}
-	}
-	return(1);
-}

 void sh_iounpipe(Shell_t *shp)
 {
@ -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<<n))
-		{
-			sh_close(n);
-			fcntl(1, F_DUPFD, n);
-			shp->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,13 +1556,15 @@ 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))
+				sh_subtmpfile(shp);
+				if((type&(FAMP|TFORK))==(FAMP|TFORK))
+				{
+					if(shp->comsub && !(shp->fdstatus[1]&IONOSEEK))
 						unpipe = iousepipe(shp);
-				if((type&(FAMP|TFORK))==(FAMP|TFORK) && !shp->subshare)
+					if(!shp->subshare)
 						sh_subfork();
 				}
+			}
 			no_fork = !ntflag && !(type&(FAMP|FPOU)) && !shp->subshell &&
 			    !(shp->st.trapcom[SIGINT] && *shp->st.trapcom[SIGINT]) &&
 			    !shp->st.trapcom[0] && !shp->st.trap[SH_ERRTRAP] &&
@ -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
--- a/src/cmd/ksh93/tests/subshell.sh
+++ b/src/cmd/ksh93/tests/subshell.sh
@ -1048,5 +1048,40 @@ got=$(<r624627501.out)
 [[ $got == "$exp" ]] || err_exit 'background job optimization within virtual subshell causes program flow corruption' \
 	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"

+# ======
+# https://github.com/ksh93/ksh/issues/305
+sleep 2 & spid=$!
+for ((i=1; i<2048; i++))
+do	print $i
+done | (		# trigger 1: read from pipe
+        foo=`: &`	# trigger 2: bg job in backtick comsub
+        while read -r line
+        do	:
+        done
+        kill $spid
+) &
+tpid=$!
+wait $spid 2>/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))