Fix subprocess registry ownership during close

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: Correct signed and unsigned printf format specifiers in poller.c, free session.localname on the unknown-version return in snmp.c, and quote shell variables in the build scripts
 -issue: Escape the SNMP result and RRD name before the poller_output INSERT, bound the buffer_output_errors write to the space left in error_string, and validate the --hostlist argument before it reaches SQL
 -issue: Restore the twelve headers and spine.conf.dist missing from the dist tarball so a release tarball can be compiled from

nft_popen.c

@@ -101,6 +101,37 @@ static pthread_mutex_t ListMutex = PTHREAD_MUTEX_INITIALIZER;
 
 static void	close_cleanup(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_popen
@@ -330,31 +361,25 @@ 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);
-
-	cur->fd = -1;		/* Prevent the fd being closed twice. */
+	pthread_setcancelstate(cancel_state, NULL);
 
 	do { pid = waitpid(cur->pid, &pstat, 0);
 	} while (pid == -1 && errno == EINTR);
@@ -372,29 +397,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);
 }

poller.c

@@ -2486,7 +2486,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);

tests/unit/test_linked.c

@@ -14,11 +14,17 @@
 
 #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"
+#include "nft_popen.h"
 
 /* provided by tests/fuzz/stubs.c, as spine.c would */
 extern int *debug_devices;
@@ -457,6 +463,182 @@ 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);
+}
+
 int main(void) {
 	const struct CMUnitTest tests[] = {
 		cmocka_unit_test(test_strncopy_truncates_within_the_buffer),
@@ -496,6 +678,9 @@ 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),
 	};
 
 	return cmocka_run_group_tests(tests, NULL, NULL);