1
0
Fork 0
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:
Johnothan King 2021-04-15 10:29:50 -07:00 committed by GitHub
parent 2fdf394b99
commit 6701bb30de
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 31 additions and 10 deletions

5
NEWS
View file

@ -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

View file

@ -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. */

View file

@ -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))
{

View file

@ -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:

View file

@ -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.

View file

@ -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

View file

@ -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
# ======

View file

@ -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