mirror of
git://git.code.sf.net/p/cdesktopenv/code
synced 2025-03-09 15:50:02 +00:00
Fix <>; redirection for final command exec optimization (#277)
The <>; operator doesn't work correctly if it's used as the last
command of a -c script. Reproducer:
$ echo test > a; ksh -c 'echo x 1<>; a'; cat a
x
st
This bug is caused by ksh running the last command of -c scripts
with execve(2) instead of posix_spawn(3) or fork(2). The <>;
operator is noted by the man page as being incompatible with the
exec builtin (see also the ksh93u+ man page), so it's not
surprising this bug occurs when ksh runs a command using execve:
> <>;word cannot be used with the exec and redirect built-ins.
The ksh2020 fix simply removed the code required for ksh to use
this optimization at all. It's not a performance friendly fix and
only papers over the bug, so this commit provides a better fix.
This bug was first reported at:
https://github.com/att/ast/issues/9
In addition, this commit re-enables the execve(2) optimization for
the last command for scripts loaded from a file. It was enabled in
in older ksh versions, and was only disabled in interactive shells:
https://github.com/ksh93/ast-open-history/blob/2011-06-30/src/cmd/ksh93/sh/main.c#L593-L599
It was changed on 2011-12-24 to only be used for -c scripts:
https://github.com/ksh93/ast-open-history/blob/2011-12-24/src/cmd/ksh93/sh/main.c#L593-L599
We think there is no good reason why scripts loaded from a file
should be optimised less than scripts loaded from a -c argument.
They're both scripts; there's no essential difference between them.
So this commit reverts that change. If there is a bug left in the
optimization after this fix, this revert increases the chance of
exposing it so that it can be fixed.
src/cmd/ksh93/sh/xec.c:
- The IOREWRITE flag is set when handling the <>; operator, so to
fix this bug, avoid exec'ing the last command if it uses <>;. See
also commit 17ebfbf6
, which fixed another issue related to the
execve optimization.
src/cmd/ksh93/tests/io.sh:
- Enable a regression test that was failing because of this bug.
- Add the reproducer from https://github.com/att/ast/issues/9 as a
regression test.
src/cmd/ksh93/sh/main.c:
- Only avoid the non-forking optimization in interactive shells.
src/cmd/ksh93/tests/signal.sh:
- Add an extra comment to avoid the non-forking optimization in the
regression test for rhbz#1469624.
- If the regression test for rhbz#1469624 fails, show the incorrect
exit status in the error message.
src/cmd/ksh93/tests/builtins.sh,
src/cmd/ksh93/tests/options.sh:
- This bugfix was causing the options regression test to segfault
when run under shcomp. The cause is the same as
<https://github.com/ksh93/ksh/issues/165>, so as a workaround,
avoid parsing process substitutions with shcomp until that is
fixed. This workaround should also avoid the other problem
detailed in <https://github.com/ksh93/ksh/issues/274>.
Resolves: https://github.com/ksh93/ksh/issues/274
This commit is contained in:
parent
2fdf394b99
commit
6701bb30de
8 changed files with 31 additions and 10 deletions
5
NEWS
5
NEWS
|
@ -3,6 +3,11 @@ For full details, see the git log at: https://github.com/ksh93/ksh
|
|||
|
||||
Any uppercase BUG_* names are modernish shell bug IDs.
|
||||
|
||||
2021-04-15:
|
||||
|
||||
- Fixed an optimization bug that caused the <>; redirection operator to fail
|
||||
when used in -c scripts.
|
||||
|
||||
2021-04-14:
|
||||
|
||||
- Path-bound built-ins (such as /opt/ast/bin/cat) can now be executed by
|
||||
|
|
|
@ -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-04-14" /* must be in this format for $((.sh.version)) */
|
||||
#define SH_RELEASE_DATE "2021-04-15" /* 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. */
|
||||
|
|
|
@ -575,7 +575,7 @@ static void exfile(register Shell_t *shp, register Sfio_t *iop,register int fno)
|
|||
{
|
||||
execflags = sh_state(SH_ERREXIT)|sh_state(SH_INTERACTIVE);
|
||||
/* The last command may not have to fork */
|
||||
if(!sh_isstate(SH_PROFILE) && sh_isoption(SH_CFLAG) &&
|
||||
if(!sh_isstate(SH_PROFILE) && !sh_isstate(SH_INTERACTIVE) &&
|
||||
(fno<0 || !(shp->fdstatus[fno]&(IOTTY|IONOSEEK)))
|
||||
&& !sfreserve(iop,0,0))
|
||||
{
|
||||
|
|
|
@ -1003,8 +1003,8 @@ int sh_exec(register const Shnode_t *t, int flags)
|
|||
shp->exitval=0;
|
||||
shp->lastsig = 0;
|
||||
shp->lastpath = 0;
|
||||
if(shp->exittrap || shp->errtrap)
|
||||
execflg = 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:
|
||||
|
|
|
@ -986,6 +986,9 @@ EOF
|
|||
|
||||
# ======
|
||||
# Builtins should handle unrecognized options correctly
|
||||
# Note: To workaround https://github.com/ksh93/ksh/issues/165, put the list
|
||||
# of builtins in a file, then read from that.
|
||||
builtin > "$tmp/builtin-list"
|
||||
while IFS= read -r bltin <&3
|
||||
do case $bltin in
|
||||
echo | test | true | false | \[ | : | getconf | */getconf | uname | */uname | catclose | catgets | catopen | Dt* | _Dt* | X* | login | newgrp )
|
||||
|
@ -999,7 +1002,7 @@ do case $bltin in
|
|||
esac
|
||||
[[ $actual == *"$expect"* ]] || err_exit "$bltin should show usage info on unrecognized options" \
|
||||
"(expected string containing $(printf %q "$expect"), got $(printf %q "$actual"))"
|
||||
done 3< <(builtin)
|
||||
done 3< "$tmp/builtin-list"
|
||||
|
||||
# ======
|
||||
# The 'alarm' builtin could make 'read' crash due to IFS table corruption caused by unsafe asynchronous execution.
|
||||
|
|
|
@ -500,12 +500,19 @@ actual=$(cat "$tmp/nums1")
|
|||
[[ "$actual" = "$expect" ]] || err_exit "Failed to truncate file in subshell \
|
||||
(expected $(printf %q "$expect"), got $(printf %q "$actual"))"
|
||||
|
||||
: <<\INACTIVE # TODO: the >#5 is optimised away by a '-c' optimisation corner case bug
|
||||
# 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"))"
|
||||
INACTIVE
|
||||
|
||||
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"))"
|
||||
|
||||
# ======
|
||||
# Exit behaviour of 'exec', 'command exec', 'redirect' on redirections
|
||||
|
|
|
@ -537,8 +537,12 @@ fi # SHOPT_BRACEPAT
|
|||
(set --default -o posix; [[ -o letoctal ]]) && err_exit "set --default failed to stop posix option from changing others"
|
||||
(set --posix; [[ -o letoctal ]]) || err_exit "set --posix fails to enable letoctal"
|
||||
(set -o posix; [[ -o letoctal ]]) || err_exit "set -o posix fails to enable letoctal"
|
||||
$SHELL --posix < <(echo 'exit 0') || err_exit "ksh fails to handle --posix during startup"
|
||||
$SHELL -o posix < <(echo 'exit 0') || err_exit "ksh fails to handle -o posix during startup"
|
||||
|
||||
# Note: To workaround erratic behavior caused by https://github.com/ksh93/ksh/issues/165,
|
||||
# avoid parsing the process substitutions with shcomp by running
|
||||
# the tests with eval.
|
||||
eval "$SHELL --posix < <(echo 'exit 0')" || err_exit "ksh fails to handle --posix during startup"
|
||||
eval "$SHELL -o posix < <(echo 'exit 0')" || err_exit "ksh fails to handle -o posix during startup"
|
||||
fi
|
||||
|
||||
# ======
|
||||
|
|
|
@ -450,6 +450,7 @@ let "$? == 256+9" && err_exit 'exit with status > 256 makes shell kill itself'
|
|||
cat >"$tmp/sigtest.sh" <<\EOF
|
||||
echo begin
|
||||
"$1" -c 'kill -9 "$$"'
|
||||
# this extra comment disables an exec optimization
|
||||
EOF
|
||||
expect=$'^begin\n/.*/sigtest.sh: line 2: [1-9][0-9]*: Killed\n[1-9][0-9]{1,2}$'
|
||||
actual=$(LANG=C "$SHELL" -c '"$1" "$2" "$1"; echo "$?"' x "$SHELL" "$tmp/sigtest.sh" 2>&1)
|
||||
|
@ -457,7 +458,8 @@ if ! [[ $actual =~ $expect ]]
|
|||
then [[ $actual == *Killed*Killed* ]] && msg='ksh killed itself' || msg='unexpected output'
|
||||
err_exit "$msg after child process signal (expected match to $(printf %q "$expect"); got $(printf %q "$actual"))"
|
||||
fi
|
||||
let "${actual##*$'\n'} > 128" || err_exit "child process signal did not cause exit status > 128"
|
||||
let "${actual##*$'\n'} > 128" || err_exit "child process signal did not cause exit status > 128" \
|
||||
"(got ${actual##*$'\n'})"
|
||||
|
||||
# ======
|
||||
# Killing a non-existent job shouldn't cause a segfault. Note that `2> /dev/null` has no effect when
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue