Skip to content

Commit

Permalink
shcomp: fix redirection with process substitution
Browse files Browse the repository at this point in the history
The commands within a process substitution used as an argument to a
redirection (e.g. < <(...) or > >(...)) are simply not included in
parse trees dumped by shcomp. This can be verified with a command
like hexdump -C. As a result, these process substitutions do not
work when running a bytecode-compiled shell script.

The fix is surprisingly simple. A process substitution is encoded
as a complete parse tree. When used with a redirection, that parse
tree is used as the file name for the redirection. All we need to
do is treat the "file name" as a parse tree instead of a string if
flags indicate a process substitution.

A process substitution is detected by the struct ionod field
'iofile'. Checking the IOPROCSUB bit flag is not enough. We also
need to exclude the IOLSEEK flag as that form of redirection may
use the IOARITH flag which has the same bit value as IOPROCSUB (see
include/shnodes.h).

src/cmd/ksh93/sh/tdump.c: p_redirect():
- Call p_tree() instead of p_string() for a process substitution.

src/cmd/ksh93/sh/trestore.c: r_redirect():
- Call r_tree() instead of r_string() for a process substitution.

src/cmd/ksh93/include/version.h:
- Bump the shcomp binary header version as this change is not
  backwards compatible; previous trestore.c versions don't know how
  to read the newly compiled process substitutions and would crash.

src/cmd/ksh93/tests/io.sh:
- Add test.

src/cmd/ksh93/tests/builtins.sh,
src/cmd/ksh93/tests/options.sh:
- Revert shcomp workarounds. (re: 6701bb3)

Resolves: #165
  • Loading branch information
McDutchie committed Apr 22, 2021
1 parent b7dde4e commit 32d1abb
Show file tree
Hide file tree
Showing 7 changed files with 29 additions and 17 deletions.
5 changes: 5 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -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-22:

- shcomp (the shell bytecode compiler) was fixed to correctly compile process
substitutions used as the file name to a redirection, as in 'cmd < <(cmd)'.

2021-04-21:

- Fixed a bug introduced on 2020-09-28 that caused an interactive ksh to exit
Expand Down
7 changes: 2 additions & 5 deletions 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-04-21" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_DATE "2021-04-22" /* 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 All @@ -39,8 +39,5 @@
* For shcomp: the version number (0-255) for the binary bytecode header.
* Only increase very rarely, i.e.: if incompatible changes are made that
* cause bytecode from newer versions to fail on older versions of ksh.
*
* The version number was last increased in 2021 for ksh 93u+m because
* most of the predefined aliases were converted to builtin commands.
*/
#define SHCOMP_HDR_VERSION 4
#define SHCOMP_HDR_VERSION 5
5 changes: 4 additions & 1 deletion src/cmd/ksh93/sh/tdump.c
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,10 @@ static int p_redirect(register const struct ionod *iop)
sfputl(outfile,iop->iofile|IOVNM);
else
sfputl(outfile,iop->iofile);
p_string(iop->ioname);
if((iop->iofile & IOPROCSUB) && !(iop->iofile & IOLSEEK))
p_tree((Shnode_t*)iop->ioname); /* process substitution as file name to redirection */
else
p_string(iop->ioname); /* file name, descriptor, etc. */
if(iop->iodelim)
{
p_string(iop->iodelim);
Expand Down
5 changes: 4 additions & 1 deletion src/cmd/ksh93/sh/trestore.c
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,10 @@ static struct ionod *r_redirect(Shell_t* shp)
else
iopold->ionxt = iop;
iop->iofile = l;
iop->ioname = r_string(shp->stk);
if((l & IOPROCSUB) && !(l & IOLSEEK))
iop->ioname = (char*)r_tree(shp); /* process substitution as file name to redirection */
else
iop->ioname = r_string(shp->stk); /* file name, descriptor, etc. */
if(iop->iodelim = r_string(shp->stk))
{
iop->iosize = sfgetl(infile);
Expand Down
5 changes: 1 addition & 4 deletions src/cmd/ksh93/tests/builtins.sh
Original file line number Diff line number Diff line change
Expand Up @@ -1012,9 +1012,6 @@ 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 )
Expand All @@ -1028,7 +1025,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< "$tmp/builtin-list"
done 3< <(builtin)
# ======
# The 'alarm' builtin could make 'read' crash due to IFS table corruption caused by unsafe asynchronous execution.
Expand Down
11 changes: 11 additions & 0 deletions src/cmd/ksh93/tests/io.sh
Original file line number Diff line number Diff line change
Expand Up @@ -841,5 +841,16 @@ exp='Foo bar'
[[ $exp == $got ]] || err_exit "BUG_CSUBSTDO: Closing stdout outside of command substitution breaks stdout inside of command substitution" \
"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
# ======
# In shcomp, process substitution did not work when used as the file name to a redirection.
# https://github.com/ksh93/ksh/issues/165
# Note: avoid testing this in a command substitution, as those are always parsed at runtime,
# meaning shcomp will also include them as literal source text instead of compiling them.
echo ok > >(cat >out1)
wait "$!" # the procsub is run asynchronously, so wait before reading from the file
cat >out2 < <(case x in x) cat out1;; esac)
[[ $(<out2) == ok ]] || err_exit "process substitution not working as file name to redirection" \
"(expected 'ok', got $(printf %q "$(<out2)"))"
# ======
exit $((Errors<125?Errors:125))
8 changes: 2 additions & 6 deletions src/cmd/ksh93/tests/options.sh
Original file line number Diff line number Diff line change
Expand Up @@ -537,12 +537,8 @@ 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"

# 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"
$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"
fi

# ======
Expand Down

0 comments on commit 32d1abb

Please sign in to comment.