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

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

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;

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 "

src/cmd/ksh93/include/version.h

@@ -17,4 +17,4 @@
 *                  David Korn <dgk@research.att.com>                   *
 *                                                                      *
 ***********************************************************************/
-#define SH_RELEASE	"93u+m 2020-08-07"
+#define SH_RELEASE	"93u+m 2020-08-08"

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

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 = 0: normal redirection; file descriptors are restored when command terminates
- * flag = 2 if files are to be closed on exec
+ * flag = 1: redirections persist
- * flag = 3 when called from $( < ...), just open file and return
+ * 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)

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' */
+							if(np == SYSEXEC)		/* 'exec' */
-								type=1+!com[1];		/* redirections persist if no args */
+								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;

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

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\n status = 2'
-expect="*: redirect: incorrect syntax"
+actual=$( (redirect /dev/null/foo) 2>&1; echo " status = $?" );
-[[ $actual == $expect ]] || err_exit "redirect command wrongly accepting non-redir args"
+[[ $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