Fix unused process substitutions hanging (#214)

On systems where ksh needs to use the older and less secure FIFO
method for process substitutions (which is currently all of them as
the more modern and solid /dev/fd method is still broken, see #67),
process substitutions could leave background processes hanging in
these two scenarios:

1. If the parent process exits without opening a pipe to the child
   process forked by the process substitution. The fifo_check()
   function in xec.c, which is periodically called to check if the
   parent process still exists while waiting for it to open the
   FIFO, verified the parent process's existence by checking if the
   PPID had reverted to 1, the traditional PID of init. However,
   POSIX specifies that the PPID can revert to any implementation-
   defined system process in that case. So this breaks on certain
   systems, causing unused process substitutions to hang around
   forever as they never detect that the parent disappeared.
   The fix is to save the current PID before forking and having the
   child check if the PPID has changed from that saved PID.

2. If command invoked from the main shell is passed a process
   substitution, but terminates without opening the pipe to the
   process substitution. In that case, the parent process never
   disappears in the first place, because the parent process is the
   main shell. So the same infinite wait occurs in unused process
   substitutions, even after correcting problem 1.
   The fix is to remember all FIFOs created for any number of
   process substitutions passed to a single command, and unlink any
   remaining FIFOs as they represent unused command substitutions.
   Unlinking them FIFOs causes sh_open() in the child to fail with
   ENOENT on the next periodic check, which can easily be handled.

Fixing these problems causes the FIFO method to act identically to
the /dev/fd method, which is good for compatibility. Even when #67
is fixed this will still be important, as ksh also runs on systems
that do not have /dev/fd (such as AIX, HP-UX, and QNX), so will
fall back to using FIFOs.

--- Fix problem 1 ---

src/cmd/ksh93/sh/xec.c:
- Add new static fifo_save_ppid variable.
- sh_exec(): If a FIFO is defined, save the current PID in
  fifo_save_ppid for the forked child to use.
- fifo_check(): Compare PPID against the saved value instead of 1.

--- Fix problem 2 ---

To keep things simple I'm abusing the name-value pair routines used
for variables for this purpose. The overhead is negligible. A more
elegant solution is possible but would involve adding more code.

src/cmd/ksh93/include/defs.h: _SH_PRIVATE:
- Define new sh.fifo_tree pointer to a new FIFO cleanup tree.

src/cmd/ksh93/sh/args.c: sh_argprocsubs():
- After launching a process substitution in the background,
  add the FIFO to the cleanup list before freeing it.

src/cmd/ksh93/sh/xec.c:
- Add fifo_cleanup() that unlinks all FIFOs in the cleanup list and
  clears/closes the list. They should only still exist if the
  command never used them, however, just run 'unlink' and don't
  check for existence first as that would only add overhead.
- sh_exec():
  * Call fifo_cleanup() on finishing all simple commands (when
    setting $?) or when a special builtin fails.
  * When forking, clear/close the cleanup list; we do not want
    children doing duplicate cleanup, particularly as this can
    interfere when using multiple process substitutions in one
    command.
  * Process substitution handling:
    > Change FIFO check frequency from 500ms to 50ms.
      Note that each check sends a signal that interrupts open(2),
      causing sh_open() to reinvoke it. This causes sh_open() to
      fail with ENOENT on the next check when the FIFO no longer
      exists, so we do not need to add an additional check for
      existence to fifo_check(). Unused process substitutions now
      linger for a maximum of 50ms.
    > Do not issue an error message if errno == ENOENT.
- sh_funct(): Process substitutions can be passed to functions as
  well, and we do not want commands within the function to clean up
  the FIFOs for the process substitutions passed to it from the
  outside. The problem is solved by simply saving fifo_tree in a
  local variable, setting it to null before running the function,
  and cleaning it up before restoring the parent one at the end.
  Since sh_funct() is called recursively for multiple-level
  function calls, this correctly gives each function a locally
  scoped fifo_tree.

--- Tests ---

src/cmd/ksh93/tests/io.sh:
- Add tests covering the failing scenarios.

Co-authored-by: Martijn Dekker <martijn@inlv.org>

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-11:
+
+- Fixed an intermittent bug that caused process substitutions to infinitely
+  loop in Linux virtual machines that use systemd.
+
+- Fixed a bug that caused process substitutions to leave lingering processes
+  if the command invoking them never reads from them.
+
 2021-03-09:
 
 - The ${!foo@} and ${!foo*} expansions yield variable names beginning with foo,

src/cmd/ksh93/include/defs.h

@@ -158,7 +158,7 @@ struct shared
 	Shwait_f	waitevent;
 };
 
-#define _SH_PRIVATE \
+#define __SH_PRIVATE_1 \
 	struct shared	*gd;		/* global data */ \
 	struct sh_scoped st;		/* scoped information */ \
 	Stk_t		*stk;		/* stack pointer */ \
@@ -189,7 +189,6 @@ struct shared
 	char		*comdiv;	/* points to sh -c argument */ \
 	char		*prefix;	/* prefix for compound assignment */ \
 	sigjmp_buf	*jmplist;	/* longjmp return stack */ \
-	char		*fifo;		/* fifo name for process sub */ \
 	pid_t		bckpid;		/* background process id */ \
 	pid_t		cpid; \
 	pid_t		spid; 		/* subshell process id */ \
@@ -282,6 +281,14 @@ struct shared
 	char 		exittrap; \
 	char 		errtrap; \
 	char 		end_fn;
+#if !SHOPT_DEVFD
+#define __SH_PRIVATE_2 \
+	char		*fifo;		/* FIFO name for current process substitution */ \
+	Dt_t		*fifo_tree;	/* for cleaning up process substitution FIFOs */
+#else
+#define	__SH_PRIVATE_2
+#endif
+#define _SH_PRIVATE	__SH_PRIVATE_1 __SH_PRIVATE_2
 
 #include	<shell.h>
 

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

@@ -761,6 +761,10 @@ struct argnod *sh_argprocsub(Shell_t *shp,struct argnod *argp)
 	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) */
