From be5ea8bbb27cd39c2451e49b9d080ed40ba3f4e5 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Sat, 8 Aug 2020 23:57:57 +0100 Subject: [PATCH] redirect: check args before executing redirections (re: 7b82c338) The 'redirect' builtin command did not error out before executing any valid redirections. For example, 'redirect ls >foo.txt' issued an "incorrect syntax" error, but still created 'foo.txt' and left standard output permanently redirected to it. src/cmd/ksh93/sh/xec.c: sh_exec(): - If we have redirections (io != NULL), and the command is SYSREDIR, then check for arguments and error out if there are any, before calling sh_redirect() to execute redirections. (Note, the other check for arguments in b_exec() in bltins/misc.c must be kept, as that applies if there are no redirections.) src/cmd/ksh93/sh/io.c: sh_redirect(): - Edit comments to better explain what the flag values do. src/cmd/ksh93/bltins/misc.c: - Add a dummy b_redirect() function declaration "for the dictionary generator" as has historically been done for other builtins that share one C function. I'm not sure what that dictionary generator is supposed to be, but this also improves greppability. src/cmd/ksh93/data/builtins.c, src/cmd/ksh93/sh.1: - Fix misleading "I/O redirection arguments" term. I/O redirections are not arguments at all; no argument parser ever sees them. src/cmd/ksh93/tests/io.sh: - Test both conditions that should make 'redirect' produce an "incorrect syntax" error. - Test that any redirections are not executed if erroneous non-redirection arguments exist. src/cmd/ksh93/tests/builtins.sh: - "... should show usage info on unrecognized options" test: Because 'redirect' now refuses to process redirections on error, the error message was not captured. The fix is to run the builtin in a braces block and add the redirection to the block. --- NEWS | 7 +++++++ src/cmd/ksh93/bltins/misc.c | 4 ++++ src/cmd/ksh93/data/builtins.c | 4 ++-- src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/sh.1 | 4 ++-- src/cmd/ksh93/sh/io.c | 7 ++++--- src/cmd/ksh93/sh/xec.c | 9 +++++++-- src/cmd/ksh93/tests/builtins.sh | 2 +- src/cmd/ksh93/tests/io.sh | 12 +++++++++--- 9 files changed, 37 insertions(+), 14 deletions(-) diff --git a/NEWS b/NEWS index 8290c1e70..d04460288 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,13 @@ For full details, see the git log at: https://github.com/ksh93/ksh Any uppercase BUG_* names are modernish shell bug IDs. +2020-08-08: + +- Argument checking in the 'redirect' builtin command (see 2020-06-11) has + been improved to error out before executing redirections. For example, an + error like 'redirect ls >foo.txt' now will not create 'foo.txt' and will + not leave your standard output permanently redirected to it. + 2020-08-07: - Fixed a crash that occurred intermittently when entering the 60 second diff --git a/src/cmd/ksh93/bltins/misc.c b/src/cmd/ksh93/bltins/misc.c index 196c04c83..a5627b116 100644 --- a/src/cmd/ksh93/bltins/misc.c +++ b/src/cmd/ksh93/bltins/misc.c @@ -76,6 +76,10 @@ static void noexport(register Namval_t* np, void *data) /* * 'exec' special builtin and 'redirect' builtin */ +#if 0 +/* for the dictionary generator */ +int b_redirect(int argc,char *argv[],Shbltin_t *context){} +#endif int b_exec(int argc,char *argv[], Shbltin_t *context) { struct login logdata; diff --git a/src/cmd/ksh93/data/builtins.c b/src/cmd/ksh93/data/builtins.c index bcd59d021..6e001a14a 100644 --- a/src/cmd/ksh93/data/builtins.c +++ b/src/cmd/ksh93/data/builtins.c @@ -1461,9 +1461,9 @@ USAGE_LICENSE ; const char sh_optredirect[] = -"[-1c?\n@(#)$Id: redirect (ksh93) 2020-06-11 $\n]" +"[-1c?\n@(#)$Id: redirect (ksh93) 2020-08-08 $\n]" "[+NAME?redirect - open/close and duplicate file descriptors]" -"[+DESCRIPTION?This command only accepts input/output redirection arguments. " +"[+DESCRIPTION?This command only accepts input/output redirections. " "It can open and close files and modify file descriptors from \b0\b " "to \b9\b using the standard redirection mechanism available to all " "commands, with the difference that the effect persists past the " diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index 8145f8a48..9c60971ab 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -17,4 +17,4 @@ * David Korn * * * ***********************************************************************/ -#define SH_RELEASE "93u+m 2020-08-07" +#define SH_RELEASE "93u+m 2020-08-08" diff --git a/src/cmd/ksh93/sh.1 b/src/cmd/ksh93/sh.1 index 4ed099f6e..000cb2975 100644 --- a/src/cmd/ksh93/sh.1 +++ b/src/cmd/ksh93/sh.1 @@ -5910,7 +5910,7 @@ to become for the new process. If .I arg\^ -is not given and only I/O redirection arguments are given, +is not given and only I/O redirections are given, then this command persistently modifies file descriptors as in .BR redirect. .TP @@ -6792,7 +6792,7 @@ When defining a type, if the value of a readonly sub-variable is not defined the value is required when creating each instance. .TP \f3redirect\fP -This command only accepts input/output redirection arguments. +This command only accepts input/output redirections. It can open and close files and modify file descriptors from .B 0 to diff --git a/src/cmd/ksh93/sh/io.c b/src/cmd/ksh93/sh/io.c index 26ba79c86..f1e72f4b2 100644 --- a/src/cmd/ksh93/sh/io.c +++ b/src/cmd/ksh93/sh/io.c @@ -1074,9 +1074,10 @@ static char *io_usename(char *name, int *perm, int fno, int mode) /* * I/O redirection - * flag = 0 if files are to be restored - * flag = 2 if files are to be closed on exec - * flag = 3 when called from $( < ...), just open file and return + * flag = 0: normal redirection; file descriptors are restored when command terminates + * flag = 1: redirections persist + * flag = 2: redirections persist, but file descriptors > 2 are closed when executing an external command + * flag = 3: just open file and return; for use when called from $( < ...) * flag = SH_SHOWME for trace only */ int sh_redirect(Shell_t *shp,struct ionod *iop, int flag) diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c index 255f47713..c79dc6b7b 100644 --- a/src/cmd/ksh93/sh/xec.c +++ b/src/cmd/ksh93/sh/xec.c @@ -1273,8 +1273,13 @@ int sh_exec(register const Shnode_t *t, int flags) if(io) { struct openlist *item; - if(np==SYSEXEC || np==SYSREDIR) /* 'exec' or 'redirect' */ - type=1+!com[1]; /* redirections persist if no args */ + if(np == SYSEXEC) /* 'exec' */ + type = 1 + !com[1]; + else if(np == SYSREDIR) /* 'redirect' */ + if(!com[1]) + type = 2; + else + errormsg(SH_DICT,ERROR_exit(2),"redirect: %s",e_badsyntax); else type = (execflg && !shp->subshell && !shp->st.trapcom[0]); shp->redir0 = 1; diff --git a/src/cmd/ksh93/tests/builtins.sh b/src/cmd/ksh93/tests/builtins.sh index 1952c867a..bb9cc3fa3 100755 --- a/src/cmd/ksh93/tests/builtins.sh +++ b/src/cmd/ksh93/tests/builtins.sh @@ -877,7 +877,7 @@ EOF ( builtin $(builtin -l | awk -F "/" '/\/opt/ {print $5}') # Load all /opt/ast/bin builtins for name in $(builtin -l | grep -Ev '(echo|/opt|test|true|false|getconf|uname|\[|:)'); do - actual="$($name --this-option-does-not-exist 2>&1)" + actual=$({ "$name" --this-option-does-not-exist; } 2>&1) expect="Usage: $name" [[ $actual =~ $expect ]] || err_exit "$name should show usage info on unrecognized options (expected $(printf '%q' "$expect"), got $(printf '%q' "$actual"))" done diff --git a/src/cmd/ksh93/tests/io.sh b/src/cmd/ksh93/tests/io.sh index 73f7fc862..2dfd036e9 100755 --- a/src/cmd/ksh93/tests/io.sh +++ b/src/cmd/ksh93/tests/io.sh @@ -430,6 +430,7 @@ then export LC_ALL=$lc_utf8 done fi fi +LC_ALL=C command true # restore correct shellquoting on old ksh: https://github.com/ksh93/ksh/issues/5 file=$tmp/file ( @@ -528,9 +529,14 @@ actual=$(redirect 2>&- 3>&2; echo ok) [[ $actual == ok ]] || err_exit "redirection error in 'redirect' causes exit" # Test that 'redirect' does not accept non-redir args -actual=$(redirect ls 2>&1) -expect="*: redirect: incorrect syntax" -[[ $actual == $expect ]] || err_exit "redirect command wrongly accepting non-redir args" +expect=$'*: redirect: incorrect syntax\n status = 2' +actual=$( (redirect /dev/null/foo) 2>&1; echo " status = $?" ); +[[ $actual == $expect ]] || err_exit 'redirect command accepts non-redirection argument' \ + "(expected $(printf %q "$expect"), got $(printf %q "$actual"))" +actual=$( (redirect /dev/null/foo >$tmp/wrong_redirect) 2>&1; echo " status = $?" ) +[[ $actual == $expect ]] || err_exit 'redirect command accepts non-redirection argument along with redirection' \ + "(expected $(printf %q "$expect"), got $(printf %q "$actual"))" +[[ -e $tmp/wrong_redirect ]] && err_exit "redirect command executes redirections before checking arguments" # ====== # Process substitution