From e072e7c170150f8117c89810205b96ec47f28784 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Mon, 27 Dec 2021 03:39:32 +0000 Subject: [PATCH] Fix crash in xtrace while processing here-document (re: d7cada7b) Depending on the OS, the heredoc.sh regression tests, and possibly others, still crashed with the -x option (xtrace) on. Analysis: The lexer crashes in lex_advance(). Something has caused an inconsistent lexer state, and it happened earlier on, so the backtrace is useless for figuring out where that happened. But I think I've found it. It's the sh_mactry() call here: src/cmd/ksh93/sh/xec.c, lines 2800 to 2807 in f7213f03 2800: if(!(cp=nv_getval(sh_scoped(shp,PS4NOD)))) 2801: cp = "+ "; 2802: else 2803: { 2804: sh_offoption(SH_XTRACE); 2805: cp = sh_mactry(shp,cp); 2806: sh_onoption(SH_XTRACE); 2807: } sh_mactry() needs to parse the contents of $PS4 to perform expansions and command substitutions in it, which involves the lexer. If that happens in a here-document, the lexer is in the C function call stack, in the middle of parsing the here-document. Result: inconsistent lexer state. Solution: save and restore lexer state in sh_mactry(). After this commit, all regression tests should pass with the '-x'/'--xtrace' option in use, with no errors or crashes. Note for backporters: this fix depends both on on d7cada7b and on the consistency fix for the Lex_t type's size applied in a7ed5d9f. src/cmd/ksh93/include/shlex.h: - Cosmetic fix: remove a copied & pasted backslash. (re: a7ed5d9f) src/cmd/ksh93/sh/macro.c: sh_mactry(): - Save and restore the lexer state before letting sh_mactrim() indirectly parse and execute code. src/cmd/ksh93/tests/*.sh: - Turn off xtrace in various command substitutions that contain 2>&1 redirections, so that the xtrace output is not caught by the command substitutions, causing tests to fail incorrectly. - Turn off xtrace for a few code blocks with 2>&1 redirections, stopping xtrace output from being written to standard output. Resolves: https://github.com/ksh93/ksh/issues/306 (again) --- NEWS | 4 ++++ src/cmd/ksh93/include/shlex.h | 2 +- src/cmd/ksh93/sh/macro.c | 3 +++ src/cmd/ksh93/tests/arith.sh | 2 +- src/cmd/ksh93/tests/enum.sh | 2 +- src/cmd/ksh93/tests/io.sh | 2 +- src/cmd/ksh93/tests/return.sh | 21 +++++++++++++-------- src/cmd/ksh93/tests/signal.sh | 4 ++-- src/cmd/ksh93/tests/types.sh | 2 +- src/cmd/ksh93/tests/variables.sh | 2 +- 10 files changed, 28 insertions(+), 16 deletions(-) diff --git a/NEWS b/NEWS index 7cb844e8f..06f6592f6 100644 --- a/NEWS +++ b/NEWS @@ -8,6 +8,10 @@ Any uppercase BUG_* names are modernish shell bug IDs. - Fixed a crash or freeze that would occur on Linux when using Ctrl+C to interrupt a command substitution containing a pipe in an interactive shell. +- Fixed a crash that could occur while processing a here-document while + xtrace (set -x) is on and the $PS4 prompt contains parameter expansions or + command substitutions. + 2021-12-26: - Listing aliases or tracked aliases in a script no longer corrupts diff --git a/src/cmd/ksh93/include/shlex.h b/src/cmd/ksh93/include/shlex.h index 2f93f65dd..1720620f3 100644 --- a/src/cmd/ksh93/include/shlex.h +++ b/src/cmd/ksh93/include/shlex.h @@ -103,7 +103,7 @@ typedef struct _shlex_ Dt_t *entity_tree; /* for entity ids */ #endif /* SHOPT_KIA */ /* The following two struct members are considered private to lex.c */ - struct _shlex_pvt_lexdata_ lexd; \ + struct _shlex_pvt_lexdata_ lexd; struct _shlex_pvt_lexstate_ lex; } Lex_t; diff --git a/src/cmd/ksh93/sh/macro.c b/src/cmd/ksh93/sh/macro.c index 85afd8da5..4010276d4 100644 --- a/src/cmd/ksh93/sh/macro.c +++ b/src/cmd/ksh93/sh/macro.c @@ -124,6 +124,7 @@ void *sh_macopen(Shell_t *shp) /* * perform only parameter substitution and catch failures + * (also save lexer state to allow use while in here-docs) */ char *sh_mactry(Shell_t *shp,register char *string) { @@ -132,11 +133,13 @@ char *sh_mactry(Shell_t *shp,register char *string) int jmp_val; int savexit = shp->savexit; struct checkpt buff; + Lex_t *lexp = (Lex_t*)sh.lex_context, savelex = *lexp; sh_pushcontext(shp,&buff,SH_JMPSUB); jmp_val = sigsetjmp(buff.buff,0); if(jmp_val == 0) string = sh_mactrim(shp,string,0); sh_popcontext(shp,&buff); + *lexp = savelex; shp->savexit = savexit; return(string); } diff --git a/src/cmd/ksh93/tests/arith.sh b/src/cmd/ksh93/tests/arith.sh index 516465f4f..000fe802d 100755 --- a/src/cmd/ksh93/tests/arith.sh +++ b/src/cmd/ksh93/tests/arith.sh @@ -943,7 +943,7 @@ fi # ====== # https://github.com/ksh93/ksh/issues/334#issuecomment-968603087 exp=21 -got=$(typeset -Z x=0x15; { echo $((x)); } 2>&1) +got=$(typeset -Z x=0x15; set +x; { echo $((x)); } 2>&1) [[ $got == "$exp" ]] || err_exit "typeset -Z corrupts hexadecimal number in arithmetic context" \ "(expected $(printf %q "$exp"), got $(printf %q "$got"))" diff --git a/src/cmd/ksh93/tests/enum.sh b/src/cmd/ksh93/tests/enum.sh index d8402492d..af1fb3832 100755 --- a/src/cmd/ksh93/tests/enum.sh +++ b/src/cmd/ksh93/tests/enum.sh @@ -150,7 +150,7 @@ a=blue; let "a*=3" 2>/dev/null && err_exit "arithmetic can assign out of range ( # Enum types should parse with 'command' prefix(es) and options and instantly # recognise subsequent builtins it creates, even as a oneliner, even with # shcomp. (This requires an ugly parser hack that this tests for.) -got=$(eval 2>&1 'command command command enum -i -i -iii --igno -ii PARSER_t=(r g b); '\ +got=$(set +x; eval 2>&1 'command command command enum -i -i -iii --igno -ii PARSER_t=(r g b); '\ 'command command PARSER_t -r -rrAAA -A -rArArA -Arrrrrrr hack=([C]=G); typeset -p hack') exp='PARSER_t -r -A hack=([C]=g)' [[ $got == "$exp" ]] || err_exit "incorrect typeset output for enum with command prefix and options" \ diff --git a/src/cmd/ksh93/tests/io.sh b/src/cmd/ksh93/tests/io.sh index 44da71f61..92fe16435 100755 --- a/src/cmd/ksh93/tests/io.sh +++ b/src/cmd/ksh93/tests/io.sh @@ -593,7 +593,7 @@ result=$("$SHELL" -c 'echo ok > >(sed s/ok/good/); wait' 2>&1) # Process substitution in an interactive shell or profile script shouldn't # print the process ID of the asynchronous process. echo 'false >(false)' > "$tmp/procsub-envtest" -result=$(ENV=$tmp/procsub-envtest "$SHELL" -ic 'true >(true)' 2>&1) +result=$(set +x; ENV=$tmp/procsub-envtest "$SHELL" -ic 'true >(true)' 2>&1) [[ -z $result ]] || err_exit 'interactive shells and/or profile scripts print a PID during process substitution' \ "(expected '', got $(printf %q "$result"))" diff --git a/src/cmd/ksh93/tests/return.sh b/src/cmd/ksh93/tests/return.sh index c4b182b8c..b268421bb 100755 --- a/src/cmd/ksh93/tests/return.sh +++ b/src/cmd/ksh93/tests/return.sh @@ -171,6 +171,9 @@ fi # ====== # Tests for 'return' and 'exit' without argument: they should pass down the previous exit status +# +# Some of these tests use 'function foo' instead of foo() to locally turn off +# xtrace; otherwise the 2>&1 redirection would write xtrace to standard output. foo() { return; } false @@ -187,24 +190,26 @@ foo && err_exit "'return &&' does not preserve exit status" foo() { false; while return; do true; done; } foo && err_exit "'while return' does not preserve exit status" -foo() { false; while return; do true; done 2>&1; } +function foo { false; while return; do true; done 2>&1; } foo && err_exit "'while return' with redirection does not preserve exit status" foo() { false; until return; do true; done; } foo && err_exit "'until return' does not preserve exit status" -foo() { false; until return; do true; done 2>&1; } +function foo { false; until return; do true; done 2>&1; } foo && err_exit "'until return' with redirection does not preserve exit status" foo() { false; for i in 1; do return; done; } foo && err_exit "'return' within 'for' does not preserve exit status" -foo() { false; for i in 1; do return; done 2>&1; } +function foo { false; for i in 1; do return; done 2>&1; } foo && err_exit "'return' within 'for' with redirection does not preserve exit status" -foo() { false; { return; } 2>&1; } +function foo { false; { return; } 2>&1; } foo && err_exit "'return' within { block; } with redirection does not preserve exit status" +# Subshell functions. These have no ksh variant, but we can just turn off xtrace directly in them (set +x). + foo() ( exit ) false foo && err_exit "'exit' within function does not preserve exit status" @@ -220,22 +225,22 @@ foo && err_exit "'exit &&' does not preserve exit status" foo() ( false; while exit; do true; done ) foo && err_exit "'while exit' does not preserve exit status" -foo() ( false; while exit; do true; done 2>&1 ) +foo() ( set +x; false; while exit; do true; done 2>&1 ) foo && err_exit "'while exit' with redirection does not preserve exit status" foo() ( false; until exit; do true; done ) foo && err_exit "'until exit' does not preserve exit status" -foo() ( false; until exit; do true; done 2>&1 ) +foo() ( set +x; false; until exit; do true; done 2>&1 ) foo && err_exit "'until exit' with redirection does not preserve exit status" foo() ( false; for i in 1; do exit; done ) foo && err_exit "'exit' within 'for' does not preserve exit status" -foo() ( false; for i in 1; do exit; done 2>&1 ) +foo() ( set +x; false; for i in 1; do exit; done 2>&1 ) foo && err_exit "'exit' within 'for' with redirection does not preserve exit status" -foo() ( false; { exit; } 2>&1 ) +foo() ( set +x; false; { exit; } 2>&1 ) foo && err_exit "'exit' within { block; } with redirection does not preserve exit status" foo() { false; (exit); } diff --git a/src/cmd/ksh93/tests/signal.sh b/src/cmd/ksh93/tests/signal.sh index ee757eeef..14662c8d6 100755 --- a/src/cmd/ksh93/tests/signal.sh +++ b/src/cmd/ksh93/tests/signal.sh @@ -538,7 +538,7 @@ cat > exit267 <<-EOF # unquoted delimiter; expansion active foo EOF exp="OK $((signum+256))" -got=$( { "$SHELL" exit267; } 2>&1 ) +got=$( set +x; { "$SHELL" exit267; } 2>&1 ) (( (e=$?)==signum+128 )) && [[ $got == "$exp" ]] || err_exit "'return' with status > 256:" \ "(expected status $((signum+128)) and $(printf %q "$exp"), got status $e and $(printf %q "$got"))" @@ -549,7 +549,7 @@ cat > bar <<-'EOF' echo OK EOF exp="OK" -got=$( { "$SHELL" bar; } 2>&1 ) +got=$( set +x; { "$SHELL" bar; } 2>&1 ) (( (e=$?)==0 )) && [[ $got == "$exp" ]] || err_exit "segfaulting child process:" \ "(expected status 0 and $(printf %q "$exp"), got status $e and $(printf %q "$got"))" diff --git a/src/cmd/ksh93/tests/types.sh b/src/cmd/ksh93/tests/types.sh index dea1c386e..e88f9db50 100755 --- a/src/cmd/ksh93/tests/types.sh +++ b/src/cmd/ksh93/tests/types.sh @@ -467,7 +467,7 @@ cat > B_t <<- \EOF EOF unset n -if n=$(FPATH=$PWD PATH=$PWD:$PATH "$SHELL" -c 'A_t a; print ${a.b.n}' 2>&1) +if n=$(set +x; FPATH=$PWD PATH=$PWD:$PATH "$SHELL" -c 'A_t a; print ${a.b.n}' 2>&1) then [[ $n == '5' ]] || err_exit "dynamic loading of types gives wrong result (got $(printf %q "$n"))" else err_exit "unable to load types dynamically (got $(printf %q "$n"))" fi diff --git a/src/cmd/ksh93/tests/variables.sh b/src/cmd/ksh93/tests/variables.sh index e32469fba..cade47cf6 100755 --- a/src/cmd/ksh93/tests/variables.sh +++ b/src/cmd/ksh93/tests/variables.sh @@ -822,7 +822,7 @@ Errors=$? # ensure error count survives subshell ( # $x must be an unknown locale. for x in x x.b@d xx_XX xx_XX.b@d - do errmsg=$({ LANG=$x; } 2>&1) + do errmsg=$(set +x; { LANG=$x; } 2>&1) [[ -n $errmsg ]] && break done if [[ -z $errmsg ]]