fix(process): unify subprocess ownership and child lifecycle safety (#614)

## Summary

- make subprocess-registry detach atomic so exactly one closer owns each
entry
- make cancellation cleanup release only the detached node it owns
- replace `vfork`/`execve` child setup with `posix_spawn` and explicit
file actions
- mark every pipe and stdio-collision duplicate close-on-exec
- restore the child's default SIGPIPE disposition while preserving the
parent's handling
- bound TERM/KILL reaping and park unreaped children in a fixed-capacity
sweep list
- harden PHP script-server slot ownership, startup, restart, signal,
descriptor, and cleanup paths
- retire and restart failed or oversized reads so stale response tails
cannot poison later data sources
- revalidate every command retry and cap recovery handshakes at two
seconds

This folds the child-process lifecycle portion of #597 into the narrower
ownership PR. It does not absorb unrelated poll-host, SQL, SNMP, ICMP,
or build-infrastructure work.

## Review-driven corrections

- close-on-exec is explicitly restored after `dup()` in both read and
write stdio-collision branches
- the source invariant check now uses portable POSIX character classes
- direct tests cover duplicate flags, both collision branches, failure
after pipe creation, abandoned-child sweeping, and capacity overflow
- collision cleanup preserves current-child redirects even when an older
registry entry occupies fd 0 or 1
- script children receive SIGTERM before SIGKILL and unreaped PHP
children transfer to the bounded sweep registry
- the Docker integration suite now starts a real PHP script server,
verifies its startup protocol and database result, and checks that
shutdown leaves no zombie PHP process

## Validation

- Ubuntu 24.04 container with GCC and GNU ld: `make check`, 6/6 targets
passed with no skips, including `test_php_runtime`
- real PHP script-server Docker integration: 6/6 checks passed; exact
value `42` reached `poller_output` and no zombie remained
- macOS Homebrew dependency build passed
- macOS `make check`: 5/5 available binaries passed; GNU `--wrap`
runtime suite is Linux-only
- PHP fixture syntax checked through mise PHP 8.3
- actionlint, shellcheck, and `git diff --check` passed

Closes #610.

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>

.github/workflows/integration.yml

@@ -3,7 +3,7 @@ name: Integration
 on:
   pull_request:
     branches: [develop]
-    paths: ['**.c', '**.h', 'Dockerfile', '.dockerignore', 'tests/snmpv3/**', '.github/workflows/integration.yml']
+    paths: ['**.c', '**.h', 'Dockerfile', '.dockerignore', 'tests/snmpv3/**', 'tests/integration/test_script_server_reap.sh', '.github/workflows/integration.yml']
   workflow_dispatch:
 
 permissions:
@@ -25,6 +25,9 @@ jobs:
       - name: Poll an SNMPv3 device end to end
         run: tests/snmpv3/scripts/run-integration.sh
 
+      - name: Reap a real PHP script-server process
+        run: tests/integration/test_script_server_reap.sh
+
       - name: Show container logs on failure
         if: failure()
         run: |

CHANGELOG

@@ -26,6 +26,7 @@ The Cacti Group | spine
 -issue#552: Terminate die() output so consecutive fatal messages no longer run together
 -issue#561: Reserve room for the terminator in php_readpipe() so a full script server result cannot write past result_string
 -issue#562: Escalate PHP script server shutdown to SIGKILL after a bounded grace period so a stuck child is not orphaned
+-issue#610: Give nft_pclose exclusive ownership of subprocess registry entries
 -issue#575: Probe for usmDESPrivProtocol instead of assuming the header's advertised default
 -issue#583: Size escaped_result and db_escape()'s trim buffer to the caller's max_size instead of a fixed DBL_BUFSIZE
 -issue#594: Call mysql_thread_end() on the device-row-missing exit path in poll_host()

Dockerfile

@@ -26,6 +26,8 @@ RUN apt-get update && apt-get install -y --no-install-recommends \
         libmariadb3 \
         libsnmp40 \
         libssl3 \
+        php-cli \
+        procps \
         zlib1g \
     && rm -rf /var/lib/apt/lists/*
 

Makefile.am

@@ -80,6 +80,16 @@ check_PROGRAMS += \
 	tests/unit/test_build_fixes \
 	tests/unit/test_safety_fixes \
 	tests/unit/test_linked
+
+# A small executable which speaks the subset of the PHP Script Server protocol
+# needed by test_php_runtime. Keeping it as a real child covers the shipped
+# pipe, spawn, command, response, shutdown and reap paths.
+noinst_PROGRAMS = tests/unit/php_test_server
+tests_unit_php_test_server_SOURCES = tests/unit/php_test_server.c
+
+if HAVE_LD_WRAP
+check_PROGRAMS += tests/unit/test_php_runtime
+endif
 endif
 
 # test_util_strings pulls in common.h and the sources under test, so it needs
@@ -101,4 +111,20 @@ tests_unit_test_linked_SOURCES = tests/unit/test_linked.c tests/fuzz/stubs.c \
 	src/core/keywords.c error.c
 tests_unit_test_linked_LDADD   = $(CMOCKA_LIBS) $(LIBS)
 
-TESTS = $(check_PROGRAMS)
+tests_unit_test_php_runtime_SOURCES = tests/unit/test_php_runtime.c tests/fuzz/stubs.c \
+	sql.c util.c snmp.c src/core/locks.c poller.c nft_popen.c php.c ping.c \
+	src/core/keywords.c error.c
+tests_unit_test_php_runtime_LDADD = $(CMOCKA_LIBS) $(LIBS)
+tests_unit_test_php_runtime_LDFLAGS = \
+	-Wl,--wrap=posix_spawn -Wl,--wrap=dup -Wl,--wrap=fcntl \
+	-Wl,--wrap=pthread_sigmask -Wl,--wrap=write \
+	-Wl,--wrap=posix_spawnattr_setsigdefault -Wl,--wrap=posix_spawnattr_destroy
+tests_unit_test_php_runtime_CPPFLAGS = $(AM_CPPFLAGS) \
+	-DPHP_TEST_SERVER_PATH='"$(abs_builddir)/tests/unit/php_test_server"' \
+	-DSPINE_PHP_RUNTIME_TESTING
+tests_unit_test_php_runtime_DEPENDENCIES = tests/unit/php_test_server$(EXEEXT)
+
+TEST_EXTENSIONS = .sh
+SH_LOG_COMPILER = $(SHELL)
+TESTS = $(check_PROGRAMS) tests/regression/test_child_process_safety.sh
+EXTRA_DIST += tests/regression/test_child_process_safety.sh

configure.ac

@@ -379,6 +379,26 @@ AC_DEFINE_UNQUOTED(SNMP_LOCALNAME, $havelocalname, If snmp localname session str
 
 AC_CHECK_LIB(netsnmp, snmp_timeout)
 
+# ****************** Linker --wrap support (tests only) *********************
+dnl The PHP runtime suite interposes on process and descriptor operations so
+dnl destructive failure paths are deterministic. GNU ld and lld provide the
+dnl required --wrap option; probe it so other supported linkers skip cleanly.
+AC_MSG_CHECKING([whether the linker supports --wrap])
+spine_save_LDFLAGS="$LDFLAGS"
+LDFLAGS="$LDFLAGS -Wl,--wrap=spine_ld_wrap_probe"
+AC_LINK_IFELSE([AC_LANG_PROGRAM([[
+  int spine_ld_wrap_probe(void);
+  int __wrap_spine_ld_wrap_probe(void);
+  int __wrap_spine_ld_wrap_probe(void) { return 0; }
+]], [[
+  return spine_ld_wrap_probe();
+]])],
+  [spine_ld_wrap=yes],
+  [spine_ld_wrap=no])
+LDFLAGS="$spine_save_LDFLAGS"
+AC_MSG_RESULT($spine_ld_wrap)
+AM_CONDITIONAL([HAVE_LD_WRAP], [test "x$spine_ld_wrap" = "xyes"])
+
 # ****************** SNMPv3 USM Error Constants Check ***********************
 AC_MSG_CHECKING([for SNMPv3 USM error constants])
 AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[

error.c

@@ -129,9 +129,6 @@ static void spine_signal_handler(int spine_signal) {
 		case SIGQUIT:
 			message = "FATAL: Spine Encountered a Keyboard Quit Command\n";
 			break;
-		case SIGPIPE:
-			message = "FATAL: Spine Encountered a Broken Pipe\n";
-			break;
 		default:
 			break;
 	}
@@ -157,7 +154,6 @@ static void spine_signal_handler(int spine_signal) {
 
 static int spine_fatal_signals[] = {
 	SIGINT,
-	SIGPIPE,
 	SIGSEGV,
 	SIGBUS,
 	SIGFPE,
@@ -167,6 +163,13 @@ static int spine_fatal_signals[] = {
 	0
 };
 
+/* A caught disposition is reset to SIG_DFL by exec(), unlike SIG_IGN. Keep
+ * broken pipes non-fatal in Spine while preserving normal SIGPIPE semantics
+ * for operator scripts launched through either posix_spawn() or libc popen(). */
+static void spine_sigpipe_handler(int spine_signal) {
+	(void) spine_signal;
+}
+
 /*! \fn void install_spine_signal_handler(void)
  *  \brief installs the spine signal handler to stop certain calls from
  *         abending Spine.
@@ -178,6 +181,17 @@ void install_spine_signal_handler(void) {
 	struct sigaction sa;
 	void (*ohandler)(int);
 
+	/* Broken pipes are ordinary runtime failures for database sockets, script
+	 * pipes and redirected logs. A caught handler makes write() return EPIPE,
+	 * and exec'd children automatically regain SIG_DFL. */
+	memset(&sa, 0, sizeof(sa));
+	sa.sa_handler = spine_sigpipe_handler;
+	sigemptyset(&sa.sa_mask);
+	sa.sa_flags = SA_RESTART;
+	if (sigaction(SIGPIPE, &sa, NULL) != 0) {
+		SPINE_LOG(("ERROR: Unable to install SIGPIPE handler: %s", strerror(errno)));
+	}
+
 	for (i=0; spine_fatal_signals[i]; ++i) {
 		sigaction(spine_fatal_signals[i], NULL, &sa);
 		if (sa.sa_handler == SIG_DFL) {
@@ -208,6 +222,17 @@ void uninstall_spine_signal_handler(void) {
 	struct sigaction sa;
 	void (*ohandler)(int);
 
+	if (sigaction(SIGPIPE, NULL, &sa) != 0) {
+		SPINE_LOG(("WARNING: Unable to inspect SIGPIPE handler during shutdown: %s", strerror(errno)));
+	} else if (sa.sa_handler == spine_sigpipe_handler) {
+		sa.sa_handler = SIG_DFL;
+		sigemptyset(&sa.sa_mask);
+		sa.sa_flags = 0;
+		if (sigaction(SIGPIPE, &sa, NULL) != 0) {
+			SPINE_LOG(("WARNING: Unable to restore the default SIGPIPE handler: %s", strerror(errno)));
+		}
+	}
+
 	for (i=0; spine_fatal_signals[i]; ++i) {
 		sigaction(spine_fatal_signals[i], NULL, &sa);
 		if (sa.sa_handler == spine_signal_handler) {

nft_popen.c

@@ -87,6 +87,8 @@
 #include "common.h"
 #include "spine.h"
 #include <spawn.h>
+#include <fcntl.h>
+#include <sys/wait.h>
 
 /* An instance of this struct is created for each popen() fd. */
 static struct pid
@@ -99,7 +101,230 @@ static struct pid
 /* Serialize access to PidList. */
 static pthread_mutex_t ListMutex = PTHREAD_MUTEX_INITIALIZER;
 
+/* Children nft_pclose() gave up waiting for. Nothing else in spine reaps: there
+   is no SIGCHLD handler and no waitpid(-1), so a child dropped here would stay
+   a zombie for the daemon's lifetime and accumulate once per affected script
+   per cycle against RLIMIT_NPROC. SA_NOCLDWAIT would fix the leak but auto-reap
+   every child, and spine reads exit status to tell a failed script from a silent
+   one, so the pids are parked here and swept with WNOHANG instead. Bounded: past
+   the cap the pid is logged and dropped, because an unbounded list trades a pid
+   leak for a memory leak. */
+static pid_t	AbandonedPids[NFT_ABANDONED_MAX];
+static int	AbandonedCount;
+
 static void	close_cleanup(void *);
+static void	nft_sweep_abandoned(void);
+
+/* Close and remove an entry from the shared registry, then transfer exclusive
+ * ownership to the caller.  Closing under ListMutex preserves the invariant
+ * that every descriptor still visible to nft_popen() is open, so its
+ * posix_spawn addclose walk cannot queue an already-closed descriptor.  Once
+ * returned, no other thread can find or free the entry.
+ * Keep this noinline: GCC 12.2 emits -Wclobbered for the helper local when it
+ * is inlined into nft_pclose()'s pthread cleanup macro scope.
+ */
+static __attribute__((noinline)) struct pid *pid_list_close_and_take(int fd)
+{
+	struct pid **link;
+	struct pid *cur = NULL;
+
+	pthread_mutex_lock(&ListMutex);
+
+	for (link = &PidList; *link != NULL; link = &(*link)->next) {
+		if ((*link)->fd == fd) {
+			cur = *link;
+			(void)close(cur->fd);
+			cur->fd = -1;
+			*link = cur->next;
+			cur->next = NULL;
+			break;
+		}
+	}
+
+	pthread_mutex_unlock(&ListMutex);
+
+	return cur;
+}
+
+/* nft_pclose() must not block a poller thread indefinitely. A script that
+   writes its value and then lingers, or that ignores SIGPIPE, would otherwise
+   pin the thread across polling cycles while holding its available_scripts
+   token. Poll with WNOHANG, then escalate to SIGKILL. */
+/* Budget to SIGKILL is about five seconds on both platforms. The counts differ
+ * because the granularity does: everywhere else in the tree usleep() is simply
+ * skipped under SOLAR_THREAD rather than replaced, so the coarsest wait
+ * available there is a whole second and the attempt count scales to match.
+ * Leaving the counts equal made the Solaris path roughly a hundred seconds,
+ * longer than a polling cycle, while nft_pclose() holds an available_scripts
+ * token throughout. */
+#define NFT_PCLOSE_REAP_USEC 50000
+#define NFT_PCLOSE_SPIN_USEC 200
+#define NFT_PCLOSE_SPIN_ATTEMPTS 10
+#ifndef SOLAR_THREAD
+#define NFT_PCLOSE_TERM_ATTEMPTS 100
+#define NFT_PCLOSE_KILL_ATTEMPTS 20
+#else
+#define NFT_PCLOSE_TERM_ATTEMPTS 5
+#define NFT_PCLOSE_KILL_ATTEMPTS 2
+#endif
+
+int spine_set_cloexec(int fd) {
+	int flags;
+
+	flags = fcntl(fd, F_GETFD);
+	if (flags < 0) {
+		SPINE_LOG(("ERROR: Unable to read descriptor flags on fd %d: %s", fd, strerror(errno)));
+		return -1;
+	}
+
+	if (fcntl(fd, F_SETFD, flags | FD_CLOEXEC) != 0) {
+		SPINE_LOG(("ERROR: Unable to set close-on-exec on fd %d: %s", fd, strerror(errno)));
+		return -1;
+	}
+
+	return 0;
+}
+
+int spine_dup_cloexec(int fd) {
+	int duplicate;
+	int saved_errno;
+
+	duplicate = dup(fd);
+	if (duplicate < 0)
+		return -1;
+
+	if (spine_set_cloexec(duplicate) != 0) {
+		saved_errno = errno;
+		(void)close(duplicate);
+		errno = saved_errno;
+		return -1;
+	}
+
+	return duplicate;
+}
+
+/*! \fn static int open_pipe_cloexec(int pdes[2])
+ *  \brief open a pipe whose descriptors are not inherited across exec
+ *
+ *  nft_popen() creates the pipe before taking ListMutex, so a second thread
+ *  can spawn while these descriptors are live. Without close-on-exec that
+ *  child holds the first thread's write end, the first thread never sees EOF,
+ *  and it blocks to script_timeout for a data source that answered.
+ *
+ *  pipe2(pdes, O_CLOEXEC) would set the flag atomically, but it needs
+ *  _GNU_SOURCE on glibc and spine defines no feature macro, so the fcntl()
+ *  pair stays. It leaves a window between the two calls, which is narrower
+ *  than none.
+ *
+ *  \return TRUE on success, FALSE with the descriptors closed on failure
+ */
+int spine_open_pipe_cloexec(int pdes[2]) {
+	if (pipe(pdes) < 0) {
+		SPINE_LOG(("ERROR: Unable to create a pipe: %s", strerror(errno)));
+		return FALSE;
+	}
+
+	/* spine_set_cloexec() has already said which descriptor failed and why;
+	 * a descriptor that stays inheritable is worse than no pipe at all, so
+	 * this fails rather than continuing without the flag. */
+	if (spine_set_cloexec(pdes[0]) != 0 || spine_set_cloexec(pdes[1]) != 0) {
+		(void)close(pdes[0]);
+		(void)close(pdes[1]);
+
+		/* the caller owns nothing on failure, so do not leave it holding two
+		   descriptor numbers that now belong to whoever opens next; a caller
+		   with one cleanup path would close them a second time */
+		pdes[0] = -1;
+		pdes[1] = -1;
+
+		return FALSE;
+	}
+
+	return TRUE;
+}
+
+int spine_spawnattr_sigpipe_default(posix_spawnattr_t *attr) {
+	sigset_t defaults;
+	int rc;
+
+	if (attr == NULL) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	rc = posix_spawnattr_init(attr);
+	if (rc != 0) {
+		errno = rc;
+		return -1;
+	}
+
+	sigemptyset(&defaults);
+	sigaddset(&defaults, SIGPIPE);
+	rc = posix_spawnattr_setsigdefault(attr, &defaults);
+	if (rc == 0) rc = posix_spawnattr_setflags(attr, POSIX_SPAWN_SETSIGDEF);
+	if (rc != 0) {
+		posix_spawnattr_destroy(attr);
+		errno = rc;
+		return -1;
+	}
+
+	return 0;
+}
+
+/*! \fn static int reap_child_bounded(pid_t pid, int *pstat, int attempts)
+ *  \return 0 when reaped, 1 when still running after attempts, -1 on error
+ */
+int spine_reap_child_bounded(pid_t pid, int *pstat, int attempts) {
+	int attempt;
+	pid_t waited;
+
+	if (pstat == NULL) {
+		return -1;
+	}
+
+	for (attempt = 0; attempt < attempts; attempt++) {
+		do {
+			waited = waitpid(pid, pstat, WNOHANG);
+		} while (waited < 0 && errno == EINTR);
+
+		if (waited == pid) {
+			return 0;
+		}
+
+		if (waited < 0 && errno == ECHILD) {
+			/* someone else reaped it, so no status is available */
+			*pstat = 0;
+			return 0;
+		}
+
+		if (waited < 0) {
+			/* leave errno as waitpid set it; nft_pclose() reports it */
+			return -1;
+		}
+
+		/* The delay is load-bearing: without it the attempts are spent in
+		   nanoseconds and SIGKILL lands before the child can exit.
+
+		   Starting at the full 50ms charged that to every script that exits a
+		   moment after closing stdout, which is the common case for anything
+		   that flushes or tears down an interpreter. nft_pclose() runs while
+		   the caller still holds an available_scripts token, so that delay
+		   costs poller capacity rather than one thread. Spin briefly first,
+		   then settle. The attempt count and so the time to SIGKILL are
+		   unchanged. */
+		#ifndef SOLAR_THREAD
+		if (attempt < NFT_PCLOSE_SPIN_ATTEMPTS) {
+			usleep(NFT_PCLOSE_SPIN_USEC);
+		} else {
+			usleep(NFT_PCLOSE_REAP_USEC);
+		}
+		#else
+		sleep(1);
+		#endif
+	}
+
+	return 1;
+}
 
 /*! ------------------------------------------------------------------------------
  *
@@ -130,6 +355,7 @@ int nft_popen(const char * command, const char * type) {
 	struct pid *cur;
 	struct pid *p;
 	int    pdes[2];
+	int     inherit_fd = -1;
 	int    fd, twoway;
 	pid_t  pid;
 	char   *argv[4];
@@ -154,7 +380,7 @@ int nft_popen(const char * command, const char * type) {
 		}
 	}
 
-	if (pipe(pdes) < 0)
+	if (!spine_open_pipe_cloexec(pdes))
 		return -1;
 
 	/* Disable thread cancellation from this point forward. */
@@ -185,6 +411,11 @@ int nft_popen(const char * command, const char * type) {
 	 */
 	pthread_mutex_lock(&ListMutex);
 
+	/* Drain anything a previous nft_pclose() gave up on. Doing it here means the
+	   list empties on the next script poll rather than waiting for another
+	   failure to trigger a sweep. */
+	nft_sweep_abandoned();
+
 	/* Build file actions for posix_spawn to replace vfork+execve. */
 	posix_spawn_file_actions_t fa;
 	if (posix_spawn_file_actions_init(&fa) != 0) {
@@ -198,6 +429,19 @@ int nft_popen(const char * command, const char * type) {
 		return -1;
 	}
 
+	/* The pipe ends are close-on-exec, which is the point: another thread
+	 * spawning in this window must not inherit them. The child needs its own
+	 * end, and dup2 clears the flag on its target, so the usual paths are
+	 * fine.
+	 *
+	 * When the end already sits on the descriptor it is destined for, there is
+	 * no dup2 to clear anything and the child would exec with that descriptor
+	 * closed. That happens whenever stdin or stdout was closed before this
+	 * call, which for a daemon is not exotic, and the failure is silent: every
+	 * script data source records U. Duplicate the end to a fresh descriptor,
+	 * explicitly mark that duplicate close-on-exec so concurrent children
+	 * cannot inherit it, and let this child dup2 from it. dup2 clears the flag
+	 * on its target. */
 	if (*type == 'r') {
 		posix_spawn_file_actions_addclose(&fa, pdes[0]);
 		if (pdes[1] != STDOUT_FILENO) {
@@ -205,20 +449,50 @@ int nft_popen(const char * command, const char * type) {
 			posix_spawn_file_actions_addclose(&fa, pdes[1]);
 			if (twoway)
 				posix_spawn_file_actions_adddup2(&fa, STDOUT_FILENO, STDIN_FILENO);
-		} else if (twoway && (pdes[1] != STDIN_FILENO)) {
-			posix_spawn_file_actions_adddup2(&fa, pdes[1], STDIN_FILENO);
+		} else {
+			inherit_fd = spine_dup_cloexec(pdes[1]);
+
+			if (inherit_fd < 0) {
+				SPINE_LOG(("ERROR: Unable to duplicate the pipe for the child: %s", strerror(errno)));
+				goto spawn_failed;
+			}
+
+			posix_spawn_file_actions_adddup2(&fa, inherit_fd, STDOUT_FILENO);
+			posix_spawn_file_actions_addclose(&fa, inherit_fd);
+
+			if (twoway)
+				posix_spawn_file_actions_adddup2(&fa, STDOUT_FILENO, STDIN_FILENO);
 		}
 	} else {
 		if (pdes[0] != STDIN_FILENO) {
 			posix_spawn_file_actions_adddup2(&fa, pdes[0], STDIN_FILENO);
 			posix_spawn_file_actions_addclose(&fa, pdes[0]);
+		} else {
+			inherit_fd = spine_dup_cloexec(pdes[0]);
+
+			if (inherit_fd < 0) {
+				SPINE_LOG(("ERROR: Unable to duplicate the pipe for the child: %s", strerror(errno)));
+				goto spawn_failed;
+			}
+
+			posix_spawn_file_actions_adddup2(&fa, inherit_fd, STDIN_FILENO);
+			posix_spawn_file_actions_addclose(&fa, inherit_fd);
 		}
 		posix_spawn_file_actions_addclose(&fa, pdes[1]);
 	}
 
 	/* Close all other pipes in the child (Posix.2 requirement). */
-	for (p = PidList; p; p = p->next)
+	for (p = PidList; p; p = p->next) {
+		/* File actions run in order. Do not close a standard descriptor after
+		 * this spawn has redirected a fresh pipe onto it merely because an
+		 * older parent-side pipe happens to use the same descriptor number. */
+		if ((*type == 'r' && p->fd == STDOUT_FILENO) ||
+		    (*type == 'r' && twoway && p->fd == STDIN_FILENO) ||
+		    (*type == 'w' && p->fd == STDIN_FILENO)) {
+			continue;
+		}
 		posix_spawn_file_actions_addclose(&fa, p->fd);
+	}
 
 	/* Spawn the child process with retry on EAGAIN/ENOMEM. */
 	#if defined(__CYGWIN__)
@@ -239,7 +513,20 @@ int nft_popen(const char * command, const char * type) {
 		}
 
 		SPINE_LOG(("ERROR: SCRIPT: posix_spawn failed: %s", strerror(spawn_err)));
+
+spawn_failed:
+		/* One teardown for every failure after the file actions exist and the
+		 * list mutex is held. ListMutex is process-global, so a path that
+		 * returns still holding it wedges every later nft_popen() and
+		 * nft_pclose() in every poller thread and the daemon stops collecting
+		 * script data until it is restarted. */
 		posix_spawn_file_actions_destroy(&fa);
+
+		if (inherit_fd != -1) {
+			(void)close(inherit_fd);
+			inherit_fd = -1;
+		}
+
 		(void)close(pdes[0]);
 		(void)close(pdes[1]);
 		pthread_mutex_unlock(&ListMutex);
@@ -251,6 +538,15 @@ int nft_popen(const char * command, const char * type) {
 
 	posix_spawn_file_actions_destroy(&fa);
 
+	/* The child holds its own duplicate. Keeping this one would hold the pipe's
+	 * write end open, so the reader never sees EOF and exec_poll() blocks to
+	 * script_timeout on a script that already answered. That is the failure the
+	 * close-on-exec work exists to prevent. */
+	if (inherit_fd != -1) {
+		(void)close(inherit_fd);
+		inherit_fd = -1;
+	}
+
 	/* Parent. */
 	if (*type == 'r') {
 		fd = pdes[0];
@@ -330,40 +626,151 @@ nft_pclose(int fd)
 {
 	struct pid *cur;
 	int		pstat;
+	int		cancel_state;
 	pid_t	pid;
 
-	/* Find the appropriate file descriptor. */
-	pthread_mutex_lock(&ListMutex);
-
-	for (cur = PidList; cur; cur = cur->next)
-	if (cur->fd == fd) break;
-
-	pthread_mutex_unlock(&ListMutex);
+	/* Cancellation must remain disabled until the detached entry is protected
+	 * by the cleanup handler.  Detaching transfers exclusive ownership here.
+	 */
+	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cancel_state);
+	cur = pid_list_close_and_take(fd);
 
 	if (cur == NULL) {
+		pthread_setcancelstate(cancel_state, NULL);
 		errno = EBADF;
 		return -1;
 	}
 
-	/* The close and waitpid calls below are cancellation points.
-	 * We want to ensure that the fd is closed and the PidList
-	 * entry freed despite cancellation, so push a cleanup handler.
-	 */
+	/* Install the cleanup handler before restoring cancellation. */
 	pthread_cleanup_push(close_cleanup, cur);
 
-	/* end the process nicely and then forcefully */
-	(void)close(fd);
+	pthread_setcancelstate(cancel_state, NULL);
 
-	cur->fd = -1;		/* Prevent the fd being closed twice. */
-
-	do { pid = waitpid(cur->pid, &pstat, 0);
-	} while (pid == -1 && errno == EINTR);
+	/* Give a child a brief chance to observe pipe EOF, then request graceful
+	 * termination before escalating to SIGKILL. */
+	switch (spine_reap_child_bounded(cur->pid, &pstat, NFT_PCLOSE_SPIN_ATTEMPTS)) {
+	case 0:
+		pid = cur->pid;
+		break;
+	case 1:
+		(void)kill(cur->pid, SIGTERM);
+		if (spine_reap_child_bounded(cur->pid, &pstat, NFT_PCLOSE_TERM_ATTEMPTS) == 0) {
+			pid = cur->pid;
+		} else {
+			(void)kill(cur->pid, SIGKILL);
+			if (spine_reap_child_bounded(cur->pid, &pstat, NFT_PCLOSE_KILL_ATTEMPTS) == 0) {
+				pid = cur->pid;
+			} else {
+				nft_abandon_child(cur->pid, "kill budget expired");
+				errno = ETIMEDOUT;
+				pid = -1;
+			}
+		}
+		break;
+	default:
+		nft_abandon_child(cur->pid, "waitpid failed");
+		pid = -1;
+		break;
+	}
 
 	pthread_cleanup_pop(1);	/* Execute the cleanup handler. */
 
 	return (pid == -1 ? -1 : pstat);
 }
 
+/*! ------------------------------------------------------------------------------
+  * nft_sweep_abandoned	- reap any child a previous nft_pclose() gave up on.
+  *
+  * Called with ListMutex held. WNOHANG only: this runs on a poller thread and
+  * must never block on a child that is still stuck.
+  *------------------------------------------------------------------------------
+ */
+static void
+nft_sweep_abandoned(void)
+{
+	int	i = 0;
+	int	status;
+	pid_t	waited;
+
+	while (i < AbandonedCount) {
+		do {
+			waited = waitpid(AbandonedPids[i], &status, WNOHANG);
+		} while (waited < 0 && errno == EINTR);
+
+		if (waited == AbandonedPids[i] || (waited < 0 && errno == ECHILD)) {
+			SPINE_LOG_DEBUG(("DEBUG: Reaped abandoned script child pid %ld", (long) AbandonedPids[i]));
+			AbandonedPids[i] = AbandonedPids[AbandonedCount - 1];
+			AbandonedCount--;
+		} else {
+			i++;
+		}
+	}
+}
+
+/*! ------------------------------------------------------------------------------
+  * nft_abandoned_pending	- sweep, then report how many pids are still parked.
+  *
+  * Takes ListMutex itself, so a caller that already holds it uses
+  * nft_sweep_abandoned() directly.
+  *------------------------------------------------------------------------------
+ */
+int
+nft_abandoned_pending(void)
+{
+	int	remaining;
+	int	oldstate;
+
+	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldstate);
+	pthread_mutex_lock(&ListMutex);
+
+	nft_sweep_abandoned();
+	remaining = AbandonedCount;
+
+	pthread_mutex_unlock(&ListMutex);
+	pthread_setcancelstate(oldstate, NULL);
+
+	return remaining;
+}
+
+/*! ------------------------------------------------------------------------------
+  * nft_abandon_child	- record a child that outlived its kill budget.
+  *
+  * The pid and the reason are logged either way. A silent drop leaves PID
+  * exhaustion with nothing in the log pointing at its cause.
+  *------------------------------------------------------------------------------
+ */
+void
+nft_abandon_child(pid_t pid, const char *reason)
+{
+	int	parked;
+	int	oldstate;
+
+	/* nft_pclose() calls this inside its pthread_cleanup_push() region, and
+	   close_cleanup() takes ListMutex. A cancel delivered while this held the
+	   lock would run the handler straight into it, so hold it uncancellable. */
+	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldstate);
+
+	pthread_mutex_lock(&ListMutex);
+
+	nft_sweep_abandoned();
+
+	parked = (AbandonedCount < NFT_ABANDONED_MAX);
+
+	if (parked) {
+		AbandonedPids[AbandonedCount++] = pid;
+	}
+
+	pthread_mutex_unlock(&ListMutex);
+
+	pthread_setcancelstate(oldstate, NULL);
+
+	if (parked) {
+		SPINE_LOG(("WARNING: SCRIPT: pid %ld survived SIGKILL (%s); parked for reaping", (long) pid, reason));
+	} else {
+		SPINE_LOG(("ERROR: SCRIPT: pid %ld survived SIGKILL (%s) and the abandoned list is full; it will remain a zombie", (long) pid, reason));
+	}
+}
+
 /*! ------------------------------------------------------------------------------
   * close_cleanup	- close the pipe and free the pidlist entry.
   *------------------------------------------------------------------------------
@@ -372,29 +779,6 @@ static void
 close_cleanup(void * arg)
 {
 	struct pid * cur = arg;
-	struct pid * prev;
 
-	/* Close the pipe fd if necessary. */
-	if (cur->fd >= 0) {
-		(void)close(cur->fd);
-	}
-
-	/* Remove the entry from the linked list. */
-	pthread_mutex_lock(&ListMutex);
-
-	if (PidList == cur) {
-		PidList =  cur->next;
-	}else{
-		for (prev = PidList; prev; prev = prev->next)
-		if (prev->next == cur) {
-			prev->next =  cur->next;
-			break;
-		}
-
-		assert(prev != NULL);	/* Search should not fail */
-	}
-
-	pthread_mutex_unlock(&ListMutex);
-
-	free(cur);
+	SPINE_FREE(cur);
 }

nft_popen.h

@@ -49,6 +49,8 @@
  ******************************************************************************
  */
 
+#include <spawn.h>
+
 /*!
  *  The nft_popen() function forks a command in a child process, and returns
  *  a pipe that is connected to the child's standard input and output. It is
@@ -94,4 +96,68 @@ extern int	nft_pchild(int fd);
  */
 extern int	nft_pclose(int fd);
 
+/*!
+ *  spine_set_cloexec
+ *
+ *  Mark a descriptor close-on-exec.
+ *
+ *  Returns 0 on success, -1 on failure with errno set by fcntl().
+ */
+extern int	spine_set_cloexec(int fd);
+
+/* Duplicate a descriptor and mark the duplicate close-on-exec. On failure,
+ * no descriptor is returned and errno describes dup() or fcntl(). */
+extern int	spine_dup_cloexec(int fd);
+
+/*!
+ *  spine_open_pipe_cloexec
+ *
+ *  Open a pipe whose descriptors are not inherited across exec. Spine spawns
+ *  children from several threads, so a descriptor left inheritable is held by
+ *  an unrelated child and the reader never sees EOF.
+ *
+ *  Returns TRUE on success. On failure the descriptors are closed and FALSE is
+ *  returned, so the caller owns nothing.
+ */
+extern int	spine_open_pipe_cloexec(int pdes[2]);
+
+/* Restore SIGPIPE's default disposition in posix_spawned children while the
+ * Spine parent handles broken pipes itself. */
+extern int	spine_spawnattr_sigpipe_default(posix_spawnattr_t *attr);
+
+/*!
+ *  spine_reap_child_bounded
+ *
+ *  Reap a child with WNOHANG, sleeping between attempts, so a wedged script
+ *  cannot pin a poller thread indefinitely.
+ *
+ *  Returns 0 when the child was reaped, 1 when it is still running after
+ *  attempts, and -1 on a waitpid() error other than EINTR or ECHILD.
+ */
+extern int	spine_reap_child_bounded(pid_t pid, int *pstat, int attempts);
+
+/*!
+ *  The cap on parked pids. Past it a child is logged and dropped, because an
+ *  unbounded list would trade a pid leak for a memory leak.
+ */
+#define NFT_ABANDONED_MAX 64
+
+/*!
+ *  nft_abandon_child
+ *
+ *  Record a child that outlived nft_pclose()'s kill budget. Nothing else in
+ *  spine reaps, so a dropped child would stay a zombie for the daemon's
+ *  lifetime; parked pids are swept on the next script poll. The pid and the
+ *  reason are logged either way.
+ */
+extern void	nft_abandon_child(pid_t pid, const char *reason);
+
+/*!
+ *  nft_abandoned_pending
+ *
+ *  Sweep the parked pids with WNOHANG and return how many are still running.
+ *  Zero means nothing is leaking.
+ */
+extern int	nft_abandoned_pending(void);
+
 #endif /* SPINE_NFT_POPEN_H */

php.c

@@ -38,6 +38,118 @@
 
 extern char **environ;
 
+/*! \fn static int php_addclose_unless_std(posix_spawn_file_actions_t *fa, int fd)
+ *  \brief queue a close for a pipe end unless it is stdin or stdout
+ *
+ *  After the dup2 redirects, descriptors 0 and 1 hold the child's ends. Closing
+ *  them here would undo the redirect that was just set up.
+ */
+/*! \fn static void php_close_fd(int *fd)
+ *  \brief close a descriptor once and mark it gone
+ *
+ *  php_init() has one cleanup path for six descriptors, some of which are
+ *  handed to php_processes[] on the way out. Clearing as it closes is what
+ *  keeps the shared teardown from closing a descriptor the parent still owns,
+ *  or one that another thread has since been given.
+ */
+static void php_close_fd(int *fd) {
+	if (*fd >= 0) {
+		(void) close(*fd);
+		*fd = -1;
+	}
+}
+
+static int php_addclose_unless_std(posix_spawn_file_actions_t *fa, int fd) {
+	if (fd == STDIN_FILENO || fd == STDOUT_FILENO) {
+		return 0;
+	}
+
+	return posix_spawn_file_actions_addclose(fa, fd);
+}
+
+static void php_process_lock(int php_process) {
+	thread_mutex_lock(LOCK_PHP_PROC_0 + php_process);
+}
+
+static void php_process_unlock(int php_process) {
+	thread_mutex_unlock(LOCK_PHP_PROC_0 + php_process);
+}
+
+static char *php_undefined_result(void) {
+	char *result = strdup("U");
+
+	if (result == NULL)
+		die("ERROR: Fatal malloc error: php.c php_cmd!");
+
+	return result;
+}
+
+static void php_fail_read(int php_process, int allow_restart) {
+	php_processes[php_process].php_state = PHP_BUSY;
+	if (allow_restart) {
+		php_close(php_process);
+		php_init(php_process);
+	}
+}
+
+void php_processes_initialize(php_t *processes, int count) {
+	int i;
+
+	if (processes == NULL || count <= 0)
+		return;
+
+	for (i = 0; i < count; i++) {
+		processes[i].php_state = PHP_BUSY;
+		processes[i].php_pid = -1;
+		processes[i].php_read_fd = -1;
+		processes[i].php_write_fd = -1;
+	}
+}
+
+static char *php_read_result(int php_process, char *command, int allow_restart);
+
+/* Block SIGPIPE in the calling thread around Spine's two pipe writes. The
+ * daemon normally catches SIGPIPE with a no-op handler process-wide, but this local guard
+ * makes these writes safe even before signal initialization or in unit tests
+ * that temporarily restore SIG_DFL. Only the default disposition needs its
+ * generated signal drained; a caught signal is safe when the old mask returns. */
+static ssize_t php_write_no_sigpipe(int fd, const void *buffer, size_t length) {
+	sigset_t blocked;
+	sigset_t old_mask;
+	sigset_t pending;
+	struct sigaction sigpipe_action;
+	ssize_t result;
+	int saved_errno;
+	int mask_error;
+	int received_signal;
+	int should_drain = FALSE;
+	int was_pending = FALSE;
+
+	sigemptyset(&blocked);
+	sigaddset(&blocked, SIGPIPE);
+	mask_error = pthread_sigmask(SIG_BLOCK, &blocked, &old_mask);
+	if (mask_error != 0) {
+		errno = mask_error;
+		return -1;
+	}
+	if (sigpending(&pending) == 0) was_pending = sigismember(&pending, SIGPIPE);
+	if (sigaction(SIGPIPE, NULL, &sigpipe_action) == 0 &&
+	    sigpipe_action.sa_handler == SIG_DFL) {
+		should_drain = TRUE;
+	}
+
+	result = write(fd, buffer, length);
+	saved_errno = errno;
+	if (result < 0 && saved_errno == EPIPE && !was_pending && should_drain) {
+		do {
+			mask_error = sigwait(&blocked, &received_signal);
+		} while (mask_error == EINTR);
+	}
+	pthread_sigmask(SIG_SETMASK, &old_mask, NULL);
+	errno = saved_errno;
+	return result;
+}
+
 /*! \fn char *php_cmd(const char *php_command, int php_process)
  *  \brief calls the script server and executes a script command
  *  \param php_command the formatted php script server command
@@ -58,45 +170,49 @@ char *php_cmd(const char *php_command, int php_process) {
 	int retries = 0;
 
 	assert(php_command != 0);
+	if (php_processes == NULL || php_process < 0 || php_process >= set.php_servers ||
+	    php_process >= MAX_PHP_SERVERS) {
+		SPINE_LOG(("ERROR: SS[%i] PHP Script Server slot is unavailable", php_process));
+		return php_undefined_result();
+	}
 
 	/* pad command with CR-LF */
 	snprintf(command, BUFSIZE, "%s\r\n", php_command);
 
-	/* place lock around mutex */
-	switch (php_process) {
-	case 0:  thread_mutex_lock(LOCK_PHP_PROC_0);  break;
-	case 1:  thread_mutex_lock(LOCK_PHP_PROC_1);  break;
-	case 2:  thread_mutex_lock(LOCK_PHP_PROC_2);  break;
-	case 3:  thread_mutex_lock(LOCK_PHP_PROC_3);  break;
-	case 4:  thread_mutex_lock(LOCK_PHP_PROC_4);  break;
-	case 5:  thread_mutex_lock(LOCK_PHP_PROC_5);  break;
-	case 6:  thread_mutex_lock(LOCK_PHP_PROC_6);  break;
-	case 7:  thread_mutex_lock(LOCK_PHP_PROC_7);  break;
-	case 8:  thread_mutex_lock(LOCK_PHP_PROC_8);  break;
-	case 9:  thread_mutex_lock(LOCK_PHP_PROC_9);  break;
-	case 10: thread_mutex_lock(LOCK_PHP_PROC_10); break;
-	case 11: thread_mutex_lock(LOCK_PHP_PROC_11); break;
-	case 12: thread_mutex_lock(LOCK_PHP_PROC_12); break;
-	case 13: thread_mutex_lock(LOCK_PHP_PROC_13); break;
-	case 14: thread_mutex_lock(LOCK_PHP_PROC_14); break;
+	php_process_lock(php_process);
+
+	retry:
+	/* Validate every attempt under the same per-slot lock that protects
+	 * close/restart. A
+	 * check before the lock races a recovery and can use a descriptor after it
+	 * has been closed and reused by another thread. A failed restart may also
+	 * leave live descriptors in a BUSY slot, which must not receive a command. */
+	if (php_processes[php_process].php_state != PHP_READY ||
+	    php_processes[php_process].php_pid <= 1 ||
+	    php_processes[php_process].php_read_fd < 0 ||
+	    php_processes[php_process].php_write_fd < 0) {
+		php_process_unlock(php_process);
+		SPINE_LOG(("ERROR: SS[%i] PHP Script Server slot is unavailable", php_process));
+		return php_undefined_result();
 	}
 
 	/* send command to the script server */
-	retry:
-	bytes = write(php_processes[php_process].php_write_fd, command, strlen(command));
+	bytes = php_write_no_sigpipe(php_processes[php_process].php_write_fd, command, strlen(command));
 
 	/* if write status is <= 0 then the script server may be hung */
 	if (bytes <= 0) {
-		result_string = strdup("U");
 		SPINE_LOG(("ERROR: SS[%i] PHP Script Server communications lost sending Command[%s].  Restarting PHP Script Server", php_process, command));
 
 		php_close(php_process);
-		php_init(php_process);
-		/* increment and retry a few times on the next item */
 		retries++;
-		if (retries < 3) {
+		if (retries < 3 && php_init(php_process) == TRUE &&
+		    php_processes[php_process].php_state == PHP_READY) {
 			goto retry;
 		}
+
+		/* allocated only once the retry budget is spent: a successful retry
+		   reassigns result_string below and would orphan an earlier copy */
+		result_string = php_undefined_result();
 	} else {
 		/* read the result from the php_command */
 		result_string = php_readpipe(php_process, command);
@@ -107,24 +223,7 @@ char *php_cmd(const char *php_command, int php_process) {
 		}
 	}
 
-	/* unlock around php process */
-	switch (php_process) {
-	case 0:  thread_mutex_unlock(LOCK_PHP_PROC_0);  break;
-	case 1:  thread_mutex_unlock(LOCK_PHP_PROC_1);  break;
-	case 2:  thread_mutex_unlock(LOCK_PHP_PROC_2);  break;
-	case 3:  thread_mutex_unlock(LOCK_PHP_PROC_3);  break;
-	case 4:  thread_mutex_unlock(LOCK_PHP_PROC_4);  break;
-	case 5:  thread_mutex_unlock(LOCK_PHP_PROC_5);  break;
-	case 6:  thread_mutex_unlock(LOCK_PHP_PROC_6);  break;
-	case 7:  thread_mutex_unlock(LOCK_PHP_PROC_7);  break;
-	case 8:  thread_mutex_unlock(LOCK_PHP_PROC_8);  break;
-	case 9:  thread_mutex_unlock(LOCK_PHP_PROC_9);  break;
-	case 10: thread_mutex_unlock(LOCK_PHP_PROC_10); break;
-	case 11: thread_mutex_unlock(LOCK_PHP_PROC_11); break;
-	case 12: thread_mutex_unlock(LOCK_PHP_PROC_12); break;
-	case 13: thread_mutex_unlock(LOCK_PHP_PROC_13); break;
-	case 14: thread_mutex_unlock(LOCK_PHP_PROC_14); break;
-	}
+	php_process_unlock(php_process);
 
 	return result_string;
 }
@@ -135,23 +234,95 @@ char *php_cmd(const char *php_command, int php_process) {
  *  This very simple function simply returns the next PHP Script Server
  *  process id to poll using a round robin algorithm.
  *
- *  \return the integer number of the next script server to use
+ *  \return the next usable script server, or -1 if none is available
  *
  */
 int php_get_process(void) {
-	int i;
+	int candidate;
+	int checked;
+	int contended_candidate = -1;
+	int recovery_candidate = -1;
+	int start_candidate;
+	int server_count;
 
+	if (php_processes == NULL || set.php_servers <= 0) return -1;
+	server_count = set.php_servers > MAX_PHP_SERVERS ? MAX_PHP_SERVERS : set.php_servers;
+
+	/* LOCK_PHP protects only the round-robin cursor. A startup handshake can
+	 * wait for script_timeout, so it must never run under this process-global
+	 * lock. */
 	thread_mutex_lock(LOCK_PHP);
-	if (set.php_current_server >= set.php_servers) {
-		set.php_current_server = 0;
-	}
-	i = set.php_current_server;
-	set.php_current_server++;
+	if (set.php_current_server >= server_count) set.php_current_server = 0;
+	start_candidate = set.php_current_server++;
 	thread_mutex_unlock(LOCK_PHP);
 
-	return i;
+	/* Prefer any ready slot. Each snapshot uses the same per-slot mutex as
+	 * php_cmd() and recovery, so descriptors cannot change under the check. */
+	for (checked = 0; checked < server_count; checked++) {
+		candidate = (start_candidate + checked) % server_count;
+		if (thread_mutex_trylock(LOCK_PHP_PROC_0 + candidate) != 0) {
+			if (contended_candidate < 0) contended_candidate = candidate;
+			continue;
+		}
+		if (php_processes[candidate].php_state == PHP_READY &&
+		    php_processes[candidate].php_pid > 1 &&
+		    php_processes[candidate].php_read_fd >= 0 &&
+		    php_processes[candidate].php_write_fd >= 0) {
+			php_process_unlock(candidate);
+			return candidate;
+		}
+		if (recovery_candidate < 0) recovery_candidate = candidate;
+		php_process_unlock(candidate);
+	}
+
+	/* A locked slot is normally a healthy server executing another command.
+	 * Return it so php_cmd() queues on the slot mutex instead of turning routine
+	 * contention into an undefined data point, but first repair any failed slot
+	 * observed by this scan so steady contention cannot starve pool recovery.
+	 * Validation still happens after the contended lock is acquired. */
+	if (contended_candidate >= 0 && recovery_candidate < 0)
+		return contended_candidate;
+
+	if (recovery_candidate < 0 ||
+	    thread_mutex_trylock(LOCK_PHP_PROC_0 + recovery_candidate) != 0) {
+		return contended_candidate;
+	}
+
+	/* Recover at most one slot per request. The per-slot lock prevents a
+	 * simultaneous php_cmd() or another recovery from closing the same fd or
+	 * signalling a recycled pid. */
+	if (php_processes[recovery_candidate].php_state == PHP_READY &&
+	    php_processes[recovery_candidate].php_pid > 1 &&
+	    php_processes[recovery_candidate].php_read_fd >= 0 &&
+	    php_processes[recovery_candidate].php_write_fd >= 0) {
+		php_process_unlock(recovery_candidate);
+		return recovery_candidate;
+	}
+	if (php_processes[recovery_candidate].php_pid > 1 ||
+	    php_processes[recovery_candidate].php_read_fd >= 0 ||
+	    php_processes[recovery_candidate].php_write_fd >= 0) {
+		php_close(recovery_candidate);
+	}
+	if (php_init(recovery_candidate) == TRUE &&
+	    php_processes[recovery_candidate].php_state == PHP_READY) {
+		php_process_unlock(recovery_candidate);
+		return recovery_candidate;
+	}
+	php_process_unlock(recovery_candidate);
+
+	return contended_candidate;
 }
 
+#ifdef SPINE_PHP_RUNTIME_TESTING
+ssize_t php_write_no_sigpipe_for_test(int fd, const void *buffer, size_t length) {
+	return php_write_no_sigpipe(fd, buffer, length);
+}
+
+char *php_read_result_for_test(int php_process, char *command, int allow_restart) {
+	return php_read_result(php_process, command, allow_restart);
+}
+#endif
+
 /*! \fn char *php_readpipe(int php_process, char *command)
  *  \brief read a line from a PHP Script Server process
  *  \param php_process the PHP Script Server process to obtain output from
@@ -163,13 +334,24 @@ int php_get_process(void) {
  *
  *  \return a string pointer to the PHP Script Server response
  */
-char *php_readpipe(int php_process, char *command) {
+/*! \fn static char *php_read_result(int php_process, char *command, int allow_restart)
+ *  \brief reads one script server response.
+ *
+ *  allow_restart is FALSE for the startup handshake. php_init() calls this to
+ *  confirm the server it just spawned is answering, and a restart from inside
+ *  that read would call php_init() again, which reads again: a server that
+ *  starts but never answers put a poller thread into unbounded mutual
+ *  recursion, spawning a fresh server at every level. Refusing the restart on
+ *  the handshake bounds the depth at one by construction.
+ */
+static char *php_read_result(int php_process, char *command, int allow_restart) {
 	fd_set fds;
 	struct timeval timeout;
 	double begin_time = 0;
 	double end_time = 0;
 	double remaining_usec = 0;
 	char *result_string;
+	int response_timeout;
 
 	ssize_t i;
 	char *cp;
@@ -184,27 +366,28 @@ char *php_readpipe(int php_process, char *command) {
 	begin_time = get_time_as_double();
 
 	/* establish timeout value for the PHP script server to respond */
-	timeout.tv_sec = set.script_timeout;
+	/* Selection-path recovery runs this handshake while holding the slot lock.
+	 * Bound startup independently so a bad PHP configuration cannot pin every
+	 * selector for the full per-command timeout. */
+	response_timeout = allow_restart || set.script_timeout < 2 ? set.script_timeout : 2;
+	timeout.tv_sec = response_timeout;
 	timeout.tv_usec = 0;
 
 	/* check to see which pipe talked and take action
 	 * should only be the READ pipe */
 	retry:
 
-	/* FD_SET() does not range check; a descriptor at or above FD_SETSIZE
-	 * would write past the end of the on-stack fd_set below. */
+	/* FD_SET on a descriptor at or past FD_SETSIZE writes outside fds, which is
+	   a stack object here. ping_icmp() guards its socket the same way. */
 	if (php_processes[php_process].php_read_fd < 0 || php_processes[php_process].php_read_fd >= FD_SETSIZE) {
-		SPINE_LOG(("ERROR: SS[%i] PHP Script Server read descriptor %i is outside of FD_SETSIZE %i", php_process, php_processes[php_process].php_read_fd, FD_SETSIZE));
+		SPINE_LOG(("ERROR: SS[%i] Script server descriptor %d exceeds FD_SETSIZE %d", php_process, php_processes[php_process].php_read_fd, FD_SETSIZE));
 
 		SET_UNDEFINED(result_string);
-
-		/* Close the misbehaving server only. php_init() validates a new
-		 * server by calling back into this function, and a replacement
-		 * descriptor handed out under the same descriptor-table exhaustion
-		 * can land >= FD_SETSIZE again, recursing without bound. Mark the
-		 * slot busy and leave the restart to the next independent call. */
-		php_close(php_process);
-		php_processes[php_process].php_state = PHP_BUSY;
+		/* A descriptor that select() cannot represent makes this slot unusable.
+		 * Mark it unhealthy even during the startup handshake, where restarting
+		 * recursively is deliberately disabled, so the scheduler cannot keep
+		 * handing out a permanently poisoned READY slot. */
+		php_fail_read(php_process, allow_restart);
 
 		return result_string;
 	}
@@ -229,8 +412,8 @@ char *php_readpipe(int php_process, char *command) {
 				end_time = get_time_as_double();
 
 				/* re-establish new timeout value */
-				timeout.tv_sec  = rint(floor(set.script_timeout-(end_time-begin_time)));
-				remaining_usec  = set.script_timeout - timeout.tv_sec - (end_time - begin_time);
+					timeout.tv_sec = rint(floor(response_timeout - (end_time - begin_time)));
+					remaining_usec = response_timeout - timeout.tv_sec - (end_time - begin_time);
 
 				if (remaining_usec > 0) {
 					timeout.tv_usec = rint(remaining_usec * 1000000);
@@ -257,67 +440,84 @@ char *php_readpipe(int php_process, char *command) {
 		}
 
 		SET_UNDEFINED(result_string);
-
-		/* kill script server because it is misbehaving */
-		php_close(php_process);
-		php_init(php_process);
+		php_fail_read(php_process, allow_restart);
 		break;
 	case 0:
 		/* record end time */
 		end_time = get_time_as_double();
 		SPINE_LOG(("WARNING: SS[%i] The PHP Script Server did not respond in time for Timeout[%0.2f], Command[%s] and will therefore be restarted", php_process, end_time - begin_time, command));
 		SET_UNDEFINED(result_string);
-
-		/* kill script server because it is misbehaving */
-		php_close(php_process);
-		php_init(php_process);
+		php_fail_read(php_process, allow_restart);
 		break;
 	default:
-		if (FD_ISSET(php_processes[php_process].php_read_fd, &fds)) {
-			bptr = result_string;
+		{
+			int read_ok = TRUE;
 
-			while (1) {
-				/* reserve one byte for the trailing '\0' written below */
-				size_t used = (size_t)(bptr - result_string);
+			if (FD_ISSET(php_processes[php_process].php_read_fd, &fds)) {
+				bptr = result_string;
 
-				if (used >= RESULTS_BUFFER - 1) {
-					SPINE_LOG(("ERROR: SS[%i] The Script Server result was longer than the acceptable range", php_process));
-					SET_UNDEFINED(result_string);
-					break;
-				}
+				while (1) {
+					/* reserve one byte for the trailing '\0' written below */
+					size_t used = (size_t)(bptr - result_string);
 
-				size_t space = (size_t)RESULTS_BUFFER - 1 - used;
-				i = read(php_processes[php_process].php_read_fd, bptr, space);
+					if (used >= RESULTS_BUFFER - 1) {
+						SPINE_LOG(("ERROR: SS[%i] The Script Server result was longer than the acceptable range", php_process));
+						SET_UNDEFINED(result_string);
+						read_ok = FALSE;
+						break;
+					}
 
-				if (i <= 0) {
-					SET_UNDEFINED(result_string);
-					break;
-				}
+					size_t space = (size_t)RESULTS_BUFFER - 1 - used;
+					i = read(php_processes[php_process].php_read_fd, bptr, space);
 
-				bptr += i;
-				*bptr = '\0';	/* make what we've got into a string */
+					if (i <= 0) {
+						SET_UNDEFINED(result_string);
+						read_ok = FALSE;
+						break;
+					}
 
-				if ((cp = strstr(result_string,"\n")) != 0) {
-					break;
-				}
+					bptr += i;
+					*bptr = '\0';	/* make what we've got into a string */
 
-				if (bptr >= result_string + RESULTS_BUFFER - 1) {
-					SPINE_LOG(("ERROR: SS[%i] The Script Server result was longer than the acceptable range", php_process));
-					SET_UNDEFINED(result_string);
-					break;
+					if ((cp = strstr(result_string,"\n")) != 0) {
+						break;
+					}
+
+					if (bptr >= result_string + RESULTS_BUFFER - 1) {
+						SPINE_LOG(("ERROR: SS[%i] The Script Server result was longer than the acceptable range", php_process));
+						SET_UNDEFINED(result_string);
+						read_ok = FALSE;
+						break;
+					}
 				}
+			} else {
+				SPINE_LOG(("ERROR: SS[%i] The FD was not set as expected", php_process));
+				SET_UNDEFINED(result_string);
+				read_ok = FALSE;
 			}
-		} else {
-			SPINE_LOG(("ERROR: SS[%i] The FD was not set as expected", php_process));
-			SET_UNDEFINED(result_string);
-		}
 
-		php_processes[php_process].php_state = PHP_READY;
+			if (read_ok) {
+				php_processes[php_process].php_state = PHP_READY;
+			} else {
+				php_fail_read(php_process, allow_restart);
+			}
+		}
+		break;
 	}
 
 	return result_string;
 }
 
+/*! \fn char *php_readpipe(int php_process, char *command)
+ *  \brief reads a script server response, restarting a server that stops
+ *         answering.
+ *
+ *  \return a string pointer to the PHP Script Server response
+ */
+char *php_readpipe(int php_process, char *command) {
+	return php_read_result(php_process, command, TRUE);
+}
+
 /*! \fn int php_init(int php_process)
  *  \brief initialize either a specific PHP Script Server or all of them.
  *  \param php_process the process number to start or PHP_INIT
@@ -330,8 +530,8 @@ char *php_readpipe(int php_process, char *command) {
  *  \return TRUE if the PHP Script Server is know running or FALSE otherwise
  */
 int php_init(int php_process) {
-	int  cacti2php_pdes[2];
-	int  php2cacti_pdes[2];
+	int  cacti2php_pdes[2] = { -1, -1 };
+	int  php2cacti_pdes[2] = { -1, -1 };
 	pid_t  pid;
 	char poller_id[TINY_BUFSIZE];
 	char *argv[7];
@@ -340,13 +540,28 @@ int php_init(int php_process) {
 	char arg_environ_spine[] = "--environ=spine";
 	char arg_mode_online[] = "--mode=online";
 	char arg_mode_offline[] = "--mode=offline";
-	int  cancel_state;
-	char *result_string = 0;
+	posix_spawn_file_actions_t fa;
+	posix_spawnattr_t attr;
+	int  fa_valid    = FALSE;
+	int  attr_valid  = FALSE;
+	int  cancel_state = 0;
+	int  cancel_held = FALSE;
+	int  child_stdin;
+	int  child_stdout;
+	int  dup_stdin  = -1;
+	int  dup_stdout = -1;
+	char *result_string = NULL;
 	int num_processes;
+	int slot = -1;
 	int i;
-	int retry_count = 0;
+	int rc = FALSE;
 	char *command = strdup("INIT");
 
+	if (command == NULL) {
+		SPINE_LOG(("ERROR: Fatal malloc error: php.c php_init!"));
+		return FALSE;
+	}
+
 	/* special code to start all PHP Servers */
 	if (php_process == PHP_INIT) {
 		num_processes = set.php_servers;
@@ -355,22 +570,31 @@ int php_init(int php_process) {
 	}
 
 	for (i=0; i < num_processes; i++) {
+		/* the spawn retry budget is per server. Sharing one counter across the
+		   loop meant that once the first server spent it on EAGAIN, every
+		   server after it got none, under exactly the resource pressure the
+		   retry exists to ride out. */
+		int retry_count = 0;
+
+		slot = (php_process == PHP_INIT) ? i : php_process;
+
 		SPINE_LOG_DEBUG(("DEBUG: SS[%i] PHP Script Server Routine Starting", i));
 
 		/* create the output pipes from Spine to php*/
-		if (pipe(cacti2php_pdes) < 0) {
+		if (!spine_open_pipe_cloexec(cacti2php_pdes)) {
 			SPINE_LOG(("ERROR: SS[%i] Could not allocate php server pipes", i));
-			return FALSE;
+			goto cleanup;
 		}
 
 		/* create the input pipes from php to Spine */
-		if (pipe(php2cacti_pdes) < 0) {
+		if (!spine_open_pipe_cloexec(php2cacti_pdes)) {
 			SPINE_LOG(("ERROR: SS[%i] Could not allocate php server pipes", i));
-			return FALSE;
+			goto cleanup;
 		}
 
 		/* disable thread cancellation from this point forward. */
 		pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cancel_state);
+		cancel_held = TRUE;
 
 		/* establish arguments for script server execution */
 		if (set.cacti_version <= 1222) {
@@ -412,39 +636,66 @@ int php_init(int php_process) {
 		SPINE_LOG_DEBUG(("DEBUG: SS[%i] PHP Script Server About to spawn Child Process", i));
 
 		{
-			posix_spawn_file_actions_t fa;
 			int spawn_err;
 
 			if (posix_spawn_file_actions_init(&fa) != 0) {
 				SPINE_LOG(("ERROR: SS[%i] posix_spawn_file_actions_init failed", i));
-				close(cacti2php_pdes[0]);
-				close(cacti2php_pdes[1]);
-				close(php2cacti_pdes[0]);
-				close(php2cacti_pdes[1]);
-				pthread_setcancelstate(cancel_state, NULL);
-				return FALSE;
+				goto cleanup;
 			}
 
+			fa_valid = TRUE;
+			if (spine_spawnattr_sigpipe_default(&attr) != 0) {
+				SPINE_LOG(("ERROR: SS[%i] posix_spawnattr setup failed: %s", i, strerror(errno)));
+				goto cleanup;
+			}
+			attr_valid = TRUE;
+
 			/* wire cacti->php read end to child stdin, php->cacti write end to child stdout */
-			if (posix_spawn_file_actions_adddup2(&fa, cacti2php_pdes[0], STDIN_FILENO) != 0 ||
-			    posix_spawn_file_actions_adddup2(&fa, php2cacti_pdes[1], STDOUT_FILENO) != 0 ||
-			    /* close all four pipe ends in the child after dup2 redirects are in place */
-			    posix_spawn_file_actions_addclose(&fa, cacti2php_pdes[0]) != 0 ||
-			    posix_spawn_file_actions_addclose(&fa, cacti2php_pdes[1]) != 0 ||
-			    posix_spawn_file_actions_addclose(&fa, php2cacti_pdes[0]) != 0 ||
-			    posix_spawn_file_actions_addclose(&fa, php2cacti_pdes[1]) != 0) {
+			/* The pipe ends are close-on-exec, and dup2 clears that on its target, so
+			 * the usual case is fine. When an end already sits on the descriptor it is
+			 * destined for, dup2(fd, fd) is a no-op that clears nothing and the
+			 * unconditional close below would then shut the child's stdin or stdout.
+			 * The script server would exec with it closed, never answer, and every
+			 * script-server data source would record U with no diagnostic. Reaching it
+			 * needs spine to start with fd 0 or 1 closed, which a daemon can do.
+			 * Same treatment as nft_popen(): dup() to a fresh descriptor, mark the
+			 * temporary copy close-on-exec, then use a child file action to dup2 it. */
+			child_stdin  = cacti2php_pdes[0];
+			child_stdout = php2cacti_pdes[1];
+
+			if (child_stdin == STDIN_FILENO) {
+				dup_stdin = dup(child_stdin);
+				if (dup_stdin >= 0 && spine_set_cloexec(dup_stdin) != 0) {
+					php_close_fd(&dup_stdin);
+				}
+				child_stdin = dup_stdin;
+			}
+
+			if (child_stdout == STDOUT_FILENO) {
+				dup_stdout = dup(child_stdout);
+				if (dup_stdout >= 0 && spine_set_cloexec(dup_stdout) != 0) {
+					php_close_fd(&dup_stdout);
+				}
+				child_stdout = dup_stdout;
+			}
+
+			if (child_stdin < 0 || child_stdout < 0 ||
+			    posix_spawn_file_actions_adddup2(&fa, child_stdin, STDIN_FILENO) != 0 ||
+			    posix_spawn_file_actions_adddup2(&fa, child_stdout, STDOUT_FILENO) != 0 ||
+			    /* close the pipe ends the child does not need. Skip fd 0 and 1: after
+			       the redirects above they hold the copies the child polls on. */
+			    php_addclose_unless_std(&fa, cacti2php_pdes[0]) != 0 ||
+			    php_addclose_unless_std(&fa, cacti2php_pdes[1]) != 0 ||
+			    php_addclose_unless_std(&fa, php2cacti_pdes[0]) != 0 ||
+			    php_addclose_unless_std(&fa, php2cacti_pdes[1]) != 0 ||
+			    (dup_stdin  != -1 && posix_spawn_file_actions_addclose(&fa, dup_stdin)  != 0) ||
+			    (dup_stdout != -1 && posix_spawn_file_actions_addclose(&fa, dup_stdout) != 0)) {
 				SPINE_LOG(("ERROR: SS[%i] posix_spawn_file_actions setup failed", i));
-				posix_spawn_file_actions_destroy(&fa);
-				close(cacti2php_pdes[0]);
-				close(cacti2php_pdes[1]);
-				close(php2cacti_pdes[0]);
-				close(php2cacti_pdes[1]);
-				pthread_setcancelstate(cancel_state, NULL);
-				return FALSE;
+				goto cleanup;
 			}
 
 			do {
-				spawn_err = posix_spawn(&pid, argv[0], &fa, NULL, argv, environ);
+				spawn_err = posix_spawn(&pid, argv[0], &fa, &attr, argv, environ);
 				if ((spawn_err == EAGAIN || spawn_err == ENOMEM) && retry_count < 3) {
 					retry_count++;
 					#ifndef SOLAR_THREAD
@@ -456,6 +707,13 @@ int php_init(int php_process) {
 			} while (1);
 
 			posix_spawn_file_actions_destroy(&fa);
+			fa_valid = FALSE;
+			posix_spawnattr_destroy(&attr);
+			attr_valid = FALSE;
+
+			/* the child holds its own copies now */
+			php_close_fd(&dup_stdin);
+			php_close_fd(&dup_stdout);
 
 			if (spawn_err != 0) {
 				if (spawn_err == EAGAIN) {
@@ -466,15 +724,8 @@ int php_init(int php_process) {
 					SPINE_LOG(("ERROR: SS[%i] Could not spawn PHP Script Server Unknown Reason", i));
 				}
 
-				close(php2cacti_pdes[0]);
-				close(php2cacti_pdes[1]);
-				close(cacti2php_pdes[0]);
-				close(cacti2php_pdes[1]);
-
 				SPINE_LOG(("ERROR: SS[%i] Could not spawn PHP Script Server", i));
-				pthread_setcancelstate(cancel_state, NULL);
-
-				return FALSE;
+				goto cleanup;
 			}
 
 			SPINE_LOG_DEBUG(("DEBUG: SS[%i] PHP Script Server Child spawn Success", i));
@@ -482,60 +733,74 @@ int php_init(int php_process) {
 
 		/* Parent */
 		/* close unneeded pipes */
-		close(cacti2php_pdes[0]);
-		close(php2cacti_pdes[1]);
+		php_close_fd(&cacti2php_pdes[0]);
+		php_close_fd(&php2cacti_pdes[1]);
 
-		if (php_process == PHP_INIT) {
-			php_processes[i].php_pid = pid;
-			php_processes[i].php_write_fd = cacti2php_pdes[1];
-			php_processes[i].php_read_fd = php2cacti_pdes[0];
-		} else {
-			php_processes[php_process].php_pid = pid;
-			php_processes[php_process].php_write_fd = cacti2php_pdes[1];
-			php_processes[php_process].php_read_fd = php2cacti_pdes[0];
-		}
+		php_processes[slot].php_pid = pid;
+		php_processes[slot].php_write_fd = cacti2php_pdes[1];
+		php_processes[slot].php_read_fd = php2cacti_pdes[0];
+
+		/* php_processes[] owns these now; the cleanup below must not close them */
+		cacti2php_pdes[1] = -1;
+		php2cacti_pdes[0] = -1;
 
 		/* restore caller's cancellation state. */
 		pthread_setcancelstate(cancel_state, NULL);
+		cancel_held = FALSE;
 
 		/* check pipe to insure startup took place */
-		if (php_process == PHP_INIT) {
-			result_string = php_readpipe(i, command);
-		} else {
-			result_string = php_readpipe(php_process, command);
-		}
+		result_string = php_read_result(slot, command, FALSE);
 
 		if (strstr(result_string, "Started")) {
-			if (php_process == PHP_INIT) {
-				SPINE_LOG_DEBUG(("DEBUG: SS[%i] Confirmed PHP Script Server running using readfd[%i], writefd[%i]", i, php2cacti_pdes[0], cacti2php_pdes[1]));
+			SPINE_LOG_DEBUG(("DEBUG: SS[%i] Confirmed PHP Script Server running using readfd[%i], writefd[%i]", slot, php_processes[slot].php_read_fd, php_processes[slot].php_write_fd));
 
-				php_processes[i].php_state = PHP_READY;
-			} else {
-				SPINE_LOG_DEBUG(("DEBUG: SS[%i] Confirmed PHP Script Server running using readfd[%i], writefd[%i]", php_process, php2cacti_pdes[0], cacti2php_pdes[1]));
-
-				php_processes[php_process].php_state = PHP_READY;
-			}
+			php_processes[slot].php_state = PHP_READY;
 		} else {
-			if (php_process == PHP_INIT) {
-				SPINE_LOG(("ERROR: SS[%i] Script Server did not start properly return message was: '%s'", i, result_string));
+			SPINE_LOG(("ERROR: SS[%i] Script Server did not start properly return message was: '%s'", slot, result_string));
 
-				php_processes[i].php_state = PHP_BUSY;
-			} else {
-				SPINE_LOG(("ERROR: SS[%i] Script Server did not start properly return message was: '%s'", php_process, result_string));
-
-				php_processes[php_process].php_state = PHP_BUSY;
-			}
+			php_processes[slot].php_state = PHP_BUSY;
 		}
 
-		free(result_string);
+		SPINE_FREE(result_string);
 	}
 
+	rc = TRUE;
+
+cleanup:
+	/* One owner for everything this function allocates. The five exits used to
+	 * spell their own teardown out and they had already drifted: every one of
+	 * them leaked `command`, and each carried a slightly different subset of
+	 * the closes. See ping_icmp() and #593 for the same shape. */
+	if (fa_valid) {
+		posix_spawn_file_actions_destroy(&fa);
+	}
+	if (attr_valid) {
+		posix_spawnattr_destroy(&attr);
+	}
+
+	php_close_fd(&dup_stdin);
+	php_close_fd(&dup_stdout);
+	php_close_fd(&cacti2php_pdes[0]);
+	php_close_fd(&cacti2php_pdes[1]);
+	php_close_fd(&php2cacti_pdes[0]);
+	php_close_fd(&php2cacti_pdes[1]);
+
+	if (cancel_held) {
+		pthread_setcancelstate(cancel_state, NULL);
+	}
+	if (!rc && php_processes != NULL && slot >= 0 && slot < MAX_PHP_SERVERS) {
+		php_processes[slot].php_pid = -1;
+		php_processes[slot].php_read_fd = -1;
+		php_processes[slot].php_write_fd = -1;
+		php_processes[slot].php_state = PHP_BUSY;
+	}
+
+	SPINE_FREE(result_string);
 	free(command);
 
-	return TRUE;
+	return rc;
 }
-
-static void php_terminate_and_reap(pid_t pid) {
+static int php_terminate_and_reap(pid_t pid) {
 	int attempts;
 	int phase;
 	int status;
@@ -553,12 +818,12 @@ static void php_terminate_and_reap(pid_t pid) {
 			} while (waited < 0 && errno == EINTR);
 
 			if (waited == pid || (waited < 0 && errno == ECHILD)) {
-				return;
+				return TRUE;
 			}
 
 			if (waited < 0) {
 				SPINE_LOG(("WARNING: Unable to reap PHP Script Server PID[%ld]: %s", (long)pid, strerror(errno)));
-				return;
+				return FALSE;
 			}
 
 			/* The delay is load-bearing: without it both phases burn twenty
@@ -570,7 +835,7 @@ static void php_terminate_and_reap(pid_t pid) {
 		signal_number = SIGKILL;
 	}
 
-	SPINE_LOG(("WARNING: PHP Script Server PID[%ld] did not exit after SIGKILL", (long)pid));
+	return FALSE;
 }
 
 /*! \fn void php_close(int php_process)
@@ -618,7 +883,7 @@ void php_close(int php_process) {
 		if (phpp->php_write_fd >= 0) {
 			static const char quit[] = "quit\r\n";
 
-			len = write(phpp->php_write_fd, quit, strlen(quit));
+			len = php_write_no_sigpipe(phpp->php_write_fd, quit, strlen(quit));
 
 			if (len < 0) {
 				SPINE_LOG_DEBUG(("DEBUG: SS[%i] Script Server quit write failed, closing anyway", i));
@@ -639,7 +904,9 @@ void php_close(int php_process) {
 	 	 * a process group leader), and PID 1 is "init".
 	  	 */
 		if (phpp->php_pid > 1) {
-			php_terminate_and_reap(phpp->php_pid);
+			if (!php_terminate_and_reap(phpp->php_pid)) {
+				nft_abandon_child(phpp->php_pid, "PHP child survived shutdown budget");
+			}
 
 			phpp->php_pid = -1;
 		}

php.h

@@ -38,5 +38,11 @@ extern char *php_readpipe(int php_process, char *command);
 extern int php_init(int php_process);
 extern void php_close(int php_process);
 extern int php_get_process(void);
+extern void php_processes_initialize(php_t *processes, int count);
+
+#ifdef SPINE_PHP_RUNTIME_TESTING
+extern ssize_t php_write_no_sigpipe_for_test(int fd, const void *buffer, size_t length);
+extern char *php_read_result_for_test(int php_process, char *command, int allow_restart);
+#endif
 
 #endif /* SPINE_PHP_H */

poller.c

@@ -2524,7 +2524,11 @@ char *exec_poll(host_t *current_host, char *command, int id, const char *type) {
 					SPINE_LOG_MEDIUM(("Device[%i] ERROR: The NIFTY POPEN timed out", current_host->id));
 
 					pid = nft_pchild(cmd_fd);
-					kill(pid, SIGKILL);
+					if (pid > 0) {
+						kill(pid, SIGKILL);
+					} else {
+						SPINE_LOG(("Device[%i] ERROR: Unable to find the timed-out POPEN child", current_host->id));
+					}
 					#endif
 
 					SET_UNDEFINED(result_string);

spine.c

@@ -246,12 +246,16 @@ int main(int argc, char *argv[]) {
 
 	/* establish php processes and initialize space */
 	php_processes = (php_t*) calloc(MAX_PHP_SERVERS, sizeof(php_t));
-	for (i = 0; i < MAX_PHP_SERVERS; i++) {
-		php_processes[i].php_state = PHP_BUSY;
+	if (php_processes == NULL) {
+		die("ERROR: Fatal malloc error: spine.c php_processes!");
 	}
+	php_processes_initialize(php_processes, MAX_PHP_SERVERS);
 
 	/* create the array of debug devices */
 	debug_devices = calloc(MAX_DEBUG_DEVICES, sizeof(int));
+	if (debug_devices == NULL) {
+		die("ERROR: Fatal malloc error: spine.c debug_devices!");
+	}
 
 	/* initialize icmp_avail */
 	set.icmp_avail = TRUE;

tests/integration/test_script_server_reap.sh

@@ -7,10 +7,8 @@
 # data source (poller_item.action = POLLER_ACTION_PHP_SCRIPT_SERVER = 2) and
 # asserts no zombie php child survives the run.
 #
-# Requires: docker compose AND a spine runtime image that bundles PHP plus a
-# script_server.php the poller can exec.  The default snmpv3 fixture image is
-# debian-slim with no PHP, so this test skips (exit 77) there with the exact
-# additions needed.  Wire it up in CI where the full Cacti image is available.
+# Requires docker compose. The checked-in fixture supplies PHP and the minimal
+# script-server protocol needed to exercise the real Spine process lifecycle.
 #
 # Usage: ./tests/integration/test_script_server_reap.sh
 set -euo pipefail
@@ -24,29 +22,22 @@ FAIL=0
 pass() { echo "  PASS: $*"; PASS=$((PASS+1)); }
 fail() { echo "  FAIL: $*"; FAIL=$((FAIL+1)); }
 
-skip() {
-	echo "  SKIP: $1"
-	exit 77
-}
-
 # ---------------------------------------------------------------------------
 # Preconditions: docker, the compose fixture, and a PHP-capable spine image.
 # ---------------------------------------------------------------------------
-command -v docker >/dev/null 2>&1 || skip "docker not installed"
-docker compose version >/dev/null 2>&1 || skip "docker compose plugin not available"
+command -v docker >/dev/null 2>&1 || { echo "FAIL: docker not installed"; exit 1; }
+docker compose version >/dev/null 2>&1 || { echo "FAIL: docker compose plugin not available"; exit 1; }
 
 echo ""
 echo "=== Setup: build spine image and probe for PHP ==="
 "${COMPOSE[@]}" build spine >/dev/null 2>&1 \
-	|| skip "spine image failed to build (build env not available)"
+	|| { echo "FAIL: spine image failed to build"; exit 1; }
 
 # The script server execs PHP; without it the action=2 path cannot run.
 if ! "${COMPOSE[@]}" run --rm --no-deps --entrypoint sh spine \
 		-c 'command -v php >/dev/null 2>&1'; then
-	skip "spine runtime image has no PHP. To run this test, extend
-        tests/snmpv3/Dockerfile (or use the full Cacti image) to install
-        php-cli and provide a script_server.php, then seed a poller_item with
-        action=2 (POLLER_ACTION_PHP_SCRIPT_SERVER)."
+	echo "FAIL: spine runtime image has no PHP" >&2
+	exit 1
 fi
 
 cleanup() {
@@ -69,6 +60,14 @@ done
 [[ "$count" -gt 0 ]] || { fail "database did not start"; exit 1; }
 pass "infrastructure ready"
 
+"${COMPOSE[@]}" exec -T db mariadb -uspine -pspine cacti -e "
+INSERT INTO settings (name, value) VALUES
+  ('path_webroot', '/opt/cacti'),
+  ('path_php_binary', '/usr/bin/php')
+ON DUPLICATE KEY UPDATE value = VALUES(value);
+" 2>/dev/null
+pass "PHP script-server settings seeded"
+
 # ---------------------------------------------------------------------------
 # Seed a script-server data source (action=2) for host 1.
 # ---------------------------------------------------------------------------
@@ -91,14 +90,19 @@ pass "script-server poller_item seeded"
 echo ""
 echo "=== Poll and check for zombie php children ==="
 
+set +e
 poll_out=$("${COMPOSE[@]}" run --rm --entrypoint sh spine -c '
 	/usr/local/bin/spine --conf=/etc/spine/spine.conf -f 1 -l 1 -S
 	echo "---PROCTABLE---"
 	ps -eo pid,ppid,stat,comm 2>/dev/null || true
-' 2>&1 || true)
+' 2>&1)
+poll_status=$?
+set -e
 echo "$poll_out"
 
-if echo "$poll_out" | grep -qi "segfault\|SIGSEGV\|Aborted"; then
+if [[ $poll_status -ne 0 ]]; then
+	fail "spine exited with status $poll_status during script-server poll"
+elif echo "$poll_out" | grep -qi "segfault\|SIGSEGV\|Aborted"; then
 	fail "spine crashed during script-server poll"
 else
 	pass "spine completed script-server poll without crash"
@@ -112,6 +116,14 @@ else
 	pass "no zombie php child after poll"
 fi
 
+value=$("${COMPOSE[@]}" exec -T db mariadb -uspine -pspine cacti -N -B \
+	-e "SELECT output FROM poller_output WHERE local_data_id = 900 LIMIT 1;" 2>/dev/null)
+if [[ $value == 42 ]]; then
+	pass "script-server command returned and stored 42"
+else
+	fail "script-server command did not store 42 (got '$value')"
+fi
+
 # ---------------------------------------------------------------------------
 # Summary
 # ---------------------------------------------------------------------------

tests/regression/test_child_process_safety.sh

@@ -1,41 +1,95 @@
 #!/bin/sh
+# Structural guard for the child process hardening.
+#
+# The behaviour is covered by tests/unit/test_linked.c, which opens a pipe and
+# checks the flag survives an exec, and by test_poll_host_release. This script
+# is the cheaper backstop for the failure that actually happened: PR #542
+# removed the whole mechanism in a stale-branch merge, and nothing noticed.
+#
+# It stayed red on develop from that merge onward because nothing ran it. It is
+# in TESTS now, so `make check` fails instead of a person having to remember.
 set -eu
 
+# automake runs the suite from the build directory, which is not the source
+# directory under `make distcheck`. Everything below reads source files.
+srcdir="${srcdir:-.}"
+cd "$srcdir"
+
 fail() {
 	echo "FAIL: $*" >&2
 	exit 1
 }
 
-grep -q '#include <fcntl.h>' common.h ||
-	fail "common.h must include fcntl.h for FD_CLOEXEC helpers"
-
-grep -q 'FD_CLOEXEC' php.c ||
-	fail "php.c must set close-on-exec on script-server pipe fds"
-
+# The close-on-exec helper lives in nft_popen.c and both callers share it.
 grep -q 'FD_CLOEXEC' nft_popen.c ||
-	fail "nft_popen.c must set close-on-exec on popen pipe fds"
+	fail "nft_popen.c must set close-on-exec on pipe fds"
 
-grep -q 'php_set_pipe_cloexec(cacti2php_pdes)' php.c ||
-	fail "php.c must protect cacti-to-php pipe fds with close-on-exec"
+grep -q 'spine_open_pipe_cloexec(pdes)' nft_popen.c ||
+	fail "nft_popen() must open its pipe with close-on-exec"
 
-grep -q 'php_set_pipe_cloexec(php2cacti_pdes)' php.c ||
-	fail "php.c must protect php-to-cacti pipe fds with close-on-exec"
+grep -q 'spine_open_pipe_cloexec(cacti2php_pdes)' php.c ||
+	fail "php.c must protect the cacti-to-php pipe with close-on-exec"
 
-grep -q 'set_pipe_cloexec(pdes)' nft_popen.c ||
-	fail "nft_popen.c must protect popen pipe fds with close-on-exec"
+grep -q 'spine_open_pipe_cloexec(php2cacti_pdes)' php.c ||
+	fail "php.c must protect the php-to-cacti pipe with close-on-exec"
 
+# The reap must be bounded and must escalate.
 grep -q 'waitpid(pid, pstat, WNOHANG)' nft_popen.c ||
 	fail "nft_popen.c must reap child processes with WNOHANG"
 
 grep -q 'kill(cur->pid, SIGKILL)' nft_popen.c ||
-	fail "nft_popen.c must escalate timed-out child reaping to SIGKILL"
+	fail "nft_popen.c must escalate a timed-out reap to SIGKILL"
 
 if grep -q 'waitpid(cur->pid, &pstat, 0)' nft_popen.c; then
-	fail "nft_popen.c must not use blocking waitpid() in nft_pclose"
+	fail "nft_pclose() must not block in waitpid()"
 fi
 
 if grep -q 'waitpid(phpp->php_pid, &wstatus, 0)' php.c; then
-	fail "php.c must not use blocking waitpid() in php_close"
+	fail "php_close() must not block in waitpid()"
 fi
 
 echo "PASS: child process safety invariants"
+
+# php_init() has one cleanup path that closes every descriptor it still holds,
+# so spine_open_pipe_cloexec() must not return FALSE while leaving live
+# descriptor numbers in the caller's array; that path would close them twice,
+# and in a threaded daemon the second close lands on whatever another thread
+# opened in between. The cloexec branch cannot be driven from a unit test:
+# fcntl(F_SETFD) does not fail on a valid descriptor, and ld --wrap cannot
+# intercept the call because it is inside the same translation unit.
+awk '/^int spine_open_pipe_cloexec/,/^}/' nft_popen.c |
+	grep -q 'pdes\[0\] = -1;' ||
+	fail "spine_open_pipe_cloexec() must clear pdes when it fails after opening the pipe"
+
+# php_init() must have exactly one teardown. Five hand-copied ones drifted and
+# every one of them leaked the command buffer.
+php_init_body=$(awk '/^int php_init\(int php_process\) \{/{f=1} f{print} f&&/^\}/{exit}' php.c)
+
+printf '%s\n' "$php_init_body" | grep -cE '^[[:space:]]+return FALSE;' | grep -qx '1' ||
+	fail "php_init() must reach its teardown by goto, not by a return that skips it"
+
+printf '%s\n' "$php_init_body" | grep -q '^cleanup:' ||
+	fail "php_init() must have a single cleanup label"
+
+# php_init() must not reach a read that can restart the server. php_readpipe()
+# used to restart from inside itself, and php_init() confirms startup by
+# reading, so a server that spawned but never answered recursed without bound,
+# spawning another server at every level. The handshake read must stay on the
+# non-restarting entry point.
+php_init_body=$(awk '/^int php_init\(int php_process\) \{/{f=1} f{print} f&&/^\}/{exit}' php.c)
+
+printf '%s\n' "$php_init_body" | grep -q 'php_read_result(slot, command, FALSE)' ||
+	fail "php_init() must read the startup handshake with restarts disabled"
+
+printf '%s\n' "$php_init_body" | grep -q 'php_readpipe(' &&
+	fail "php_init() must not call php_readpipe(), which may restart the server"
+
+awk '/^static char \*php_read_result/,/^\}/' php.c |
+	grep -q 'php_fail_read(php_process, allow_restart)' ||
+	fail "php_read_result() must route failures through the guarded restart helper"
+
+awk '/^static void php_fail_read/,/^\}/' php.c |
+	grep -q 'if (allow_restart) {' ||
+	fail "php_fail_read() must gate the server restart on allow_restart"
+
+exit 0

tests/snmpv3/cacti/script_server.php

@@ -0,0 +1,26 @@
+<?php
+/*
+ +-------------------------------------------------------------------------+
+ | Copyright (C) 2004-2026 The Cacti Group                                 |
+ |                                                                         |
+ | This program is free software; you can redistribute it and/or           |
+ | modify it under the terms of the GNU Lesser General Public License      |
+ | as published by the Free Software Foundation; either version 2.1       |
+ | of the License, or (at your option) any later version.                  |
+ +-------------------------------------------------------------------------+
+ */
+
+/* Minimal Cacti script-server protocol fixture for Spine integration tests. */
+
+fwrite(STDOUT, "Started\n");
+fflush(STDOUT);
+
+while (($line = fgets(STDIN)) !== false) {
+	$command = trim($line);
+	if ($command === 'quit') {
+		break;
+	}
+
+	fwrite(STDOUT, $command === 'ss_test.php ss_value 1' ? "42\n" : "U\n");
+	fflush(STDOUT);
+}

tests/snmpv3/docker-compose.yml

@@ -44,6 +44,7 @@ services:
     volumes:
       - ./spine/spine.conf:/etc/spine/spine.conf:ro
       - ./scripts/test-script.sh:/usr/local/bin/test-script.sh:ro
+      - ./cacti:/opt/cacti:ro
     entrypoint: ["/usr/local/bin/spine", "--conf=/etc/spine/spine.conf"]
     command: ["1", "1"]   # poll host_id 1
     # SPINE_LOG_LEVEL is not read by spine; verbosity is set via Log_Level in spine.conf

tests/unit/php_test_server.c

@@ -0,0 +1,32 @@
+#include <stdio.h>
+#include <signal.h>
+#include <string.h>
+
+int main(int argc, char **argv) {
+	char command[4096];
+	const char *mode = argc > 2 ? argv[2] : "";
+	if (strstr(mode, "exit-before-start") != NULL) return 0;
+
+	if (strstr(mode, "silent") == NULL) {
+		struct sigaction action;
+		int bad_start = strstr(mode, "bad-start") != NULL;
+
+		if (strstr(mode, "check-sigpipe") != NULL &&
+		    (sigaction(SIGPIPE, NULL, &action) != 0 || action.sa_handler != SIG_DFL)) {
+			bad_start = 1;
+		}
+		puts(bad_start ? "Not ready" : "Started");
+		fflush(stdout);
+	}
+
+	while (fgets(command, sizeof(command), stdin) != NULL) {
+		if (strcmp(command, "quit\r\n") == 0 || strcmp(command, "quit\n") == 0) {
+			break;
+		}
+
+		puts(strcmp(command, "poll 7\r\n") == 0 ? "42" : "unexpected command");
+		fflush(stdout);
+	}
+
+	return 0;
+}

tests/unit/test_linked.c

@@ -14,12 +14,27 @@
 
 #include <string.h>
 #include <stdlib.h>
+#include <errno.h>
+#include <pthread.h>
+#include <signal.h>
+#include <sys/wait.h>
+#include <unistd.h>
 
 #include "common.h"
 #include "spine.h"
 #include "util.h"
 #include "ping.h"
 
+#if !defined(ICMP_DEST_UNREACH) && defined(ICMP_UNREACH)
+#define ICMP_DEST_UNREACH ICMP_UNREACH
+#endif
+#include "nft_popen.h"
+
+#include <fcntl.h>
+#include <signal.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
 /* provided by tests/fuzz/stubs.c, as spine.c would */
 extern int *debug_devices;
 
@@ -457,7 +472,429 @@ static void test_is_debug_device_matches_only_listed_ids(void **state) {
 	debug_devices = saved;
 }
 
+/* --- nft_popen(): registry entries have exactly one closing owner -------- */
+
+struct close_result {
+	int fd;
+	int result;
+	int error;
+};
+
+static void *close_from_thread(void *arg) {
+	struct close_result *result = arg;
+
+	result->result = nft_pclose(result->fd);
+	result->error = errno;
+
+	return NULL;
+}
+
+static void test_nft_pclose_has_one_owner_per_registry_entry(void **state) {
+	static struct close_result results[2];
+	pthread_t threads[2];
+	int create_results[2] = {-1, -1};
+	int fd;
+	int successes = 0;
+	int bad_fds = 0;
+	int owner_result = -1;
+	int join_results[2] = {-1, -1};
+	int reap_error;
+	int reap_result;
+	pid_t child;
+	int i;
+	(void) state;
+
+	fd = nft_popen("exit 7", "r");
+	assert_true(fd >= 0);
+	child = nft_pchild(fd);
+	assert_true(child > 0);
+
+	for (i = 0; i < 2; i++) {
+		results[i].fd = fd;
+		results[i].result = -1;
+		results[i].error = 0;
+		create_results[i] = pthread_create(&threads[i], NULL, close_from_thread, &results[i]);
+	}
+
+	for (i = 0; i < 2; i++) {
+		if (create_results[i] == 0) {
+			join_results[i] = pthread_join(threads[i], NULL);
+		}
+
+		if (results[i].result >= 0) {
+			successes++;
+			owner_result = results[i].result;
+		} else if (results[i].error == EBADF) {
+			bad_fds++;
+		}
+	}
+
+	/* Avoid leaking the child if thread creation failed before either closer ran. */
+	if (successes == 0) {
+		(void)nft_pclose(fd);
+	}
+
+	assert_int_equal(create_results[0], 0);
+	assert_int_equal(create_results[1], 0);
+	assert_int_equal(join_results[0], 0);
+	assert_int_equal(join_results[1], 0);
+	assert_int_equal(successes, 1);
+	assert_int_equal(bad_fds, 1);
+	assert_true(WIFEXITED(owner_result));
+	assert_int_equal(WEXITSTATUS(owner_result), 7);
+
+	errno = 0;
+	reap_result = waitpid(child, NULL, WNOHANG);
+	reap_error = errno;
+	assert_int_equal(fcntl(fd, F_GETFD), -1);
+	assert_int_equal(errno, EBADF);
+	assert_int_equal(reap_result, -1);
+	assert_int_equal(reap_error, ECHILD);
+}
+
+static void test_nft_pclose_cancellation_releases_registry_entry(void **state) {
+	struct close_result result;
+	pthread_t thread;
+	void *thread_result = NULL;
+	pid_t child;
+	int cancel_result = -1;
+	int create_result;
+	int detached = 0;
+	int fd;
+	int i;
+	int join_result = -1;
+	int lookup_error;
+	int lookup_result;
+	char ready;
+	int status;
+	(void) state;
+
+	fd = nft_popen("printf x; kill -STOP $$", "r");
+	assert_true(fd >= 0);
+	child = nft_pchild(fd);
+	assert_true(child > 0);
+	assert_int_equal(read(fd, &ready, 1), 1);
+	assert_int_equal(ready, 'x');
+
+	result.fd = fd;
+	result.result = -1;
+	result.error = 0;
+	create_result = pthread_create(&thread, NULL, close_from_thread, &result);
+	if (create_result == 0) {
+		/* The registry transition, rather than elapsed time, proves the closer has
+		 * taken exclusive ownership and reached waitpid().
+		 */
+		for (i = 0; i < 5000; i++) {
+			errno = 0;
+			if (nft_pchild(fd) == -1 && errno == EBADF) {
+				detached = 1;
+				break;
+			}
+			usleep(1000);
+		}
+
+		if (detached) {
+			cancel_result = pthread_cancel(thread);
+		} else {
+			kill(child, SIGKILL);
+		}
+		join_result = pthread_join(thread, &thread_result);
+	}
+
+	errno = 0;
+	lookup_result = nft_pchild(fd);
+	lookup_error = errno;
+
+	/* Cancellation stops nft_pclose() before it can reap.  Clean up the child
+	 * before asserting because cmocka assertions longjmp.
+	 */
+	if (lookup_result > 0) {
+		kill(lookup_result, SIGKILL);
+		(void)nft_pclose(fd);
+	} else if (detached) {
+		kill(child, SIGKILL);
+		do {
+			status = waitpid(child, NULL, 0);
+		} while (status < 0 && errno == EINTR);
+	}
+
+	assert_int_equal(create_result, 0);
+	assert_true(detached);
+	assert_int_equal(cancel_result, 0);
+	assert_int_equal(join_result, 0);
+	assert_ptr_equal(thread_result, PTHREAD_CANCELED);
+	assert_int_equal(lookup_result, -1);
+	assert_int_equal(lookup_error, EBADF);
+}
+
+static void test_nft_pclose_early_error_preserves_cancellation_mode(void **state) {
+	int cancel_state_after;
+	int cancel_state_before;
+	(void) state;
+
+	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cancel_state_before);
+	pthread_setcancelstate(cancel_state_before, NULL);
+
+	errno = 0;
+	assert_int_equal(nft_pclose(-1), -1);
+	assert_int_equal(errno, EBADF);
+	errno = 0;
+	assert_int_equal(nft_pchild(-1), -1);
+	assert_int_equal(errno, EBADF);
+
+	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cancel_state_after);
+	pthread_setcancelstate(cancel_state_after, NULL);
+
+	assert_int_equal(cancel_state_after, cancel_state_before);
+}
+
+/* ---------------------------------------------------------------------------
+ * Child process hardening (nft_popen.c)
+ *
+ * PR #542 removed the close-on-exec and bounded-reap code PR #557 had just
+ * added, and nothing failed, because the only guard was a shell script that
+ * grepped the source and was deleted in the same commit. These exercise the
+ * behaviour against the shipped object instead.
+ * ------------------------------------------------------------------------- */
+
+static void test_cloexec_is_set_on_both_pipe_ends(void **state) {
+	int pdes[2];
+	int i;
+
+	(void) state;
+
+	assert_true(spine_open_pipe_cloexec(pdes));
+
+	for (i = 0; i < 2; i++) {
+		int flags = fcntl(pdes[i], F_GETFD);
+
+		assert_true(flags >= 0);
+		assert_true((flags & FD_CLOEXEC) != 0);
+	}
+
+	close(pdes[0]);
+	close(pdes[1]);
+}
+
+static void test_cloexec_pipe_is_a_working_pipe(void **state) {
+	int pdes[2];
+	char buf[8];
+
+	(void) state;
+
+	assert_true(spine_open_pipe_cloexec(pdes));
+	assert_int_equal(write(pdes[1], "ok", 2), 2);
+	assert_int_equal(read(pdes[0], buf, sizeof(buf)), 2);
+	assert_memory_equal(buf, "ok", 2);
+
+	close(pdes[0]);
+	close(pdes[1]);
+}
+
+static void test_duplicated_descriptor_is_close_on_exec(void **state) {
+	int original;
+	int duplicate;
+	int flags;
+
+	(void) state;
+	original = open("/dev/null", O_RDONLY);
+	assert_true(original >= 0);
+	duplicate = spine_dup_cloexec(original);
+	assert_true(duplicate >= 0);
+	flags = fcntl(duplicate, F_GETFD);
+	assert_true(flags >= 0);
+	assert_true((flags & FD_CLOEXEC) != 0);
+	close(duplicate);
+	close(original);
+}
+
+static void test_existing_pipe_on_stdout_does_not_close_a_new_child_redirect(void **state) {
+	int saved_stdin;
+	int saved_stdout;
+	int writer;
+	int reader;
+	int writer_status;
+	int reader_status;
+	ssize_t bytes;
+	char output[32] = {0};
+
+	(void) state;
+	saved_stdin = dup(STDIN_FILENO);
+	saved_stdout = dup(STDOUT_FILENO);
+	assert_true(saved_stdin >= 0);
+	assert_true(saved_stdout >= 0);
+	close(STDIN_FILENO);
+	close(STDOUT_FILENO);
+
+	/* The write-mode parent retains fd 1 in PidList. The following read-mode
+	 * child also redirects its new pipe onto fd 1. Its later PidList close walk
+	 * must not close that newly installed stdout. */
+	writer = nft_popen("cat >/dev/null", "w");
+	reader = nft_popen("printf second-child-visible", "r");
+	bytes = reader >= 0 ? read(reader, output, sizeof(output) - 1) : -1;
+	reader_status = reader >= 0 ? nft_pclose(reader) : -1;
+	writer_status = writer >= 0 ? nft_pclose(writer) : -1;
+
+	dup2(saved_stdin, STDIN_FILENO);
+	dup2(saved_stdout, STDOUT_FILENO);
+	close(saved_stdin);
+	close(saved_stdout);
+
+	assert_true(writer >= 0);
+	assert_true(reader >= 0);
+	assert_true(bytes > 0);
+	assert_string_equal(output, "second-child-visible");
+	assert_true(WIFEXITED(reader_status));
+	assert_int_equal(WEXITSTATUS(reader_status), 0);
+	assert_true(WIFEXITED(writer_status));
+	assert_int_equal(WEXITSTATUS(writer_status), 0);
+}
+
+static void test_abandoned_children_are_swept_and_capacity_is_bounded(void **state) {
+	pid_t pids[NFT_ABANDONED_MAX + 1];
+	int i;
+	int created = 0;
+	int status;
+
+	(void) state;
+	for (i = 0; i < NFT_ABANDONED_MAX + 1; i++) {
+		pids[i] = fork();
+		if (pids[i] < 0)
+			break;
+		if (pids[i] == 0) {
+			pause();
+			_exit(0);
+		}
+		created++;
+		nft_abandon_child(pids[i], "unit test");
+	}
+
+	if (created != NFT_ABANDONED_MAX + 1) {
+		for (i = 0; i < created; i++) {
+			(void)kill(pids[i], SIGKILL);
+			(void)waitpid(pids[i], &status, 0);
+		}
+		(void)nft_abandoned_pending();
+		skip();
+	}
+
+	assert_int_equal(nft_abandoned_pending(), NFT_ABANDONED_MAX);
+	for (i = 0; i < created; i++) {
+		assert_int_equal(kill(pids[i], SIGKILL), 0);
+		assert_int_equal(waitpid(pids[i], &status, 0), pids[i]);
+	}
+	assert_int_equal(nft_abandoned_pending(), 0);
+}
+
+/* The descriptor must not survive an exec. A child that inherits the write end
+   keeps the pipe open, so the polling thread never sees EOF and blocks to
+   script_timeout for a data source that already answered. */
+static void test_pipe_is_not_inherited_across_exec(void **state) {
+	int pdes[2];
+	int status;
+	pid_t pid;
+	char fdarg[32];
+
+	(void) state;
+
+	assert_true(spine_open_pipe_cloexec(pdes));
+	snprintf(fdarg, sizeof(fdarg), "/proc/self/fd/%d", pdes[1]);
+
+	pid = fork();
+	assert_true(pid >= 0);
+
+	if (pid == 0) {
+		/* exits 0 when the descriptor survived exec, 1 when it did not */
+		execl("/bin/sh", "sh", "-c", "test -e \"$0\"", fdarg, (char *) NULL);
+		_exit(127);
+	}
+
+	assert_int_equal(waitpid(pid, &status, 0), pid);
+	assert_true(WIFEXITED(status));
+	assert_int_equal(WEXITSTATUS(status), 1);
+
+	close(pdes[0]);
+	close(pdes[1]);
+}
+
+static void test_reap_returns_still_running_rather_than_blocking(void **state) {
+	int pstat = 0;
+	int status;
+	pid_t pid;
+
+	(void) state;
+
+	pid = fork();
+	assert_true(pid >= 0);
+
+	if (pid == 0) {
+		pause();
+		_exit(0);
+	}
+
+	/* the shipped code blocked here forever; two attempts must come back */
+	assert_int_equal(spine_reap_child_bounded(pid, &pstat, 2), 1);
+
+	assert_int_equal(kill(pid, SIGKILL), 0);
+	assert_int_equal(waitpid(pid, &status, 0), pid);
+}
+
+static void test_reap_collects_an_exited_child(void **state) {
+	int pstat = 0;
+	pid_t pid;
+
+	(void) state;
+
+	pid = fork();
+	assert_true(pid >= 0);
+
+	if (pid == 0) {
+		_exit(3);
+	}
+
+	assert_int_equal(spine_reap_child_bounded(pid, &pstat, 20), 0);
+	assert_true(WIFEXITED(pstat));
+	assert_int_equal(WEXITSTATUS(pstat), 3);
+}
+
+static void test_reap_reports_an_already_reaped_child(void **state) {
+	int pstat = 99;
+	int status;
+	pid_t pid;
+
+	(void) state;
+
+	pid = fork();
+	assert_true(pid >= 0);
+
+	if (pid == 0) {
+		_exit(0);
+	}
+
+	assert_int_equal(waitpid(pid, &status, 0), pid);
+
+	/* ECHILD: someone else took the status, which is success with none */
+	assert_int_equal(spine_reap_child_bounded(pid, &pstat, 2), 0);
+	assert_int_equal(pstat, 0);
+}
+
+static void test_nft_pclose_requests_graceful_termination_before_kill(void **state) {
+	char ready[6] = {0};
+	int fd;
+	int status;
+
+	(void) state;
+	fd = nft_popen("trap 'exit 0' TERM; printf ready; while :; do sleep 1; done", "r");
+	assert_true(fd >= 0);
+	assert_int_equal(read(fd, ready, 5), 5);
+	assert_string_equal(ready, "ready");
+	status = nft_pclose(fd);
+	assert_true(WIFEXITED(status));
+	assert_int_equal(WEXITSTATUS(status), 0);
+}
+
 int main(void) {
+
 	const struct CMUnitTest tests[] = {
 		cmocka_unit_test(test_strncopy_truncates_within_the_buffer),
 		cmocka_unit_test(test_strncopy_copies_a_short_source_whole),
@@ -496,6 +933,19 @@ int main(void) {
 		cmocka_unit_test(test_get_date_format_clamps_an_out_of_range_format),
 		cmocka_unit_test(test_get_date_format_covers_each_supported_format),
 		cmocka_unit_test(test_is_debug_device_matches_only_listed_ids),
+		cmocka_unit_test(test_nft_pclose_has_one_owner_per_registry_entry),
+		cmocka_unit_test(test_nft_pclose_cancellation_releases_registry_entry),
+		cmocka_unit_test(test_nft_pclose_early_error_preserves_cancellation_mode),
+		cmocka_unit_test(test_cloexec_is_set_on_both_pipe_ends),
+		cmocka_unit_test(test_cloexec_pipe_is_a_working_pipe),
+		cmocka_unit_test(test_duplicated_descriptor_is_close_on_exec),
+		cmocka_unit_test(test_existing_pipe_on_stdout_does_not_close_a_new_child_redirect),
+		cmocka_unit_test(test_pipe_is_not_inherited_across_exec),
+		cmocka_unit_test(test_reap_returns_still_running_rather_than_blocking),
+		cmocka_unit_test(test_reap_collects_an_exited_child),
+		cmocka_unit_test(test_reap_reports_an_already_reaped_child),
+		cmocka_unit_test(test_nft_pclose_requests_graceful_termination_before_kill),
+		cmocka_unit_test(test_abandoned_children_are_swept_and_capacity_is_bounded),
 	};
 
 	return cmocka_run_group_tests(tests, NULL, NULL);