+	if(!shp->fifo_tree)
+		shp->fifo_tree = dtopen(&_Nvdisc,Dtoset);
+	nv_search(shp->fifo,shp->fifo_tree,NV_ADD);
 	free(shp->fifo);
 	shp->fifo = 0;
 #endif /* SHOPT_DEVFD */

src/cmd/ksh93/sh/xec.c

@@ -82,16 +82,34 @@ struct funenv
 /* ========	command execution	========*/
 
 #if !SHOPT_DEVFD
+    static pid_t fifo_save_ppid;
+
     static void fifo_check(void *handle)
     {
 	Shell_t	*shp = (Shell_t*)handle;
-	pid_t pid = getppid();
-	if(pid==1)
+	if(getppid() != fifo_save_ppid)
 	{
 		unlink(shp->fifo);
 		sh_done(shp,0);
 	}
     }
+
+    /* Remove any remaining FIFOs to stop unused process substitutions blocking on trying to open the FIFO */
+    static void fifo_cleanup(void)
+    {
+	if(sh.fifo_tree)
+	{
+		Namval_t	*fifo = dtfirst(sh.fifo_tree);
+		if(fifo)
+		{
+			do
+				unlink(fifo->nvname);
+			while(fifo = dtnext(sh.fifo_tree,fifo));
+			dtclose(sh.fifo_tree);
+			sh.fifo_tree = NIL(Dt_t*);
+		}
+	}
+    }
 #endif /* !SHOPT_DEVFD */
 
 #if _lib_getrusage
@@ -1371,6 +1389,9 @@ int sh_exec(register const Shnode_t *t, int flags)
 						/* failure on special built-ins fatal */
 						if(jmpval<=SH_JMPCMD  && (!nv_isattr(np,BLT_SPC) || command))
 							jmpval=0;
+#if !SHOPT_DEVFD
+						fifo_cleanup();
+#endif
 					}
 					if(bp)
 					{
@@ -1533,6 +1554,9 @@ int sh_exec(register const Shnode_t *t, int flags)
 			else if(!io)
 			{
 			setexit:
+#if !SHOPT_DEVFD
+				fifo_cleanup();
+#endif
 				exitset();
 				break;
 			}
@@ -1591,6 +1615,10 @@ int sh_exec(register const Shnode_t *t, int flags)
 					pipes[2] = 0;
 					coproc_init(shp,pipes);
 				}
+#if !SHOPT_DEVFD
+				if(shp->fifo)
+					fifo_save_ppid = shgd->current_pid;
+#endif
 #if SHOPT_SPAWN
 #   ifdef _lib_fork
 				if(com && !job.jobcontrol)
@@ -1678,6 +1706,14 @@ int sh_exec(register const Shnode_t *t, int flags)
 				struct checkpt *buffp = (struct checkpt*)stkalloc(shp->stk,sizeof(struct checkpt));
 				struct ionod *iop;
 				int	rewrite=0;
