From 70368c57d67c6d3f327a52de39697e8c6647ee7a Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Sun, 24 Jan 2021 15:46:46 +0000 Subject: [PATCH] Fix field splitting bug triggered by DEBUG trap An unquoted variable expansion evaluated in a DEBUG trap action caused IFS field splitting to be deactivated in code executed after the trap action. Thanks to Koichi Nakashima for the reproducer: | v='' | trap ': $v' DEBUG | A="a b c" | set -- $A | printf '%s\n' "$@" | | Expected | | a | b | c | | Actual | | a b c src/cmd/ksh93/sh/fault.c: sh_trap(): - Remove incorrect save/restore of sh.ifstable, the internal state table for field splitting. This reverts three lines added in ksh 93t+ 2009-11-30. Analysis: As an expansion is split into fields (macro.c, lines 2367-2471), sh.ifstable is modified. If that happens within a DEBUG trap, any modifications in ifstable are undone by the restoring memccpy, leaving an inconsistent state. src/cmd/ksh93/COMPATIBILITY: - Document the DEBUG trap fixes, particularly the incorrect inheritance by subshells and functions that some scripts may now rely on because this bug is so longstanding. (re: 2a835a2d) src/cmd/ksh93/tests/basic.sh: - Add relevant tests. Resolves: https://github.com/ksh93/ksh/issues/155 TODO: add a -T (-o functrace) option as in bash, which should allow subshells and ksh-style functions to inherit DEBUG traps. P.S.: The very handy multishell repo allows us to use 'git blame' to trace the origin of the recently fixed DEBUG trap bugs. The off-by-one error causing various bugs, reverted in 2a835a2d, was introduced in ksh 93t 2008-07-25: https://github.com/multishell/ksh93/commit/8e947ccf (fault.c, line 321) The incorrect check causing the exit status bug, reverted in d00b4b39, was introduced in ksh 93t 2008-11-04: https://github.com/multishell/ksh93/commit/b1ade268 (fault.c, line 459) The ifstable save/restore causing the field splitting bug, reverted in this commit, was introduced in ksh 93t+ 2009-11-30: https://github.com/multishell/ksh93/commit/53d9f009 (fault.c, lines 440, 444, 482) So all the bugs reported in #155 were fixed by simply reverting these specific changes. I think that they are some experiments that the developers simply forgot to remove. I've suspected such a thing multiple times before. ksh93 was developed by researchers who were genius innovators, but incredibly sloppy maintainers. --- NEWS | 9 +++++ src/cmd/ksh93/COMPATIBILITY | 4 +++ src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/sh/fault.c | 3 -- src/cmd/ksh93/tests/basic.sh | 59 +++++++++++++++++++++++++++++++-- 5 files changed, 70 insertions(+), 7 deletions(-) diff --git a/NEWS b/NEWS index 5d5c289c5..6c6b3a323 100644 --- a/NEWS +++ b/NEWS @@ -3,11 +3,19 @@ For full details, see the git log at: https://github.com/ksh93/ksh Any uppercase BUG_* names are modernish shell bug IDs. +2021-01-24: + +- Fixed: an unquoted variable expansion evaluated in a DEBUG trap action caused + IFS field splitting to be deactivated in code executed after the trap action. + This bug was introduced in ksh 93t+ 2009-11-30. + 2021-01-23: - Fixed: when the DEBUG trap was redefined in a subshell, the DEBUG trap in the parent environment was corrupted or the shell crashed. When a redirection was used in a DEBUG trap action, the trap was disabled. + DEBUG traps were also incorrectly inherited by subshells and ksh functions. + All this was caused by a bug introduced in ksh 93t 2008-07-25. 2021-01-22: @@ -18,6 +26,7 @@ Any uppercase BUG_* names are modernish shell bug IDs. - Fixed: executing a DEBUG trap in a command substitution had side effects on the exit status ($?) of non-trap commands. + This bug was introduced in ksh 93t 2008-11-04. - The typeset builtin command now gives an informative error message if an incompatible combination of options is given. diff --git a/src/cmd/ksh93/COMPATIBILITY b/src/cmd/ksh93/COMPATIBILITY index 970f78e01..c3d5e0c1f 100644 --- a/src/cmd/ksh93/COMPATIBILITY +++ b/src/cmd/ksh93/COMPATIBILITY @@ -80,6 +80,10 @@ For more details, see the NEWS file and for complete details, see the git log. 12. The 'typeset' builtin now properly detects and reports options that cannot be used together if they are given as part of the same command. +13. The DEBUG trap has reverted to pre-93t behavior. It is now once again + reset like other traps upon entering a subshell or ksh-style function, + as documented, and it is no longer prone to crash or get corrupted. + ____________________________________________________________________________ KSH-93 VS. KSH-88 diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index 3bd045bc4..98088afc7 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -20,7 +20,7 @@ #define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */ #define SH_RELEASE_SVER "1.0.0-alpha" /* semantic version number: https://semver.org */ -#define SH_RELEASE_DATE "2021-01-23" /* must be in this format for $((.sh.version)) */ +#define SH_RELEASE_DATE "2021-01-24" /* must be in this format for $((.sh.version)) */ #define SH_RELEASE_CPYR "(c) 2020-2021 Contributors to ksh " SH_RELEASE_FORK /* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */ diff --git a/src/cmd/ksh93/sh/fault.c b/src/cmd/ksh93/sh/fault.c index 1873b6378..91378ed7e 100644 --- a/src/cmd/ksh93/sh/fault.c +++ b/src/cmd/ksh93/sh/fault.c @@ -451,11 +451,9 @@ int sh_trap(const char *trap, int mode) int was_verbose = sh_isstate(SH_VERBOSE); int staktop = staktell(); char *savptr = stakfreeze(0); - char ifstable[256]; struct checkpt buff; Fcin_t savefc; fcsave(&savefc); - memcpy(ifstable,shp->ifstable,sizeof(ifstable)); sh_offstate(SH_HISTORY); sh_offstate(SH_VERBOSE); shp->intrap++; @@ -493,7 +491,6 @@ int sh_trap(const char *trap, int mode) shp->exitval=savxit; stakset(savptr,staktop); fcrestore(&savefc); - memcpy(shp->ifstable,ifstable,sizeof(ifstable)); if(was_history) sh_onstate(SH_HISTORY); if(was_verbose) diff --git a/src/cmd/ksh93/tests/basic.sh b/src/cmd/ksh93/tests/basic.sh index db2ac777b..405a02215 100755 --- a/src/cmd/ksh93/tests/basic.sh +++ b/src/cmd/ksh93/tests/basic.sh @@ -745,8 +745,9 @@ got=$(eval 'x=`for i in test; do case $i in test) true;; esac; done`' 2>&1) \ || err_exit "case in a for loop inside a \`comsub\` caused syntax error (got $(printf %q "$got"))" # ====== +# Various DEBUG trap fixes: https://github.com/ksh93/ksh/issues/155 + # Redirecting disabled the DEBUG trap -# https://github.com/ksh93/ksh/issues/155 (#1) exp=$'LINENO: 4\nfoo\nLINENO: 5\nLINENO: 6\nbar\nLINENO: 7\nbaz' got=$({ "$SHELL" -c ' PATH=/dev/null @@ -760,7 +761,6 @@ got=$({ "$SHELL" -c ' "(got status $e$( ((e>128)) && print -n / && kill -l "$e"), $(printf %q "$got"))" # The DEBUG trap crashed when re-trapping inside a subshell -# https://github.com/ksh93/ksh/issues/155 (#2) exp=$'trap -- \': main\' EXIT\ntrap -- \': main\' ERR\ntrap -- \': main\' KEYBD\ntrap -- \': main\' DEBUG' got=$({ "$SHELL" -c ' PATH=/dev/null @@ -774,8 +774,20 @@ got=$({ "$SHELL" -c ' ((!(e = $?))) && [[ $got == "$exp" ]] || err_exit 'Pseudosignal trap failed when re-trapping in subshell' \ "(got status $e$( ((e>128)) && print -n / && kill -l "$e"), $(printf %q "$got"))" +# Field splitting broke upon evaluating an unquoted expansion in a DEBUG trap +exp=$'a\nb\nc' +got=$({ "$SHELL" -c ' + PATH=/dev/null + v="" + trap ": \$v" DEBUG + A="a b c" + set -- $A + printf "%s\n" "$@" +'; } 2>&1) +((!(e = $?))) && [[ $got == "$exp" ]] || err_exit 'Field splitting broke after executing DEBUG trap' \ + "(got status $e$( ((e>128)) && print -n / && kill -l "$e"), $(printf %q "$got"))" + # The DEBUG trap had side effects on the exit status -# https://github.com/ksh93/ksh/issues/155 (#4) trap ':' DEBUG (exit 123) (((e=$?)==123)) || err_exit "DEBUG trap run in subshell affects exit status (expected 123, got $e)" @@ -785,5 +797,46 @@ r=`exit 123` (((e=$?)==123)) || err_exit "DEBUG trap run in \`comsub\` affects exit status (expected 123, got $e)" trap - DEBUG +# The DEBUG trap was incorrectly inherited by subshells +exp=$'Subshell\nDebug 1\nParentshell' +got=$( + trap 'echo Debug ${.sh.subshell}' DEBUG + (echo Subshell) + echo Parentshell +) +trap - DEBUG # bug compat +[[ $got == "$exp" ]] || err_exit "DEBUG trap inherited by subshell" \ + "(expected $(printf %q "$exp"), got $(printf %q "$got"))" + +# The DEBUG trap was incorrectly inherited by ksh functions +exp=$'Debug 0\nFunctionEnv\nDebug 0\nParentEnv' +got=$( + function myfn + { + echo FunctionEnv + } + trap 'echo Debug ${.sh.level}' DEBUG + myfn + echo ParentEnv +) +trap - DEBUG # bug compat +[[ $got == "$exp" ]] || err_exit "DEBUG trap inherited by ksh function" \ + "(expected $(printf %q "$exp"), got $(printf %q "$got"))" + +# Make sure the DEBUG trap is still inherited by POSIX functions +exp=$'Debug 0\nDebug 1\nFunction\nDebug 0\nNofunction' +got=$( + myfn() + { + echo Function + } + trap 'echo Debug ${.sh.level}' DEBUG + myfn + echo Nofunction +) +trap - DEBUG # bug compat +[[ $got == "$exp" ]] || err_exit "DEBUG trap not inherited by POSIX function" \ + "(expected $(printf %q "$exp"), got $(printf %q "$got"))" + # ====== exit $((Errors<125?Errors:125))