combining alarm and IFS caused segfault (rhbz#1176670)

McDutchie · McDutchie · commit 18b3f4aa2818 · 2020-09-27T03:03:48.000+02:00
The undocumented alarm builtin executes actions unsafely so that 'read' with an IFS assignment crashed when an alarm was triggered. This applies an edited version of a Red Hat patch: https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-alarmifs.patch Prior discussion: https://bugzilla.redhat.com/1176670 src/cmd/ksh93/bltins/alarm.c: - Add a TODO note based on dgk's 2014 email cited in the RH bug. - When executing the trap function, save and restore the IFS table. src/cmd/ksh93/sh/init.c: get_ifs(): - Remove now-unnecessary SHOPT_MULTIBYTE preprocessor directives as 8477d2c lets the compiler optimise out multibyte code if needed. - Initialise the 0 position of the IFS table to S_EOF. This corresponds with the static state tables in data/lexstates.c. src/cmd/ksh93/tests/builtins.sh: - Crash test.
diff --git a/src/cmd/ksh93/bltins/alarm.c b/src/cmd/ksh93/bltins/alarm.c
@@ -26,6 +26,18 @@
  *
  */
 
+/*
+ * TODO: 2014 email from David Korn cited at <https://bugzilla.redhat.com/1176670>:
+ *
+ * > I never documented the alarm builtin because it is problematic.  The
+ * > problem is that traps can't safely be handled asynchronously.  What should
+ * > happen is that the trap is marked for execution (sh_trapnote) and run after
+ * > the current command completes.  The time trap should wake up the shell if
+ * > it is blocked and it should return and then handle the trap.
+ *
+ * When that is done, the save_ifstable workaround can probably be removed.
+ */
+
 #include	"defs.h"
 #include	<error.h>
 #include	<stak.h>
@@ -141,7 +153,12 @@ void	sh_timetraps(Shell_t *shp)
 			{
 				tp->flags &= ~L_FLAG;
 				if(tp->action)
+				{
+					char	save_ifstable[256];
+					memcpy(save_ifstable,shp->ifstable,sizeof(save_ifstable));
 					sh_fun(tp->action,tp->node,(char**)0);
+					memcpy(shp->ifstable,save_ifstable,sizeof(save_ifstable));
+				}
 				tp->flags &= ~L_FLAG;
 				if(!tp->flags)
 				{
diff --git a/src/cmd/ksh93/sh/init.c b/src/cmd/ksh93/sh/init.c
@@ -503,21 +503,14 @@ static char* get_ifs(register Namval_t* np, Namfun_t *fp)
 		memset(shp->ifstable,0,(1<<CHAR_BIT));
 		if(cp=value)
 		{
-#if SHOPT_MULTIBYTE
-			while(n=mbsize(cp),c= *(unsigned char*)cp)
-#else
-			while(c= *(unsigned char*)cp++)
-#endif /* SHOPT_MULTIBYTE */
+			while(n = mbsize(cp), c = *(unsigned char*)cp++)
 			{
-#if SHOPT_MULTIBYTE
-				cp++;
 				if(n>1)
 				{
 					cp += (n-1);
 					shp->ifstable[c] = S_MBYTE;
 					continue;
 				}
-#endif /* SHOPT_MULTIBYTE */
 				n = S_DELIM;
 				if(c== *cp)
 					cp++;
@@ -533,6 +526,7 @@ static char* get_ifs(register Namval_t* np, Namfun_t *fp)
 			shp->ifstable[' '] = shp->ifstable['\t'] = S_SPACE;
 			shp->ifstable['\n'] = S_NL;
 		}
+		shp->ifstable[0] = S_EOF;
 	}
 	return(value);
 }
diff --git a/src/cmd/ksh93/tests/builtins.sh b/src/cmd/ksh93/tests/builtins.sh
@@ -1055,5 +1055,27 @@ do	case $bltin in
 			"(expected string containing $(printf %q "$expect"), got $(printf %q "$actual"))"
 done 3< <(builtin)
 
+# ======
+# The 'alarm' builtin could make 'read' crash due to IFS table corruption caused by unsafe asynchronous execution.
+# https://bugzilla.redhat.com/1176670
+if	(builtin alarm) 2>/dev/null
+then	got=$( { "$SHELL" -c '
+		builtin alarm
+		alarm -r alarm_handler +.001
+		i=0
+		function alarm_handler.alarm
+		{
+			let "(++i) > 100" && exit
+		}
+		while :; do
+			echo cargo,odds and ends,jetsam,junk,wreckage,castoffs,sea-drift
+		done | while IFS="," read arg1 arg2 arg3 arg4 junk; do
+			:
+		done
+	'; } 2>&1)
+	((!(e = $?))) || err_exit 'crash with alarm and IFS' \
+		"(got status $e$( ((e>128)) && print -n / && kill -l "$e"), $(printf %q "$got"))"
+fi
+
 # ======
 exit $((Errors<125?Errors:125))

Original file line number	Diff line number	Diff line change
`@@ -503,21 +503,14 @@ static char* get_ifs(register Namval_t* np, Namfun_t *fp)`
`503`	`503`	`memset(shp->ifstable,0,(1<<CHAR_BIT));`
`504`	`504`	`if(cp=value)`
`505`	`505`	`{`
`506`		`-#if SHOPT_MULTIBYTE`
`507`		`- while(n=mbsize(cp),c= (unsigned char)cp)`
`508`		`-#else`
`509`		`- while(c= (unsigned char)cp++)`
`510`		`-#endif /* SHOPT_MULTIBYTE */`
	`506`	`+ while(n = mbsize(cp), c = (unsigned char)cp++)`
`511`	`507`	`{`
`512`		`-#if SHOPT_MULTIBYTE`
`513`		`- cp++;`
`514`	`508`	`if(n>1)`
`515`	`509`	`{`
`516`	`510`	`cp += (n-1);`
`517`	`511`	`shp->ifstable[c] = S_MBYTE;`
`518`	`512`	`continue;`
`519`	`513`	`}`
`520`		`-#endif /* SHOPT_MULTIBYTE */`
`521`	`514`	`n = S_DELIM;`
`522`	`515`	`if(c== *cp)`
`523`	`516`	`cp++;`
`@@ -533,6 +526,7 @@ static char* get_ifs(register Namval_t* np, Namfun_t *fp)`
`533`	`526`	`shp->ifstable[' '] = shp->ifstable['\t'] = S_SPACE;`
`534`	`527`	`shp->ifstable['\n'] = S_NL;`
`535`	`528`	`}`
	`529`	`+ shp->ifstable[0] = S_EOF;`
`536`	`530`	`}`
`537`	`531`	`return(value);`
`538`	`532`	`}`