+#if !SHOPT_DEVFD
+				if(shp->fifo_tree)
+				{
+					/* do not clean up process substitution FIFOs in child; parent handles this */
+					dtclose(shp->fifo_tree);
+					shp->fifo_tree = NIL(Dt_t*);
+				}
+#endif
 				if(no_fork)
 					sh_sigreset(2);
 				sh_pushcontext(shp,buffp,SH_JMPEXIT);
@@ -1704,16 +1740,24 @@ int sh_exec(register const Shnode_t *t, int flags)
 #if !SHOPT_DEVFD
 				if(shp->fifo && (type&(FPIN|FPOU)))
 				{
-					int	fn,fd = (type&FPIN)?0:1;
-					void	*fifo_timer=sh_timeradd(500,1,fifo_check,(void*)shp);
+					int	fn, fd, save_errno;
+					void	*fifo_timer = sh_timeradd(50,1,fifo_check,(void*)shp);
+					fd = (type&FPIN) ? 0 : 1;
 					fn = sh_open(shp->fifo,fd?O_WRONLY:O_RDONLY);
+					save_errno = errno;
 					timerdel(fifo_timer);
-					sh_iorenumber(shp,fn,fd);
-					sh_close(fn);
-					sh_delay(.001,0);
 					unlink(shp->fifo);
 					free(shp->fifo);
 					shp->fifo = 0;
+					if(fn<0)
+					{
+						if((errno = save_errno) != ENOENT)
+							errormsg(SH_DICT, ERROR_SYSTEM|ERROR_PANIC,
+								 "process substitution: FIFO open failed");
+						sh_done(shp,0);
+					}
+					sh_iorenumber(shp,fn,fd);
+					sh_close(fn);
 					type &= ~(FPIN|FPOU);
 				}
 #endif /* !SHOPT_DEVFD */
@@ -3229,6 +3273,10 @@ static void sh_funct(Shell_t *shp,Namval_t *np,int argn, char *argv[],struct arg
 	char *fname = nv_getval(SH_FUNNAMENOD);
 	struct Level	*lp =(struct Level*)(SH_LEVELNOD->nvfun);
 	int		level, pipepid=shp->pipepid;
+#if !SHOPT_DEVFD
+	Dt_t		*save_fifo_tree = shp->fifo_tree;
+	shp->fifo_tree = NIL(Dt_t*);
+#endif
 	shp->pipepid = 0;
 	sh_stats(STAT_FUNCT);
 	if(!lp->hdr.disc)
@@ -3283,6 +3331,10 @@ static void sh_funct(Shell_t *shp,Namval_t *np,int argn, char *argv[],struct arg
 			_nv_unset(np, NV_RDONLY);
 		}
 	}
+#if !SHOPT_DEVFD
+	fifo_cleanup();
+	shp->fifo_tree = save_fifo_tree;
+#endif
 }
 
 /*

src/cmd/ksh93/tests/io.sh

@@ -715,5 +715,36 @@ got=$(export tmp; "$SHELL" -ec \
 (redirect {v}>$tmp/v.out; echo ok2 >&$v) 2>/dev/null
 [[ -r $tmp/v.out && $(<$tmp/v.out) == ok2 ]] || err_exit 'redirect {varname}>file not working in a subshell'
 
+# ======
+# Test for looping or lingering process substitution processes
+# https://github.com/ksh93/ksh/issues/213
+procsub_pid=$(
+	ulimit -t unlimited 2>/dev/null  # fork the subshell
+	true >(true) <(true) >(true) <(true)
+	echo "$!"
+)
+sleep .1
+if kill -0 "$procsub_pid" 2>/dev/null; then
+	kill -TERM "$procsub_pid" # don't leave around what is effectively a zombie process
+	err_exit "process substitutions loop or linger after parent shell finishes"
+fi
+(true <(true) >(true) <(true) >(true); wait) &
+sleep .1
+if kill -0 $! 2> /dev/null; then
+	kill -TERM $!
+	err_exit "process substitutions linger when unused"
+fi
+
+# process substitutions should work correctly with delays
+procsub_delay()
+{
+	sleep .1  # a delay >50ms, the current fifo_check delay in xec.c
+	cat "$@"
+}
+exp=$'hi\nthere\nworld'
+got=$(procsub_delay <(echo hi) <(echo there) <(echo world))
+[[ $got == "$exp" ]] || err_exit "process substitutions passed to function failed" \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+
 # ======
 exit $((Errors<125?Errors:125))