Make 'redirect' a regular builtin instead of an alias of 'exec'

This commit converts the redirect='command exec' alias to a regular
'redirect' builtin command that only accepts I/O redirections, which
persist as in 'exec'. This means that:
* 'unlias -a' no longer removes the 'redirect' command;
* users no longer accidentally get logged out of their shells if
  they type something intuitive but wrong, like 'redirect ls >file'.

This should not introduce any legitimate change in behaviour. If
someone did accidentally pass non-redirection arguments to
'redirect', unexpected behaviour would occur; this now produces
an 'incorrect syntax' error.

src/cmd/ksh93/bltins/misc.c: b_exec():
- Recognise 'redirect' when parsing options.
- If invoked as 'redirect', produce error if there are arguments.

src/cmd/ksh93/data/aliases.c:
- Remove redirect='command exec' alias.

src/cmd/ksh93/data/builtins.c:
- Update/improve comments re ordering.
- Add 'redirect' builtin entry.
- sh_optexec[]: Abbreviate redirection-related documentation;
  refer to redirect(1) instead.
- sh_optredirect[]: Add documentation.

src/cmd/ksh93/include/builtins.h:
- Add SYSREDIR parser ID, renumbering those following it.
- Improve comments.
- Add extern sh_optredirect[].

src/cmd/ksh93/sh.1:
- exec: Abbreviate redirection-related documentation; refer to
  'redirect' instead.
- redirect: Add documentation.

src/cmd/ksh93/sh/xec.c:
- Recognise SYSREDIR parser ID in addition to SYSEXEC when
  determining whether to make redirections persistent.

src/cmd/ksh93/tests/io.sh:
- To regress-test the new builtin, change most 'command exec' uses
  to 'redirect'.
- Add tests verifying the exit behaviour of 'exec', 'command exec',
  'redirect' on redirections.

NEWS

@@ -10,6 +10,13 @@ Any uppercase BUG_* names are modernish shell bug IDs.
   'alias' or 'unalias', overriding the commands by the same names. In
   technical terms, they are now regular builtins, not special builtins.
 
+- The redirect='command exec' alias has been converted to a regular
+  'redirect' builtin command that only accepts I/O redirections, which
+  persist as in 'exec'. This means that:
+  * 'unlias -a' no longer removes the 'redirect' command;
+  * users no longer accidentally get logged out of their shells if
+    they type something intuitive but wrong, like 'redirect ls >file'.
+
 2020-06-10:
 
 - The 'hash' utility is now a regular builtin instead of an alias to

TODO

@@ -20,14 +20,6 @@ Fix build system:
 ______
 Fix or remove broken or misguided default aliases:
 
-- Make a proper builtin out of the redirect='command exec' alias. It should
-  really only parse redirections. Currently, if an unwitting user notices this
-  alias and tries out something like 'redirect ls >file', it does 'exec ls
-  >file', so 'ls' replaces their shell and they get logged out. That is so
-  misdesigned I'm calling it a bug.
-     Alternatively, maybe just get rid? Who uses this anyway? 'redirect >&2'
-  takes four more keystrokes to type than 'exec >&2'.
-
 - Make proper builtins out of the following scripting-related aliases, so
   that 'unalias -a' does not eliminate them. If done correctly, this causes
   no other change in behaviour. It would be good practice to 'unalias -a' in

src/cmd/ksh93/bltins/misc.c

@@ -60,6 +60,9 @@ struct login
 	char    *arg0;
 };
 
