Fix crash upon running many subshells (#113)

JohnoKing · web-flow · commit 05ac1dbb411e · 2020-08-12T18:50:59.000+01:00
Co-authored-by: Martijn Dekker <martijn@inlv.org> An intermittent crash occurred after running many thousands of virtual/non-forked subshells. One reproducer is a crash in the shbench fibonacci.ksh test, as documented here: https://github.com/ksh-community/shbench/blob/f3d9e134/bench/fibonacci.ksh#L4-L10 The apparent cause was the signed and insufficiently large 'short' data type of 'curenv' and related variables which wrapped around to a negative number when overflowing. These IDs are necessary for the 'wait' builtin to obtain the exit status from a background job. This fix is inspired by a patch based on ksh 93v-: https://build.opensuse.org/package/view_file/shells/ksh/ksh93-longenv.dif?expand=1 https://src.fedoraproject.org/rpms/ksh/blob/f24/f/ksh-20130628-longer.patch However, we change the type to 'unsigned int' instead of 'long'. On all remotely modern systems, ints are 32-bit values, and using this type avoids a performance degradation on 32-bit sytems. Making them unsigned prevents an overflow to negative values. src/cmd/ksh93/include/defs.h, src/cmd/ksh93/include/jobs.h, src/cmd/ksh93/include/nval.h, src/cmd/ksh93/include/shell.h: - Change the types of the static global 'subenv' and the subshell structure members 'curenv', 'jobenv', 'subenv', 'p_env' and 'subshell' to one consistent type, unsigned int. src/cmd/ksh93/sh/jobs.c, src/cmd/ksh93/sh/macro.c: src/cmd/ksh93/sh/name.c: src/cmd/ksh93/sh/nvtype.c, src/cmd/ksh93/sh/subshell.c: - Updates to match new variable types. src/cmd/ksh93/tests/subshell.sh: - Show wrong exit status in message on failure of 'wait' builtin.
diff --git a/NEWS b/NEWS
@@ -3,6 +3,10 @@ For full details, see the git log at: https://github.com/ksh93/ksh
 
 Any uppercase BUG_* names are modernish shell bug IDs.
 
+2020-08-11:
+
+- Fixed an intermittent crash upon running a large number of subshells.
+
 2020-08-10:
 
 - A number of fixes have been applied to the printf formatting directives
diff --git a/src/cmd/ksh93/include/defs.h b/src/cmd/ksh93/include/defs.h
@@ -167,8 +167,8 @@ struct shared
 	Namval_t	*prev_table;	/* previous table used in nv_open  */ \
 	Sfio_t		*outpool;	/* output stream pool */ \
 	long		timeout;	/* read timeout */ \
-	short		curenv;		/* current subshell number */ \
-	short		jobenv;		/* subshell number for jobs */ \
+	unsigned int	curenv;		/* current subshell number */ \
+	unsigned int	jobenv;		/* subshell number for jobs */ \
 	int		infd;		/* input file descriptor */ \
 	short		nextprompt;	/* next prompt is PS<nextprompt> */ \
 	short		poolfiles; \
diff --git a/src/cmd/ksh93/include/jobs.h b/src/cmd/ksh93/include/jobs.h
@@ -68,7 +68,7 @@ struct process
 	unsigned short	p_exit;		/* exit value or signal number */
 	unsigned short	p_exitmin;	/* minimum exit value for xargs */
 	unsigned short	p_flag;		/* flags - see below */
-	int		p_env;		/* subshell environment number */
+	unsigned int	p_env;		/* subshell environment number */
 #ifdef JOBS
 	off_t		p_name;		/* history file offset for command */
 	struct termios	p_stty;		/* terminal state for job */
diff --git a/src/cmd/ksh93/include/nval.h b/src/cmd/ksh93/include/nval.h
@@ -75,7 +75,7 @@ struct Namfun
 {
 	const Namdisc_t	*disc;
 	char		nofree;
-	unsigned char	subshell;
+	unsigned int	subshell;
 	uint32_t	dsize;
 	Namfun_t	*next;
 	char		*last;
diff --git a/src/cmd/ksh93/include/shell.h b/src/cmd/ksh93/include/shell.h
@@ -143,7 +143,7 @@ struct Shell_s
 	int		exitval;	/* most recent exit value */
 	unsigned char	trapnote;	/* set when trap/signal is pending */
 	char		shcomp;		/* set when running shcomp */
-	short		subshell;	/* set for virtual subshell */
+	unsigned int	subshell;	/* set for virtual subshell */
 #ifdef _SH_PRIVATE
 	_SH_PRIVATE
 #endif /* _SH_PRIVATE */
diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h
@@ -17,4 +17,4 @@
 *                  David Korn <dgk@research.att.com>                   *
 *                                                                      *
 ***********************************************************************/
-#define SH_RELEASE	"93u+m 2020-08-10"
+#define SH_RELEASE	"93u+m 2020-08-11"
diff --git a/src/cmd/ksh93/sh/init.c b/src/cmd/ksh93/sh/init.c
@@ -748,7 +748,8 @@ void sh_setmatch(Shell_t *shp,const char *v, int vsize, int nmatch, regoff_t mat
 {
 	struct match	*mp = &ip->SH_MATCH_init;
 	Namval_t	*np = nv_namptr(mp->node,0); 
-	register int	i,n,x, savesub=shp->subshell;
+	register int	i,n,x;
+	unsigned int	savesub = shp->subshell;
 	Namarr_t	*ap = nv_arrayptr(SH_MATCHNOD);
 	shp->subshell = 0;
 #ifndef SHOPT_2DMATCH
diff --git a/src/cmd/ksh93/sh/jobs.c b/src/cmd/ksh93/sh/jobs.c
@@ -1647,7 +1647,7 @@ static struct process *job_unpost(register struct process *pwtop,int notify)
 	register struct process *pw;
 	/* make sure all processes are done */
 #ifdef DEBUG
-	sfprintf(sfstderr,"ksh: job line %4d: drop pid=%d critical=%d pid=%d env=%d\n",__LINE__,getpid(),job.in_critical,pwtop->p_pid,pwtop->p_env);
+	sfprintf(sfstderr,"ksh: job line %4d: drop pid=%d critical=%d pid=%d env=%u\n",__LINE__,getpid(),job.in_critical,pwtop->p_pid,pwtop->p_env);
 	sfsync(sfstderr);
 #endif /* DEBUG */
 	pwtop = pw = job_byjid((int)pwtop->p_job);
diff --git a/src/cmd/ksh93/sh/macro.c b/src/cmd/ksh93/sh/macro.c
@@ -2716,6 +2716,7 @@ static char *sh_tilde(Shell_t *shp,register const char *string)
 	register int		c;
 	register struct passwd	*pw;
 	register Namval_t *np=0;
+	unsigned int save;
 	static Dt_t *logins_tree;
 	if(*string++!='~')
 		return(NIL(char*));
@@ -2771,10 +2772,10 @@ static char *sh_tilde(Shell_t *shp,register const char *string)
 		logins_tree = dtopen(&_Nvdisc,Dtbag);
 	if(np=nv_search(string,logins_tree,NV_ADD))
 	{
-		c = shp->subshell;
+		save = shp->subshell;
 		shp->subshell = 0;
 		nv_putval(np, pw->pw_dir,0);
-		shp->subshell = c;
+		shp->subshell = save;
 	}
 	return(pw->pw_dir);
 }
diff --git a/src/cmd/ksh93/sh/name.c b/src/cmd/ksh93/sh/name.c
@@ -2464,7 +2464,7 @@ static void table_unset(Shell_t *shp, register Dt_t *root, int flags, Dt_t *oroo
 		{
 			if(nv_cover(nq))
 			{
-				int subshell = shp->subshell;
+				unsigned int subshell = shp->subshell;
 				shp->subshell = 0;
 				if(nv_isattr(nq, NV_INTEGER))
 				{
diff --git a/src/cmd/ksh93/sh/nvtype.c b/src/cmd/ksh93/sh/nvtype.c
@@ -1318,7 +1318,8 @@ int nv_settype(Namval_t* np, Namval_t *tp, int flags)
 	char		*val=0;
 	Namarr_t	*ap=0;
 	Shell_t		*shp = sh_getinterp();
-	int		nelem=0,subshell=shp->subshell;
+	int		nelem = 0;
+	unsigned int	subshell = shp->subshell;
 #if SHOPT_TYPEDEF
 	Namval_t	*tq;
 	if(nv_type(np)==tp)
diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c
@@ -103,7 +103,7 @@ static struct subshell
 	char		pwdclose;
 } *subshell_data;
 
-static int subenv;
+static unsigned int subenv;
 
 
 /*
@@ -176,7 +176,7 @@ void sh_subfork(void)
 {
 	register struct subshell *sp = subshell_data;
 	Shell_t	*shp = sp->shp;
-	int	curenv = shp->curenv;
+	unsigned int curenv = shp->curenv;
 	pid_t pid;
 	char *trap = shp->st.trapcom[0];
 	if(trap)
@@ -251,7 +251,7 @@ Namval_t *sh_assignok(register Namval_t *np,int add)
 	Dt_t			*dp= shp->var_tree;
 	Namval_t		*mpnext;
 	Namarr_t		*ap;
-	int			save;
+	unsigned int		save;
 	/* don't bother to save if in a ${ subshare; } */
 	if(sp->subshare)
 		return(np);
@@ -456,7 +456,7 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
 	struct subshell sub_data;
 	register struct subshell *sp = &sub_data;
 	int jmpval,nsig=0,duped=0;
-	int savecurenv = shp->curenv;
+	unsigned int savecurenv = shp->curenv;
 	int savejobpgid = job.curpgid;
 	int *saveexitval = job.exitval;
 	char *savsig;
diff --git a/src/cmd/ksh93/tests/subshell.sh b/src/cmd/ksh93/tests/subshell.sh
@@ -529,7 +529,8 @@ err() { return $1; }
 ( err 12 ) & pid=$!
 : $( $date)
 wait $pid
-[[ $? == 12 ]] || err_exit 'exit status from subshells not being preserved'
+exit_status=$?
+[[ $exit_status == 12 ]] || err_exit "exit status from subshells not being preserved (expected 12, got $exit_status)"
 
 if	cat /dev/fd/3 3</dev/null >/dev/null 2>&1 || whence mkfifo > /dev/null
 then	x="$(sed 's/^/Hello /' <(print "Fred" | sort))"

Original file line number	Diff line number	Diff line change
`@@ -75,7 +75,7 @@ struct Namfun`
`75`	`75`	`{`
`76`	`76`	`const Namdisc_t *disc;`
`77`	`77`	`char nofree;`
`78`		`- unsigned char subshell;`
	`78`	`+ unsigned int subshell;`
`79`	`79`	`uint32_t dsize;`
`80`	`80`	`Namfun_t *next;`
`81`	`81`	`char *last;`
Original file line number	Diff line number	Diff line change
`@@ -2464,7 +2464,7 @@ static void table_unset(Shell_t shp, register Dt_t root, int flags, Dt_t *oroo`
`2464`	`2464`	`{`
`2465`	`2465`	`if(nv_cover(nq))`
`2466`	`2466`	`{`
`2467`		`- int subshell = shp->subshell;`
	`2467`	`+ unsigned int subshell = shp->subshell;`
`2468`	`2468`	`shp->subshell = 0;`
`2469`	`2469`	`if(nv_isattr(nq, NV_INTEGER))`
`2470`	`2470`	`{`