Re-enable SHOPT_DEVFD, fixing process substitution fd leaks (#218)

This commit fixes a long-standing bug (present since at least
ksh93r) that caused a file descriptor leak when passing a process
substitution to a function, or (if compiled with SHOPT_SPAWN) to a
nonexistent command.

The leaks only occurred when ksh was compiled with SHOPT_DEVFD; the
FIFO method was unaffected.

src/cmd/ksh93/sh/xec.c: sh_exec():
- When a process substitution is passed to a built-in, the
  remaining file descriptor is closed with sh_iorestore. Do the
  same thing when passing a process substitution to a function.
  This is done by delaying the sh_iorestore() call to 'setexit:'
  where both built-ins and functions terminate and set the exit
  status ($?).
  This means that call now will not be executed if a longjmp is
  done, e.g. due to an error in a special built-in. However, there
  is already another sh_iorestore() call in main.c, exfile(), line
  418, that handles that scenario.
- sh_ntfork() can fail, so rather than assume it will succeed,
  handle a failure by closing extra file descriptors with
  sh_iorestore(). This fixes the leak on command not found with
  SHOPT_SPAWN.

src/cmd/ksh93/include/defs.h:
- Since the file descriptor leaks are now fixed, remove the
  workaround that forced ksh to use the FIFO method.

src/cmd/ksh93/SHOPT.sh:
- Add SHOPT_DEVFD as a configurable option (default: probe).

src/cmd/ksh93/tests/io.sh:
- Add a regression test for the 'not found' file descriptor leak.
- Add a test to ensure it keeps working with 'command'.

Fixes: https://github.com/ksh93/ksh/issues/67

NEWS

@@ -3,6 +3,14 @@ For full details, see the git log at: https://github.com/ksh93/ksh
 
 Any uppercase BUG_* names are modernish shell bug IDs.
 
+2021-03-13:
+
+- Fixed a file descriptor leak that occurred when ksh used /dev/fd for
+  process substitutions passed to functions.
+
+- Fixed a separate file descriptor leak that happened when a process
+  substitution was passed to a nonexistent command.
+
 2021-03-11:
 
 - Fixed an intermittent bug that caused process substitutions to infinitely

src/cmd/ksh93/SHOPT.sh

@@ -13,6 +13,7 @@ SHOPT BRACEPAT=1			# C-shell {...,...} expansions (, required)
 SHOPT CMDLIB_HDR=  # '<cmdlist.h>'	# custom -lcmd list for path-bound builtins
 SHOPT CMDLIB_DIR=  # '\"/opt/ast/bin\"'	# virtual directory prefix for path-bound builtins
 SHOPT CRNL=				# accept MS Windows newlines (<cr><nl>) for <nl>
+SHOPT DEVFD=				# use /dev/fd instead of FIFOs for process substitutions
 SHOPT DYNAMIC=1				# dynamic loading for builtins
 SHOPT ECHOPRINT=			# make echo equivalent to print
 SHOPT EDPREDICT=1			# predictive editing

src/cmd/ksh93/include/defs.h

@@ -43,14 +43,6 @@
 #   define mbmax()	1
 #endif
 
-/*
- * Until a file descriptor leak with process substitution
- * is fixed, disable /dev/fd use to avoid the problem.
- * https://github.com/ksh93/ksh/issues/67
- */
-#undef SHOPT_DEVFD
-#define SHOPT_DEVFD	0
-
 #include	<sfio.h>
 #include	<error.h>
 #include	"FEATURE/externs"

src/cmd/ksh93/include/version.h

@@ -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-03-11"	/* must be in this format for $((.sh.version)) */
+#define SH_RELEASE_DATE	"2021-03-13"	/* 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. */

src/cmd/ksh93/sh/args.c

@@ -758,7 +758,7 @@ struct argnod *sh_argprocsub(Shell_t *shp,struct argnod *argp)
 	job.jobcontrol = savejobcontrol;
 	sh_setstate(savestates);
 #if SHOPT_DEVFD
-	close(pv[1-fd]);
+	sh_close(pv[1-fd]);
 	sh_iosave(shp,-pv[fd], shp->topfd, (char*)0);
 #else
 	/* remember the FIFO for cleanup in case the command never opens it (see fifo_cleanup(), xec.c) */

src/cmd/ksh93/sh/xec.c

@@ -1014,7 +1014,7 @@ int sh_exec(register const Shnode_t *t, int flags)
 			char		*trap;
 			Namval_t	*np, *nq, *last_table;
 			struct ionod	*io;
-			int		command=0, flgs=NV_ASSIGN;
+			int		command=0, flgs=NV_ASSIGN, jmpval=0;
 			shp->bltindata.invariant = type>>(COMBITS+2);
 			type &= (COMMSK|COMSCAN);
 			sh_stats(STAT_SCMDS);
@@ -1258,7 +1258,7 @@ int sh_exec(register const Shnode_t *t, int flags)
 					volatile int scope=0, share=0;
 					volatile void *save_ptr;
 					volatile void *save_data;
-					int jmpval, save_prompt;
+					int save_prompt;
 					int was_nofork = execflg?sh_isstate(SH_NOFORK):0;
 					struct checkpt *buffp = (struct checkpt*)stkalloc(shp->stk,sizeof(struct checkpt));
 #if SHOPT_VSH
@@ -1440,10 +1440,6 @@ int sh_exec(register const Shnode_t *t, int flags)
 						sh_unscope(shp);
 					bp->ptr = (void*)save_ptr;
 					bp->data = (void*)save_data;
-					/* don't restore for 'exec' or 'redirect' in subshell */
-					if((shp->topfd>topfd) && !(shp->subshell && (np==SYSEXEC || np==SYSREDIR)))
-						sh_iorestore(shp,topfd,jmpval);
-			
 					shp->redir0 = 0;
 					if(jmpval)
 						siglongjmp(*shp->jmplist,jmpval);
@@ -1456,7 +1452,6 @@ int sh_exec(register const Shnode_t *t, int flags)
 				if(!command && np && nv_isattr(np,NV_FUNCTION))
 				{
 					volatile int indx;
-					int jmpval=0;
 					struct checkpt *buffp = (struct checkpt*)stkalloc(shp->stk,sizeof(struct checkpt));
 #if SHOPT_NAMESPACE
 					Namval_t node, *namespace=0;
@@ -1557,6 +1552,8 @@ int sh_exec(register const Shnode_t *t, int flags)
 #if !SHOPT_DEVFD
 				fifo_cleanup();
 #endif
+				if(shp->topfd > topfd && !(shp->subshell && (np==SYSEXEC || np==SYSREDIR)))
+					sh_iorestore(shp,topfd,jmpval);
 				exitset();
 				break;
 			}
@@ -1631,6 +1628,10 @@ int sh_exec(register const Shnode_t *t, int flags)
 #   endif /* _lib_fork */
 				if(parent<0)
 				{
+					/* prevent a file descriptor leak from a 'not found' error */
+					if(shp->topfd > topfd)
+						sh_iorestore(shp,topfd,0);
+
 					if(shp->comsub==1 && usepipe && unpipe)
 						sh_iounpipe(shp);
 					break;

src/cmd/ksh93/tests/io.sh

@@ -629,6 +629,7 @@ fi
 
 # ======
 # File descriptor leak with process substitution
+# https://github.com/ksh93/ksh/issues/67
 err=$(
 	ulimit -n 15 || exit 0
 	fdUser() {
@@ -641,6 +642,21 @@ err=$(
 ) || err_exit 'Process substitution leaks file descriptors when used as argument to function' \
 	"(got $(printf %q "$err"))"
 
+# File descriptor leak after 'command not found' with process substitution as argument
+err=$(
+	ulimit -n 25 || exit 0
+	set +x
+	PATH=/dev/null
+	for ((i=1; i<10; i++))
+	do	notfound <(:) >(:) 2> /dev/null
+	done 2>&1
+	exit 0
+) || err_exit "Process substitution leaks file descriptors when used as argument to nonexistent command" \
+	"(got $(printf %q "$err"))"
+
+got=$(command -x cat <(command -x echo foo) 2>&1) || err_exit "process substitution doesn't work with 'command'" \
+	"(got $(printf %q "$got"))"
+
 # ======
 # A redirection with a null command could crash under certain circumstances (rhbz#1200534)
 "$SHELL" -i >/dev/null 2>&1 -c '