+/*
+ * 'exec' special builtin and 'redirect' builtin
+ */
 int    b_exec(int argc,char *argv[], Shbltin_t *context)
 {
 	struct login logdata;
@@ -68,7 +71,7 @@ int    b_exec(int argc,char *argv[], Shbltin_t *context)
 	logdata.arg0 = 0;
 	logdata.sh = context->shp;
         logdata.sh->st.ioset = 0;
-	while (n = optget(argv, sh_optexec)) switch (n)
+	while (n = optget(argv, *argv[0]=='r' ? sh_optredirect : sh_optexec)) switch (n)
 	{
 	    case 'a':
 		logdata.arg0 = opt_info.arg;
@@ -84,9 +87,11 @@ int    b_exec(int argc,char *argv[], Shbltin_t *context)
 		errormsg(SH_DICT,ERROR_usage(0), "%s", opt_info.arg);
 		return(2);
 	}
-	argv += opt_info.index;
 	if(error_info.errors)
 		errormsg(SH_DICT,ERROR_usage(2),"%s",optusage((char*)0));
+	if(*argv[0]=='r' && argv[opt_info.index])  /* 'redirect' supports no args */
+		errormsg(SH_DICT,ERROR_exit(2),e_badsyntax);
+	argv += opt_info.index;
 	if(*argv)
                 B_login(0,argv,(Shbltin_t*)&logdata);
 	return(0);

src/cmd/ksh93/data/aliases.c

@@ -37,7 +37,6 @@ const struct shtable2 shtab_aliases[] =
 	"integer",	NV_NOFREE|BLT_DCL,	"typeset -li",
 	"nameref",	NV_NOFREE|BLT_DCL,	"typeset -n",
 	"r",		NV_NOFREE,		"hist -s",
-	"redirect",	NV_NOFREE,		"command exec",
 	"source",	NV_NOFREE,		"command .",
 #ifdef SIGTSTP
 	"stop",		NV_NOFREE,		"kill -s STOP",

src/cmd/ksh93/data/builtins.c

@@ -52,12 +52,15 @@
 #undef	dirname
 
 /*
- * The order up through "[" is significant
++  * IMPORTANT: The order of these struct members must be synchronous
++  * with the offsets on the macros defined in include/builtins.h!
++  * The order up through "local" is significant.
  */
 const struct shtable3 shtab_builtins[] =
 {
 	"login",	NV_BLTIN|BLT_ENV,		Bltin(login),
 	"exec",		NV_BLTIN|BLT_ENV|BLT_SPC,	bltin(exec),
+	"redirect",	NV_BLTIN|BLT_ENV,		bltin(exec),
 	"set",		NV_BLTIN|BLT_ENV|BLT_SPC,	bltin(set),	
 	":",		NV_BLTIN|BLT_ENV|BLT_SPC,	bltin(true),
 	"true",		NV_BLTIN|BLT_ENV,		bltin(true),
@@ -75,6 +78,9 @@ const struct shtable3 shtab_builtins[] =
 #if SHOPT_BASH
 	"local",	NV_BLTIN|BLT_ENV|BLT_SPC|BLT_DCL,bltin(typeset),
 #endif
+/*
+ * Builtins without offset macros in include/builtins.h follow.
+ */
 #if _bin_newgrp || _usr_bin_newgrp
 	"newgrp",	NV_BLTIN|BLT_ENV,		Bltin(login),
 #endif	/* _bin_newgrp || _usr_bin_newgrp */
@@ -580,7 +586,7 @@ USAGE_LICENSE
 ;
 
 const char sh_optexec[] =
-"[-1c?\n@(#)$Id: exec (AT&T Research) 1999-07-10 $\n]"
+"[-1c?\n@(#)$Id: exec (AT&T Research) 2020-06-11 $\n]"
 USAGE_LICENSE
 "[+NAME?exec - execute command, open/close and duplicate file descriptors]"
 "[+DESCRIPTION?\bexec\b is a special built-in command that can be used to "
@@ -591,18 +597,11 @@ USAGE_LICENSE
 	"for it to complete.  Note that there is no need to use "
 	"\bexec\b to enhance performance since the shell implicitly "
 	"uses the exec mechanism internally whenever possible.]"
-"[+?If no operands are specified, \bexec\b can be used to open or "
+"[+?If no operands are specified, \bexec\b can be used to persistently open "
-	"close files, or to manipulate file descriptors from \b0\b to "
+	"or close files or manipulate file descriptors as in \bredirect\b(1).]"
-	"\b9\b in the current shell environment using the standard "
-	"redirection mechanism available with all commands.  The " 
-	"close-on-exec flags will be set on file descriptor numbers "
-	"greater than \b2\b that are opened this way so that they "
-	"will be closed when another program is invoked.]"
 "[+?Because \bexec\b is a special command, any failure will cause the "
 	"script that invokes it to exit.  This can be prevented by "
 	"invoking \bexec\b from the \bcommand\b utility.]"
-"[+?\bexec\b cannot be invoked from a restricted shell to create "
-	"files or to open a file for writing or appending.]"
 "[c?Clear all environment variables before executions except variable "
 	"assignments that are part of the current \bexec\b command.]"
 "[a]:[name?\bargv[0]]\b will be set to \aname\a for \acommand\a]"
@@ -614,10 +613,9 @@ USAGE_LICENSE
 	"[+0?All I/O redirections were successful.]"
 	"[+>0?An error occurred.]"
 "}"
-"[+SEE ALSO?\bcommand\b(1), \beval\b(1)]"
+"[+SEE ALSO?\bcommand\b(1), \beval\b(1), \bredirect(1)\b]"
 ;
 
-
 const char sh_optexit[] =
 "[-1c?\n@(#)$Id: exit (AT&T Research) 1999-07-07 $\n]"
 USAGE_LICENSE
@@ -1355,6 +1353,31 @@ USAGE_LICENSE
 "[+SEE ALSO?\bsh\b(1), \btypeset\b(1)]"
 ;
 
+const char sh_optredirect[] =
+"[-1c?\n@(#)$Id: redirect (ksh community) 2020-06-11 $\n]"
+"[+NAME?redirect - open/close and duplicate file descriptors]"
+"[+DESCRIPTION?This command only accepts input/output redirection arguments. "
+	"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 "
+	"execution of the \bredirect\b command.]"
+"[+?Unlike \bexec\b(1), \bredirect\b does not abort the script or command "
+	"line if an error occurs.]"
+"[+?Any file descriptor numbers greater than \b2\b that are opened with this "
+	"mechanism are closed when invoking another program, unless "
+	"explicitly redirected to themselves as part of that invocation.]"
+"[+?\bredirect\b cannot be invoked from a restricted shell to create "
+	"files or to open a file for writing or appending.]"
+"\n"
+"\n[redirection ...]\n"
+"\n"
+"[+EXIT STATUS?The exit status is one of the following:]{"
+	"[+0?All I/O redirections were successful.]"
+	"[+>0?An error occurred.]"
+"}"
+"[+SEE ALSO?\bexec\b(1)]"
+;
+
 const char sh_optreturn[] =
 "[-1c?\n@(#)$Id: return (AT&T Research) 1999-07-07 $\n]"
 USAGE_LICENSE

src/cmd/ksh93/include/builtins.h

@@ -26,23 +26,31 @@
 #include	"FEATURE/dynamic"
 #include	"shtable.h"
 
-#define	SYSLOGIN	(shgd->bltin_cmds)
+/*
-#define SYSEXEC		(shgd->bltin_cmds+1)
+ * IDs for the parser (parse.c) and parse tree executer (xec.c)
-#define SYSSET		(shgd->bltin_cmds+2)
+ * to implement special handling for the corresponding builtins.
-#define SYSTRUE		(shgd->bltin_cmds+4)
+ * IMPORTANT: The offsets on these macros must be synchronous
-#define SYSCOMMAND	(shgd->bltin_cmds+5)
+ * with the order of shtab_builtins[] in data/builtins.c!
-#define SYSCD		(shgd->bltin_cmds+6)
+ */
-#define SYSBREAK	(shgd->bltin_cmds+7)
+#define	SYSLOGIN	(shgd->bltin_cmds)	/* login */
-#define SYSCONT		(shgd->bltin_cmds+8)
+#define SYSEXEC		(shgd->bltin_cmds+1)	/* exec */
-#define SYSTYPESET	(shgd->bltin_cmds+9)
+#define SYSREDIR	(shgd->bltin_cmds+2)	/* redirect */
-#define SYSTEST		(shgd->bltin_cmds+10)
+#define SYSSET		(shgd->bltin_cmds+3)	/* set */
-#define SYSBRACKET	(shgd->bltin_cmds+11)
+						/* : */
-#define SYSLET		(shgd->bltin_cmds+12)
+#define SYSTRUE		(shgd->bltin_cmds+5)	/* true */
-#define SYSEXPORT	(shgd->bltin_cmds+13)
+#define SYSCOMMAND	(shgd->bltin_cmds+6)	/* command */
-#define SYSDOT		(shgd->bltin_cmds+14)
+#define SYSCD		(shgd->bltin_cmds+7)	/* cd */
-#define SYSRETURN	(shgd->bltin_cmds+15)
+#define SYSBREAK	(shgd->bltin_cmds+8)	/* break */
+#define SYSCONT		(shgd->bltin_cmds+9)	/* continue */
+#define SYSTYPESET	(shgd->bltin_cmds+10)	/* typeset */
+#define SYSTEST		(shgd->bltin_cmds+11)	/* test */
+#define SYSBRACKET	(shgd->bltin_cmds+12)	/* [ */
+#define SYSLET		(shgd->bltin_cmds+13)	/* let */
+#define SYSEXPORT	(shgd->bltin_cmds+14)	/* export */
+#define SYSDOT		(shgd->bltin_cmds+15)	/* . */
+#define SYSRETURN	(shgd->bltin_cmds+16)	/* return */
 #if SHOPT_BASH
-#   define SYSLOCAL	(shgd->bltin_cmds+16)
+#   define SYSLOCAL	(shgd->bltin_cmds+17)	/* local */
 #else
 #   define SYSLOCAL	0
 #endif
@@ -153,6 +161,7 @@ extern const char sh_optdot[];
 #endif /* !ECHOPRINT */
 extern const char sh_opteval[];
 extern const char sh_optexec[];
+extern const char sh_optredirect[];
 extern const char sh_optexit[];
 extern const char sh_optexport[];
 extern const char sh_optgetopts[];

src/cmd/ksh93/sh.1

@@ -804,8 +804,6 @@ but can be unset or redefined:
 .TP
 .B "r=\(fmhist \-s\(fm"
 .TP
-.B "redirect=\(fmcommand exec\(fm"
-.TP
 .B "source=\(fmcommand \s+2.\s-2\(fm"
 .TP
 .B "stop=\(fmkill \-s \s-1STOP\s+1\(fm"
@@ -5802,10 +5800,11 @@ In addition, if
 refers to a special built-in,
 none of the special properties associated with the leading
 daggers will be honored.
-(For example, the predefined alias
+(For example, using
-.B "redirect=\(fmcommand exec\(fm"
+.B "command set -o"
+.I "option-name"
 prevents a script from terminating when an invalid
-redirection is given.)
+option name is given.)
 With the
 .B \-x
 option,
@@ -5916,18 +5915,11 @@ rather than the first
 to become
 .B argv[0]
 for the new process.
-Input/output arguments may appear and
-affect the current process.
 If
 .I arg\^
-is not given,
+is not given and only I/O redirection arguments are given,
-the effect of this command is to
+then this command persistently modifies file descriptors as in
-modify file descriptors
+.BR redirect.
-as prescribed by the input/output redirection list.
-In this case,
-any file descriptor numbers greater than 2 that are
-opened with this mechanism are closed when invoking
-another program.
 .TP
 \(dg \f3exit\fP \*(OK \f2n\^\fP \*(CK
 Causes the shell to exit
@@ -6770,6 +6762,23 @@ by subsequent assignment.
 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.
+It can open and close files and modify file descriptors from
+.B 0
+to
+.B 9
+as specified by the input/output redirection list (see the
+.I Input/Output\^
+section above),
+with the difference that the effect persists past the execution of the
+.B redirect
+command.
+Any file descriptor numbers greater than
+.B 2
+that are opened with this mechanism are closed when invoking another program,
+unless explicitly redirected to themselves as part of that invocation.
+.TP
 \(dg \f3return\fP \*(OK \f2n\^\fP \*(CK
 Causes a shell
 .I function

src/cmd/ksh93/sh/xec.c

@@ -1334,8 +1334,8 @@ int sh_exec(register const Shnode_t *t, int flags)
 							struct openlist *item;
 							if(np==SYSLOGIN)
 								type=1;
-							else if(np==SYSEXEC)
+							else if(np==SYSEXEC || np==SYSREDIR)  /* 'exec' or 'redirect' */
-								type=1+!com[1];
+								type=1+!com[1];		      /* redirections persist if no args */
 							else
 								type = (execflg && !shp->subshell && !shp->st.trapcom[0]);
 							shp->redir0 = 1;

src/cmd/ksh93/tests/io.sh

@@ -41,7 +41,7 @@ unset HISTFILE
 
 function fun
 {
-	while  command exec 3>&1
+	while  redirect 3>&1
 	do	break
 	done 2>   /dev/null
 	print -u3 good
@@ -85,7 +85,7 @@ do	[[ -e ${FDFS[fdfs].dir} ]] && { command : > ${FDFS[fdfs].dir}/1; } 2>/dev/nul
 done
 
 exec 3<> file1
-if	command exec 4< ${FDFS[fdfs].dir}/3
+if	redirect 4< ${FDFS[fdfs].dir}/3
 then	read -u3 got
 	read -u4 got
 	exp='foo|bar'
@@ -222,52 +222,52 @@ x="abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNSPQRSTUVWXYZ1234567890"
 for ((i=0; i < 62; i++))
 do	printf "%.39c\n"  ${x:i:1}
 done >  $tmp/seek
-if	command exec 3<> $tmp/seek
+if	redirect 3<> $tmp/seek
 then	(( $(3<#) == 0 )) || err_exit "not at position 0"
 	(( $(3<# ((EOF))) == 40*62 )) || err_exit "not at end-of-file"
-	command exec 3<# ((40*8)) || err_exit "absolute seek fails"
+	redirect 3<# ((40*8)) || err_exit "absolute seek fails"
 	read -u3
 	[[ $REPLY == +(i) ]] || err_exit "expected iiii..., got $REPLY"
 	[[ $(3<#) == $(3<# ((CUR)) ) ]] || err_exit '$(3<#)!=$(3<#((CUR)))'
-	command exec 3<# ((CUR+80))
+	redirect 3<# ((CUR+80))
 	read -u3
 	[[ $REPLY == {39}(l) ]] || err_exit "expected lll..., got $REPLY"
-	command exec 3<# ((EOF-80))
+	redirect 3<# ((EOF-80))
 	read -u3
 	[[ $REPLY == +(9) ]] || err_exit "expected 999..., got $REPLY"
-	command exec 3># ((80))
+	redirect 3># ((80))
 	print -u3 -f "%.39c\n"  @
-	command exec 3># ((80))
+	redirect 3># ((80))
 	read -u3
 	[[ $REPLY == +(@) ]] || err_exit "expected @@@..., got $REPLY"
 	read -u3
 	[[ $REPLY == +(d) ]] || err_exit "expected ddd..., got $REPLY"
-	command exec 3># ((EOF))
+	redirect 3># ((EOF))
 	print -u3 -f "%.39c\n"  ^
 	(( $(3<# ((CUR-0))) == 40*63 )) || err_exit "not at extended end-of-file"
-	command exec 3<# ((40*62))
+	redirect 3<# ((40*62))
 	read -u3
 	[[ $REPLY == +(^) ]] || err_exit "expected ddd..., got $REPLY"
-	command exec 3<# ((0))
+	redirect 3<# ((0))
-	command exec 3<# *jjjj*
+	redirect 3<# *jjjj*
 	read -u3
 	[[  $REPLY == {39}(j) ]] || err_exit "<# pattern failed"
-	[[ $(command exec 3<## *llll*) == {39}(k) ]] || err_exit "<## pattern not saving standard output"
+	[[ $(redirect 3<## *llll*) == {39}(k) ]] || err_exit "<## pattern not saving standard output"
 	read -u3
 	[[  $REPLY == {39}(l) ]] || err_exit "<## pattern failed to position"
-	command exec 3<# *abc*
+	redirect 3<# *abc*
 	read -u3 && err_exit "not found pattern not positioning at eof"
 	cat $tmp/seek | read -r <# *WWW*
 	[[ $REPLY == *WWWWW* ]] || err_exit '<# not working for pipes'
 	{ < $tmp/seek <# ((2358336120)) ;} 2> /dev/null || err_exit 'long seek not working'
 else	err_exit "$tmp/seek: cannot open for reading"
 fi
-command exec 3<&- || 'cannot close 3'
+redirect 3<&- || 'cannot close 3'
 for ((i=0; i < 62; i++))
 do	printf "%.39c\n"  ${x:i:1}
 done >  $tmp/seek
-if	command exec {n}<> $tmp/seek
+if	redirect {n}<> $tmp/seek
-then	{ command exec {n}<#((EOF)) ;} 2> /dev/null || err_exit '{n}<# not working'
+then	{ redirect {n}<#((EOF)) ;} 2> /dev/null || err_exit '{n}<# not working'
 	if	$SHELL -c '{n}</dev/null' 2> /dev/null
 	then	(( $({n}<#) ==  40*62))  || err_exit '$({n}<#) not working'
 	else	err_exit 'not able to parse {n}</dev/null'
@@ -524,5 +524,20 @@ actual=$(cat "$tmp/nums2")
 (expected $(printf %q "$expect"), got $(printf %q "$actual"))"
 INACTIVE
 
+# ======
+# Exit behaviour of 'exec', 'command exec', 'redirect' on redirections
+
+actual=$(exec 2>&- 3>&2; echo should not reach)
+[[ -z $actual ]] || err_exit "redirection error in 'exec' does not cause exit"
+actual=$(command exec 2>&- 3>&2; echo ok)
+[[ $actual == ok ]] || err_exit "redirection error in 'command exec' causes exit"
+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"
+
 # ======
 exit $((Errors<125?Errors:125))