ksh: Segfault on i386 on executing arithmetic command

Commit 6701bb30decdb9ac984558fe08c7a9df0cd19ab9 introduced a segfault on executing a redirection.

$ uname -a          
QNX qnx 6.5.0 2010/07/09-14:44:03EDT x86pc x86
$ SHELL=/home/m/md/src/ksh/arch/qnx.i386/bin/ksh arch/qnx.i386/bin/ksh -x src/cmd/ksh93/tests/shtests -p
+ : ksh regression test harness :
+ command=shtests
+ setslocale='*@(locale).sh'
+ timesensitive='*@(options|sigchld|subshell).sh'
+ valgrindflags='--xml=yes --log-file=/dev/null --track-origins=yes --read-var-info=yes'
+ USAGE=$'\n[-s8?\n@(#)$Id: shtests (ksh 93u+m) 2021-03-23 $\n]\n[-author?David Korn <dgk@research.att.com>]\n[-author?Glenn Fowler <gsf@research.att.com>]\n[-copyright?(c) 2000-2012 AT&T Intellectual Property]\n[-copyright?(c) 2020-2021 Contributors to https://github.com/ksh93/ksh]\n[-license?http://www.eclipse.org/org/documents/epl-v10.html]\n[+NAME?shtests - ksh regression test harness]\n[+DESCRIPTION?\bshtests\b is the \bksh\b(1) regression test harness for\n    \b$SHELL\b or \bksh\b if \bSHELL\b is not defined and exported. If\n    none of the \b--posix --utf8 --compile\b options are specified then\n    all three are enabled.]\n[+INPUT FILES?\bshtests\b regression test files are shell scripts that\n    run in an environment controlled by \bshtests\b. An identification\n    message is printed before and after each test on the standard output.\n    The default environment settings are:]\n    {\n        [+unset LANG]\n        [+unset LC_ALL]\n        [+LC_NUMERIC=C?\b.\b radix point assumed by all test scripts.]\n        [+VMALLOC_OPTIONS=abort?\bvmalloc\b(1) arena checking enabled\n            with \babort(2)\b on error.]\n    }\n[c:compile?Run test scripts using \bshcomp\b(1).]\n[d:debug?Enable \bshtests\b execution trace.]\n[k:keep?Keep temporary files after test run; shtests will report the location.]\n[l:locale?Disable \b--utf8\b and run the \b--posix\b and \b--compile\b\n    tests, if enabled, in the locale of the caller. However, for locales\n    where \b.\b is not the radix point, \bLC_NUMERIC\b is set to \bC\b\n    to avoid invalid regressions.]\n[p:posix?Run the test scripts in the posix/C locale.]\n[t!:time?Include the current date/time in the test identification\n    messages.]\n[u:utf8?Run the test scripts in the ast-specific C.UTF-8 locale.]\n[v!:vmalloc_options?Run tests with \bVMALLOC_OPTIONS=abort\b. Test\n    script names matching \b*@(options|sigchld|subshell).sh\b are run with\n    \bVMALLOC_OPTIONS\b unset.]\n[V:valgrind?Set \b--novmalloc_options\b and run the test scripts with\n    \bvalgrind\b(1) on \bksh\b. If \b$SHELL-g\b exists and is executable,\n    then it is used instead of \b$SHELL\b.]\n[x:trace?Enable script execution trace.]\n\n[ test.sh ... ] [ name=value ... ]\n\n[+SEE ALSO?\bksh\b(1), \bregress\b(1), \brt\b(1)]\n'
+ 2> /dev/null
Memory fault (core dumped)

The segfault happens on this line: https://github.com/ksh93/ksh/blob/ba43436f10a41816d1f4b78e7494879c313d31d6/src/cmd/ksh93/sh/xec.c#L1006 It is particularly this bit: && (t->tre.treio->iofile&IOREWRITE) that it doesn’t like. If that is commented out, the segfault disappears (and of course the execve optimization breaks again).

Ping @JohnoKing

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 21

Most upvoted comments

Actually, my simplification of the sh_exec invocation (sh_exec(t->fork.forktre, flags & ~simple & execflg);) is incorrect. The result is that the second argument can only ever be 0 or 1 whereas previously it would pass on all of the bits of flags except possibly the last one. It’s odd it doesn’t seem to cause any regressions.

Corrected patch:

--- a/src/cmd/ksh93/sh/xec.c
+++ b/src/cmd/ksh93/sh/xec.c
@@ -958,6 +958,21 @@ static Namval_t *enter_namespace(Shell_t *shp, Namval_t *nsp)
 }
 #endif /* SHOPT_NAMESPACE */
 
