Skip to content

Commit

Permalink
Fix field splitting bug triggered by DEBUG trap
Browse files Browse the repository at this point in the history
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: 2a835a2)

src/cmd/ksh93/tests/basic.sh:
- Add relevant tests.

Resolves: #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 2a835a2,
was introduced in ksh 93t 2008-07-25:
multishell/ksh93@8e947ccf
(fault.c, line 321)

The incorrect check causing the exit status bug, reverted in
d00b4b3, was introduced in ksh 93t 2008-11-04:
multishell/ksh93@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:
multishell/ksh93@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.
  • Loading branch information
McDutchie committed Jan 24, 2021
1 parent e664b78 commit 70368c5
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 7 deletions.
9 changes: 9 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand All @@ -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.
Expand Down
4 changes: 4 additions & 0 deletions src/cmd/ksh93/COMPATIBILITY
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/include/version.h
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down
3 changes: 0 additions & 3 deletions src/cmd/ksh93/sh/fault.c
Original file line number Diff line number Diff line change
Expand Up @@ -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++;
Expand Down Expand Up @@ -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)
Expand Down
59 changes: 56 additions & 3 deletions src/cmd/ksh93/tests/basic.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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)"
Expand All @@ -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))

0 comments on commit 70368c5

Please sign in to comment.