From b0a6c1bde5fdf67d65fb31f7739d71486bb60d2b Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Sat, 17 Apr 2021 21:31:38 +0100 Subject: [PATCH] Further fix '<>;' and fix crash on 32-bit systems (re: 6701bb30) Accessing t->tre.treio for every sh_exec() run is invalid because 't' is of type Shnode_t, which is a union that can contain many different kinds of structs. As all members of a union occupy the same address space, only one can be used at a time. Which member is valid to access depends on the node type sh_exec() was called with. The invalid access triggered a crash on 32-bit systems when executing an arithmetic command like ((x=1)). The t->tre.treio union member should be accessed for a simple command (case TCOM in sh_exec()). The fix is also needed for redirections attached to blocks (case TSETIO) in which case the union member to use is t->fork.forkio. src/cmd/ksh93/sh/xec.c: - Add check_exec_optimization() function that checks for all the conditions where the exec optimisation should not be done. For redirections we need to loop through the whole list to check for an IOREWRITE (<>;) one. - sh_exec(): case TCOM (simple command): Only bother to call check_exec_optimization() if there are either command arguments or redirections (IOW: don't bother for bare variable assignments), so move it to within the if(io||argn) block. - sh_exec(): case TSETIO: This needs a similar fix. To avoid the optimization breaking again if the last command is a subshell with a <>; redirection attached, we need to not only set execflg to 0 but also clear the SH_NOFORK state bit from the 'flags' variable which is passed on to the recursive sh_exec() call. src/cmd/ksh93/tests/io.sh: - Update and expand tests. Add tests for redirections attached to simple commands (TCOM) and various kinds of code block (TSETIO). Co-authored-by: Johnothan King Resolves: https://github.com/ksh93/ksh/issues/278 --- NEWS | 4 +- src/cmd/ksh93/sh/xec.c | 26 +++++++++++-- src/cmd/ksh93/tests/io.sh | 79 +++++++++++++++++++++++++++++---------- 3 files changed, 82 insertions(+), 27 deletions(-) diff --git a/NEWS b/NEWS index 19987e684..fb7608c23 100644 --- a/NEWS +++ b/NEWS @@ -8,10 +8,8 @@ Any uppercase BUG_* names are modernish shell bug IDs. - Fixed a bug in emacs mode: after using tab completion to complete the name of a directory, it was not possible to type numbers after the slash. -2021-04-15: - - Fixed an optimization bug that caused the <>; redirection operator to fail - when used in -c scripts. + when used with the last command in a -c script. 2021-04-14: diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c index a1d334331..21f337421 100644 --- 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); diff --git a/src/cmd/ksh93/tests/io.sh b/src/cmd/ksh93/tests/io.sh index 5581ead7b..2d6976a36 100755 --- a/src/cmd/ksh93/tests/io.sh +++ b/src/cmd/ksh93/tests/io.sh @@ -491,28 +491,67 @@ n=$( exec {n}< /dev/null; print -r -- $n) # ====== # Truncating a file using <> and >#num # https://github.com/att/ast/issues/61 - -for ((i=1; i<=10; i++)) do print "$i"; done | tee "$tmp/nums2" > "$tmp/nums1" -expect=$'1\n2\n3\n4' - -(1<>;"$tmp/nums1" >#5) -actual=$(cat "$tmp/nums1") -[[ "$actual" = "$expect" ]] || err_exit "Failed to truncate file in subshell \ -(expected $(printf %q "$expect"), got $(printf %q "$actual"))" - # The <>; redirection operator didn't work correctly in -c scripts # https://github.com/att/ast/issues/9 -"$SHELL" -c '1<>;"$1/nums2" >#5' x "$tmp" -actual=$(cat "$tmp/nums2") -[[ "$actual" = "$expect" ]] || err_exit "Failed to truncate file in -c script \ -(expected $(printf %q "$expect"), got $(printf %q "$actual"))" +# https://github.com/ksh93/ksh/issues/278 -echo test > "$tmp/ast-9-reproducer" -"$SHELL" -c "echo x 1<>; \"$tmp/ast-9-reproducer\"" -exp=x -got=$(cat "$tmp/ast-9-reproducer") -[[ $exp == "$got" ]] || err_exit "<>; redirection operator fails in -c scripts \ -(expected $(printf %q "$exp"), got $(printf %q "$got"))" +# the extra '3>&2 3>&-' is to verify it all keeps working with multiple redirections +typeset -A exp=( + ['3>&2 3>&- 1<>;nums >#5']=$'1\n2\n3\n4' + [': 3>&2 3>&- 1<>;nums >#5']=$'1\n2\n3\n4' + ['{ :; } 3>&2 3>&- 1<>;nums >#5']=$'1\n2\n3\n4' + ['(:) 3>&2 3>&- 1<>;nums >#5']=$'1\n2\n3\n4' + ['while false; do :; done 3>&2 3>&- 1<>;nums >#5']=$'1\n2\n3\n4' + ['until true; do :; done 3>&2 3>&- 1<>;nums >#5']=$'1\n2\n3\n4' + ['if false; then :; fi 3>&2 3>&- 1<>;nums >#5']=$'1\n2\n3\n4' + ['case x in y) :;; esac 3>&2 3>&- 1<>;nums >#5']=$'1\n2\n3\n4' + ['fn() { :; } 3>&2 3>&- 1<>;nums >#5; fn']=$'1\n2\n3\n4' + ['echo x 3>&2 3>&- 1<>;nums']=x + ['{ echo x; } 3>&2 3>&- 1<>;nums']=x + ['(echo x) 3>&2 3>&- 1<>;nums']=x + ['while :; do echo x; break; done 3>&2 3>&- 1<>;nums']=x + ['until ! :; do echo x; break; done 3>&2 3>&- 1<>;nums']=x + ['if :; then echo x; fi 3>&2 3>&- 1<>;nums']=x + ['case x in x) echo x;; esac 3>&2 3>&- 1<>;nums']=x + ['fn() { echo x; } 3>&2 3>&- 1<>;nums; fn']=x +) +nums=$'1\n2\n3\n4\n5\n6\n7\n8\n9\n10' +for script in "${!exp[@]}" +do + echo "$nums" >nums + eval "$script" + got=$(nums + eval "( $script )" + got=$(nums + "$SHELL" -c "$script" + got=$(nums + echo "$script" >test.sh + "$SHELL" test.sh + got=$(/dev/null' 2>&1) +exp=bye +[[ $got == "$exp" ]] || err_exit "redirection in last -c script command persists for trap" \ + "(expected $(printf %q "$exp"), got $(printf %q "$got"))" + +echo $'trap "echo bye" 0\n{ echo hi; } >/dev/null' >test.sh +got=$("$SHELL" test.sh 2>&1) +exp=bye +[[ $got == "$exp" ]] || err_exit "redirection in last regular script command persists for trap" \ + "(expected $(printf %q "$exp"), got $(printf %q "$got"))" # ====== # Exit behaviour of 'exec', 'command exec', 'redirect' on redirections @@ -538,7 +577,7 @@ actual=$( (redirect /dev/null/foo >$tmp/wrong_redirect) 2>&1; echo " status = $? # Process substitution # An output process substitution should work when combined with a redirection. -result=$("$SHELL" -c 'echo ok > >(sed s/ok/good/); wait') +result=$("$SHELL" -c 'echo ok > >(sed s/ok/good/); wait' 2>&1) [[ $result == good ]] || err_exit 'process substitution does not work with redirections' \ "(expected 'good', got $(printf %q "$result"))"