Restore the close-on-exec and bounded reap removed by #542

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
@@ -101,6 +103,92 @@ static pthread_mutex_t ListMutex = PTHREAD_MUTEX_INITIALIZER;
 
 static void	close_cleanup(void *);
 
+/* 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. */
+#define NFT_PCLOSE_REAP_USEC 50000
+#define NFT_PCLOSE_TERM_ATTEMPTS 100
+#define NFT_PCLOSE_KILL_ATTEMPTS 20
+
+int spine_set_cloexec(int fd) {
+	int flags;
+
+	flags = fcntl(fd, F_GETFD);
+	if (flags < 0) {
+		return -1;
+	}
+
+	return fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
+}
+
+/*! \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) {
+		return FALSE;
+	}
+
+	if (spine_set_cloexec(pdes[0]) != 0 || spine_set_cloexec(pdes[1]) != 0) {
+		(void)close(pdes[0]);
+		(void)close(pdes[1]);
+		return FALSE;
+	}
+
+	return TRUE;
+}
+
+/*! \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;
+
+	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) {
+			return -1;
+		}
+
+		/* The delay is load-bearing: without it the attempts are spent in
+		   nanoseconds and SIGKILL lands before the child can exit. */
+		#ifndef SOLAR_THREAD
+		usleep(NFT_PCLOSE_REAP_USEC);
+		#else
+		sleep(1);
+		#endif
+	}
+
+	return 1;
+}
+
 /*! ------------------------------------------------------------------------------
  *
  *  nft_popen
@@ -154,7 +242,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. */
@@ -356,8 +444,23 @@ nft_pclose(int fd)
 
 	cur->fd = -1;		/* Prevent the fd being closed twice. */
 
-	do { pid = waitpid(cur->pid, &pstat, 0);
-	} while (pid == -1 && errno == EINTR);
+	switch (spine_reap_child_bounded(cur->pid, &pstat, NFT_PCLOSE_TERM_ATTEMPTS)) {
+	case 0:
+		pid = cur->pid;
+		break;
+	case 1:
+		(void)kill(cur->pid, SIGKILL);
+		if (spine_reap_child_bounded(cur->pid, &pstat, NFT_PCLOSE_KILL_ATTEMPTS) == 0) {
+			pid = cur->pid;
+		} else {
+			errno = ETIMEDOUT;
+			pid = -1;
+		}
+		break;
+	default:
+		pid = -1;
+		break;
+	}
 
 	pthread_cleanup_pop(1);	/* Execute the cleanup handler. */
 

nft_popen.h

@@ -94,4 +94,36 @@ 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);
+
+/*!
+ *  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]);
+
+/*!
+ *  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);
+
 #endif /* SPINE_NFT_POPEN_H */

php.c

@@ -38,6 +38,7 @@
 
 extern char **environ;
 
+
 /*! \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
@@ -340,13 +341,15 @@ int php_init(int 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;
 		}
 
 		/* create the input pipes from php to Spine */
-		if (pipe(php2cacti_pdes) < 0) {
+		if (!spine_open_pipe_cloexec(php2cacti_pdes)) {
+			close(cacti2php_pdes[0]);
+			close(cacti2php_pdes[1]);
 			SPINE_LOG(("ERROR: SS[%i] Could not allocate php server pipes", i));
 			return FALSE;
 		}

tests/unit/test_linked.c

@@ -19,6 +19,12 @@
 #include "spine.h"
 #include "util.h"
 #include "ping.h"
+#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 +463,143 @@ static void test_is_debug_device_matches_only_listed_ids(void **state) {
 	debug_devices = saved;
 }
 
+/* ---------------------------------------------------------------------------
+ * 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]);
+}
+
+/* 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);
+}
+
 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 +638,12 @@ 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_cloexec_is_set_on_both_pipe_ends),
+		cmocka_unit_test(test_cloexec_pipe_is_a_working_pipe),
+		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),
 	};
 
 	return cmocka_run_group_tests(tests, NULL, NULL);