+/*
+ * Check whether to execve(2) the final command or make its redirections permanent.
+ */
+static int check_exec_optimization(struct ionod *iop)
+{
+	if(sh.subshell || sh.exittrap || sh.errtrap)
+		return(0);
+	/* '<>;' (IOREWRITE) redirections are incompatible with exec */
+	while(iop && !(iop->iofile & IOREWRITE))
+		iop = iop->ionxt;
+	if(iop)
+		return(0);
+	return(1);
+}
+
 int sh_exec(register const Shnode_t *t, int flags)
 {
 	register Shell_t	*shp = sh_getinterp();
@@ -1003,8 +1018,6 @@ int sh_exec(register const Shnode_t *t, int flags)
 		shp->exitval=0;
 		shp->lastsig = 0;
 		shp->lastpath = 0;
-		if(shp->exittrap || shp->errtrap || (t->tre.treio && (t->tre.treio->iofile&IOREWRITE)))
-			execflg = 0; /* don't run the command with execve(2) */
 		switch(type&COMMSK)
 		{
 		    case TCOM:
@@ -1174,6 +1187,8 @@ int sh_exec(register const Shnode_t *t, int flags)
 				int tflags = 1;
 				if(np &&  nv_isattr(np,BLT_DCL))
 					tflags |= 2;
+				if(execflg && !check_exec_optimization(io))
+					execflg = 0;
 				if(argn==0)
 				{
 					/* fake 'true' built-in */
@@ -1848,8 +1863,6 @@ int sh_exec(register const Shnode_t *t, int flags)
 			int 	jmpval, waitall = 0;
 			int 	simple = (t->fork.forktre->tre.tretyp&COMMSK)==TCOM;
 			struct checkpt *buffp = (struct checkpt*)stkalloc(shp->stk,sizeof(struct checkpt));
-			if(shp->subshell)
-				execflg = 0;
 			sh_pushcontext(shp,buffp,SH_JMPIO);
 			if(type&FPIN)
 			{
@@ -1875,6 +1888,11 @@ int sh_exec(register const Shnode_t *t, int flags)
 			{
 				if(shp->comsub==1)
 					tsetio = 1;
+				if(execflg && !check_exec_optimization(t->fork.forkio))
+				{
+					execflg = 0;
+					flags &= ~sh_state(SH_NOFORK);
+				}
 				sh_redirect(shp,t->fork.forkio,execflg);
 				(t->fork.forktre)->tre.tretyp |= t->tre.tretyp&FSHOWME;
 				sh_exec(t->fork.forktre,flags&~simple);

McDutchie on Apr 17, 2021

The if statement the above patch avoids is run after sh_exec is called here: https://github.com/ksh93/ksh/blob/ba43436f10a41816d1f4b78e7494879c313d31d6/src/cmd/ksh93/sh/xec.c#L1878-L1880

I have a new patch that takes a different approach to fixing the exec bug in subshells. Since the problematic code is only run if the nofork flag is passed to sh_exec, the new patch avoids passing that flag:

--- a/src/cmd/ksh93/sh/xec.c
+++ b/src/cmd/ksh93/sh/xec.c
@@ -1003,8 +1003,6 @@ int sh_exec(register const Shnode_t *t, int flags)
 		shp->exitval=0;
 		shp->lastsig = 0;
 		shp->lastpath = 0;
-		if(shp->exittrap || shp->errtrap || (t->tre.treio && (t->tre.treio->iofile&IOREWRITE)))
-			execflg = 0; /* don't run the command with execve(2) */
 		switch(type&COMMSK)
 		{
 		    case TCOM:
@@ -1172,8 +1170,19 @@ int sh_exec(register const Shnode_t *t, int flags)
 				Shbltin_t *bp=0;
 				static char *argv[2];
 				int tflags = 1;
-				if(np &&  nv_isattr(np,BLT_DCL))
+				struct ionod *iop = io;
+				if(np && nv_isattr(np,BLT_DCL))
 					tflags |= 2;
+				if(shp->exittrap || shp->errtrap)
+					execflg = 0;
+				else for(;iop;iop=iop->ionxt)
+				{
+					if(iop->iofile&IOREWRITE)
+					{
+						execflg = 0;
+						break;
+					}
+				}
 				if(argn==0)
 				{
 					/* fake 'true' built-in */
@@ -1847,9 +1856,20 @@ int sh_exec(register const Shnode_t *t, int flags)
 			pid_t	pid = 0;
 			int 	jmpval, waitall = 0;
 			int 	simple = (t->fork.forktre->tre.tretyp&COMMSK)==TCOM;
+			int	nofork_flag = 0;
 			struct checkpt *buffp = (struct checkpt*)stkalloc(shp->stk,sizeof(struct checkpt));
-			if(shp->subshell)
+			struct ionod *iop = t->fork.forkio;
+			if(shp->subshell || shp->exittrap || shp->errtrap)
 				execflg = 0;
+			else for(;iop;iop=iop->ionxt)
+			{
+				if(iop->iofile&IOREWRITE)
+				{
+					execflg = 0;
+					nofork_flag |= sh_state(SH_NOFORK);
+					break;
+				}
+			}
 			sh_pushcontext(shp,buffp,SH_JMPIO);
 			if(type&FPIN)
 			{
@@ -1877,7 +1897,7 @@ int sh_exec(register const Shnode_t *t, int flags)
 					tsetio = 1;
 				sh_redirect(shp,t->fork.forkio,execflg);
 				(t->fork.forktre)->tre.tretyp |= t->tre.tretyp&FSHOWME;
-				sh_exec(t->fork.forktre,flags&~simple);
+				sh_exec(t->fork.forktre,flags&~(simple|nofork_flag));
 			}
 			else
 				sfsync(shp->outpool);

JohnoKing on Apr 17, 2021

Still not right. The following script should output bye but outputs nothing because the >/dev/null redirection was done with execflg on so it persists for the trap:

trap 'echo bye' EXIT
{ echo hi; } >/dev/null

New patch that should fix this:

--- a/src/cmd/ksh93/sh/xec.c
+++ b/src/cmd/ksh93/sh/xec.c
@@ -1003,8 +1003,6 @@ int sh_exec(register const Shnode_t *t, int flags)
 		shp->exitval=0;
 		shp->lastsig = 0;
 		shp->lastpath = 0;
-		if(shp->exittrap || shp->errtrap || (t->tre.treio && (t->tre.treio->iofile&IOREWRITE)))
-			execflg = 0; /* don't run the command with execve(2) */
 		switch(type&COMMSK)
 		{
 		    case TCOM:
@@ -1174,6 +1172,8 @@ int sh_exec(register const Shnode_t *t, int flags)
 				int tflags = 1;
 				if(np &&  nv_isattr(np,BLT_DCL))
 					tflags |= 2;
+				if(shp->exittrap || shp->errtrap || io && (io->iofile & IOREWRITE))
+					execflg = 0;
 				if(argn==0)
 				{
 					/* fake 'true' built-in */
@@ -1848,7 +1848,7 @@ int sh_exec(register const Shnode_t *t, int flags)
 			int 	jmpval, waitall = 0;
 			int 	simple = (t->fork.forktre->tre.tretyp&COMMSK)==TCOM;
 			struct checkpt *buffp = (struct checkpt*)stkalloc(shp->stk,sizeof(struct checkpt));
-			if(shp->subshell)
+			if(shp->subshell || shp->exittrap || shp->errtrap || t->fork.forkio && (t->fork.forkio->iofile & IOREWRITE))
 				execflg = 0;
 			sh_pushcontext(shp,buffp,SH_JMPIO);
 			if(type&FPIN)

McDutchie on Apr 17, 2021

I’ve managed to get my 32-bit Linux system working and I’ve reproduced the ksh segfault on it. I can also confirm the patch above fixes the segfault without causing regressions.

JohnoKing on Apr 17, 2021

In fact I think the check doesn’t need to be split but can be moved entirely to the TCOM handling case, which is for executing all simple commands. Only a simple command is ever going to be possibly execved, so execflg is irrelevant for everything else (parentheses, case, while, etc.).

--- a/src/cmd/ksh93/sh/xec.c
+++ b/src/cmd/ksh93/sh/xec.c
@@ -1003,8 +1003,6 @@ int sh_exec(register const Shnode_t *t, int flags)
 		shp->exitval=0;
 		shp->lastsig = 0;
 		shp->lastpath = 0;
-		if(shp->exittrap || shp->errtrap || (t->tre.treio && (t->tre.treio->iofile&IOREWRITE)))
-			execflg = 0; /* don't run the command with execve(2) */
 		switch(type&COMMSK)
 		{
 		    case TCOM:
@@ -1100,6 +1098,8 @@ int sh_exec(register const Shnode_t *t, int flags)
 				comn = com[argn-1];
 			}
 			io = t->tre.treio;
+			if(shp->exittrap || shp->errtrap || io && (io->iofile & IOREWRITE))
+				execflg = 0; /* don't run the command with execve(2) */
 			if(shp->envlist = argp = t->com.comset)
 			{
 				if(argn==0 || (np && (nv_isattr(np,BLT_DCL) || (!command && nv_isattr(np,BLT_SPC)))))

McDutchie on Apr 17, 2021