From 502b3765f0829356a7a12087089eedebff17639c Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Thu, 3 Sep 2026 02:54:28 -0700 Subject: [PATCH 01/15] fix: give nft_pclose exclusive registry ownership Detach the subprocess entry under ListMutex before exposing it to close, wait, or cancellation cleanup. A second closer now receives EBADF instead of sharing a pointer that can be freed concurrently. Keep cancellation disabled through descriptor close so cleanup cannot close a reused descriptor number. Cover concurrent closers and cancellation against the shipped nft_popen object. Closes #610 Signed-off-by: Thomas Vincent --- CHANGELOG | 1 + nft_popen.c | 80 ++++++++--------- poller.c | 6 +- tests/unit/test_linked.c | 185 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 232 insertions(+), 40 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 4833ff70..6918b3db 100644 --- a/CHANGELOG +++ b/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 diff --git a/nft_popen.c b/nft_popen.c index d4d2fd1a..5b6de90c 100644 --- a/nft_popen.c +++ b/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); } diff --git a/poller.c b/poller.c index b03cd6e2..d651f947 100644 --- a/poller.c +++ b/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); diff --git a/tests/unit/test_linked.c b/tests/unit/test_linked.c index 71371452..c59df2b3 100644 --- a/tests/unit/test_linked.c +++ b/tests/unit/test_linked.c @@ -14,11 +14,17 @@ #include #include +#include +#include +#include +#include +#include #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); From 88a42c84d16fc895467a2bb2d6268c1c614c6f7d Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Wed, 2 Sep 2026 17:16:34 -0700 Subject: [PATCH 02/15] fix: restore the close-on-exec and bounded reap in nft_popen PR #542 removed both while adding unrelated poller fixes; its branch predated only guard was a shell script that grepped the source, and it was deleted in the same commit. An inherited pipe write end keeps a script's reader from seeing EOF, so the thread blocks to script_timeout for a device that answered. The unbounded waitpid has no timeout at all, and the thread holds its available_scripts token while it waits. The php.c reap survived #542 and is better than what was removed, so only its pipes needed the flag. Both callers now share one helper, covered by six tests against the shipped object: two of them fail if the close-on-exec goes away again. Signed-off-by: Thomas Vincent --- nft_popen.c | 109 ++++++++++++++++++++++++++++- nft_popen.h | 32 +++++++++ php.c | 7 +- tests/unit/test_linked.c | 147 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 290 insertions(+), 5 deletions(-) diff --git a/nft_popen.c b/nft_popen.c index 5b6de90c..1a083e23 100644 --- a/nft_popen.c +++ b/nft_popen.c @@ -87,6 +87,8 @@ #include "common.h" #include "spine.h" #include +#include +#include /* An instance of this struct is created for each popen() fd. */ static struct pid @@ -132,6 +134,92 @@ static __attribute__((noinline)) struct pid *pid_list_close_and_take(int fd) 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. */ +#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 @@ -185,7 +273,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. */ @@ -381,8 +469,23 @@ nft_pclose(int fd) pthread_setcancelstate(cancel_state, NULL); - 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. */ diff --git a/nft_popen.h b/nft_popen.h index ca835252..b2488718 100644 --- a/nft_popen.h +++ b/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 */ diff --git a/php.c b/php.c index b04a5e08..4c6b2bb7 100644 --- a/php.c +++ b/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; } diff --git a/tests/unit/test_linked.c b/tests/unit/test_linked.c index c59df2b3..59e8e2f8 100644 --- a/tests/unit/test_linked.c +++ b/tests/unit/test_linked.c @@ -26,6 +26,11 @@ #include "ping.h" #include "nft_popen.h" +#include +#include +#include +#include + /* provided by tests/fuzz/stubs.c, as spine.c would */ extern int *debug_devices; @@ -639,7 +644,143 @@ static void test_nft_pclose_early_error_preserves_cancellation_mode(void **state 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]); +} + +/* 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), @@ -681,6 +822,12 @@ int main(void) { 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_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); From 1366794fdd11ea5e2bad6a796f84cf500292f996 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sat, 5 Sep 2026 18:21:39 -0700 Subject: [PATCH 03/15] fix(child): harden pipe, reap, and PHP lifecycle paths Collect the child-process corrections from #597 after the foundational close-on-exec restoration. Preserve cancellation-safe abandoned-child handling, descriptor failure contracts, PHP resource ownership, startup restart bounds, and persistent SIGPIPE handling. Signed-off-by: Thomas Vincent --- error.c | 13 ++- nft_popen.c | 225 +++++++++++++++++++++++++++++++++++++++++++- nft_popen.h | 24 +++++ php.c | 262 ++++++++++++++++++++++++++++++++++++---------------- 4 files changed, 436 insertions(+), 88 deletions(-) diff --git a/error.c b/error.c index 0eecb3ce..e9fb4818 100644 --- a/error.c +++ b/error.c @@ -121,9 +121,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; } @@ -149,7 +146,6 @@ static void spine_signal_handler(int spine_signal) { static int spine_fatal_signals[] = { SIGINT, - SIGPIPE, SIGSEGV, SIGBUS, SIGFPE, @@ -170,6 +166,13 @@ void install_spine_signal_handler(void) { struct sigaction sa; void (*ohandler)(int); + /* A broken pipe is a normal condition here, not a fatal one: a script + server or popen child can exit at any time. The handler reset itself to + SIG_DFL on entry and never re-armed, so the second dead child spine wrote + to terminated the poller. Both write() call sites check their return, so + ignoring the signal lets them see EPIPE and handle it. */ + signal(SIGPIPE, SIG_IGN); + for (i=0; spine_fatal_signals[i]; ++i) { sigaction(spine_fatal_signals[i], NULL, &sa); if (sa.sa_handler == SIG_DFL) { @@ -200,6 +203,8 @@ void uninstall_spine_signal_handler(void) { struct sigaction sa; void (*ohandler)(int); + signal(SIGPIPE, SIG_DFL); + for (i=0; spine_fatal_signals[i]; ++i) { sigaction(spine_fatal_signals[i], NULL, &sa); if (sa.sa_handler == spine_signal_handler) { diff --git a/nft_popen.c b/nft_popen.c index 1a083e23..a7934566 100644 --- a/nft_popen.c +++ b/nft_popen.c @@ -101,7 +101,19 @@ 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 @@ -138,19 +150,39 @@ static __attribute__((noinline)) struct pid *pid_list_close_and_take(int fd) 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; } - return fcntl(fd, F_SETFD, flags | FD_CLOEXEC); + 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; } /*! \fn static int open_pipe_cloexec(int pdes[2]) @@ -170,12 +202,23 @@ int spine_set_cloexec(int fd) { */ 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; } @@ -189,6 +232,10 @@ 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); @@ -205,13 +252,26 @@ int spine_reap_child_bounded(pid_t pid, int *pstat, int attempts) { } 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. */ + 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 - usleep(NFT_PCLOSE_REAP_USEC); + if (attempt < NFT_PCLOSE_SPIN_ATTEMPTS) { + usleep(NFT_PCLOSE_SPIN_USEC); + } else { + usleep(NFT_PCLOSE_REAP_USEC); + } #else sleep(1); #endif @@ -249,6 +309,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]; @@ -304,6 +365,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) { @@ -317,6 +383,17 @@ 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. dup() the end to a fresh descriptor, which + * does not carry the flag, and let the child dup2 from that. */ if (*type == 'r') { posix_spawn_file_actions_addclose(&fa, pdes[0]); if (pdes[1] != STDOUT_FILENO) { @@ -324,13 +401,34 @@ 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 = dup(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 = dup(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]); } @@ -358,7 +456,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); @@ -370,6 +481,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]; @@ -478,11 +598,13 @@ nft_pclose(int fd) 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; } @@ -492,6 +614,99 @@ nft_pclose(int fd) 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. *------------------------------------------------------------------------------ diff --git a/nft_popen.h b/nft_popen.h index b2488718..d53daf08 100644 --- a/nft_popen.h +++ b/nft_popen.h @@ -126,4 +126,28 @@ extern int spine_open_pipe_cloexec(int pdes[2]); */ 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 */ diff --git a/php.c b/php.c index 4c6b2bb7..a55b7115 100644 --- a/php.c +++ b/php.c @@ -38,6 +38,34 @@ 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); +} /*! \fn char *php_cmd(const char *php_command, int php_process) * \brief calls the script server and executes a script command @@ -88,7 +116,6 @@ char *php_cmd(const char *php_command, int php_process) { /* 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); @@ -98,6 +125,10 @@ char *php_cmd(const char *php_command, int php_process) { if (retries < 3) { 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 = strdup("U"); } else { /* read the result from the php_command */ result_string = php_readpipe(php_process, command); @@ -164,7 +195,17 @@ 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; @@ -192,6 +233,16 @@ char *php_readpipe(int php_process, char *command) { * should only be the READ pipe */ retry: + /* 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 >= 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); + + return result_string; + } + /* initialize file descriptors to review for input/output */ FD_ZERO(&fds); FD_SET(php_processes[php_process].php_read_fd,&fds); @@ -242,8 +293,10 @@ 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); + if (allow_restart) { + php_close(php_process); + php_init(php_process); + } break; case 0: /* record end time */ @@ -252,8 +305,10 @@ 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); + if (allow_restart) { + php_close(php_process); + php_init(php_process); + } break; default: if (FD_ISSET(php_processes[php_process].php_read_fd, &fds)) { @@ -301,6 +356,16 @@ char *php_readpipe(int php_process, char *command) { 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 @@ -313,8 +378,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]; @@ -323,13 +388,26 @@ 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; + int fa_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; 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; @@ -338,24 +416,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 (!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 (!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; + 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) { @@ -397,35 +482,51 @@ 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; + /* 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, which does + * not carry the flag. */ + child_stdin = cacti2php_pdes[0]; + child_stdout = php2cacti_pdes[1]; + + if (child_stdin == STDIN_FILENO) { + dup_stdin = dup(child_stdin); + child_stdin = dup_stdin; + } + + if (child_stdout == STDOUT_FILENO) { + dup_stdout = dup(child_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 { @@ -441,6 +542,11 @@ int php_init(int php_process) { } while (1); posix_spawn_file_actions_destroy(&fa); + fa_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) { @@ -451,15 +557,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)); @@ -467,59 +566,64 @@ 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])); - - 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])); + 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[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)); - - 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)); + SPINE_LOG(("ERROR: SS[%i] Script Server did not start properly return message was: '%s'", slot, 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); + } + + 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); + } + + SPINE_FREE(result_string); free(command); - return TRUE; + return rc; } - static void php_terminate_and_reap(pid_t pid) { int attempts; int phase; From 4cde809388ef464dafda3a9d348522ef25746494 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sat, 5 Sep 2026 18:22:09 -0700 Subject: [PATCH 04/15] fix(php): close reviewed runtime failure paths Carry the PHP and signal-handling review fixes from #597 while leaving nft_pclose ownership to focused PR #614. The remaining registry-specific review changes will be replayed only after #614 lands. Signed-off-by: Thomas Vincent --- error.c | 29 +++++-- php.c | 240 ++++++++++++++++++++++++++++++++++++++++++++------------ php.h | 5 ++ 3 files changed, 217 insertions(+), 57 deletions(-) diff --git a/error.c b/error.c index e9fb4818..7c6c5a45 100644 --- a/error.c +++ b/error.c @@ -155,6 +155,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. @@ -166,12 +173,14 @@ void install_spine_signal_handler(void) { struct sigaction sa; void (*ohandler)(int); - /* A broken pipe is a normal condition here, not a fatal one: a script - server or popen child can exit at any time. The handler reset itself to - SIG_DFL on entry and never re-armed, so the second dead child spine wrote - to terminated the poller. Both write() call sites check their return, so - ignoring the signal lets them see EPIPE and handle it. */ - signal(SIGPIPE, SIG_IGN); + /* 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; + sigaction(SIGPIPE, &sa, NULL); for (i=0; spine_fatal_signals[i]; ++i) { sigaction(spine_fatal_signals[i], NULL, &sa); @@ -203,7 +212,13 @@ void uninstall_spine_signal_handler(void) { struct sigaction sa; void (*ohandler)(int); - signal(SIGPIPE, SIG_DFL); + sigaction(SIGPIPE, NULL, &sa); + if (sa.sa_handler == spine_sigpipe_handler) { + sa.sa_handler = SIG_DFL; + sigemptyset(&sa.sa_mask); + sa.sa_flags = 0; + sigaction(SIGPIPE, &sa, NULL); + } for (i=0; spine_fatal_signals[i]; ++i) { sigaction(spine_fatal_signals[i], NULL, &sa); diff --git a/php.c b/php.c index a55b7115..a595eac4 100644 --- a/php.c +++ b/php.c @@ -67,6 +67,58 @@ static int php_addclose_unless_std(posix_spawn_file_actions_t *fa, int fd) { 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_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 @@ -87,32 +139,32 @@ 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 strdup("U"); + } /* 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); + + /* Validate 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. */ + 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 strdup("U"); } /* 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) { @@ -139,24 +191,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; } @@ -167,23 +202,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 @@ -239,6 +346,15 @@ static char *php_read_result(int php_process, char *command, int allow_restart) 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); + /* 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_processes[php_process].php_state = PHP_BUSY; + if (allow_restart) { + php_close(php_process); + php_init(php_process); + } return result_string; } @@ -389,7 +505,9 @@ int php_init(int php_process) { char arg_mode_online[] = "--mode=online"; char arg_mode_offline[] = "--mode=offline"; 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; @@ -398,7 +516,7 @@ int php_init(int php_process) { int dup_stdout = -1; char *result_string = NULL; int num_processes; - int slot; + int slot = -1; int i; int rc = FALSE; char *command = strdup("INIT"); @@ -490,6 +608,11 @@ int php_init(int php_process) { } 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 */ /* The pipe ends are close-on-exec, and dup2 clears that on its target, so @@ -499,18 +622,24 @@ int php_init(int php_process) { * 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, which does - * not carry the flag. */ + * 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; } @@ -530,7 +659,7 @@ int php_init(int php_process) { } 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 @@ -543,6 +672,8 @@ int php_init(int php_process) { 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); @@ -607,6 +738,9 @@ int php_init(int php_process) { 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); @@ -618,6 +752,12 @@ int php_init(int php_process) { 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); @@ -707,7 +847,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)); diff --git a/php.h b/php.h index 8cf10c1c..6c94f534 100644 --- a/php.h +++ b/php.h @@ -39,4 +39,9 @@ extern int php_init(int php_process); extern void php_close(int php_process); extern int php_get_process(void); +#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 */ From c63169dbba687f11ed661f7346c1764c5de5d578 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sat, 5 Sep 2026 18:23:40 -0700 Subject: [PATCH 05/15] test(child): exercise PHP runtime and lifecycle guards Extract the production-linked PHP runtime suite, its script-server fixture, the portable linker-wrap probe, and the structural regression guard from #597. Signed-off-by: Thomas Vincent --- Makefile.am | 27 +- configure.ac | 20 + tests/regression/test_child_process_safety.sh | 23 + tests/unit/php_test_server.c | 32 + tests/unit/test_php_runtime.c | 949 ++++++++++++++++++ 5 files changed, 1050 insertions(+), 1 deletion(-) create mode 100644 tests/unit/php_test_server.c create mode 100644 tests/unit/test_php_runtime.c diff --git a/Makefile.am b/Makefile.am index 2ecbf61b..7cb78ee6 100644 --- a/Makefile.am +++ b/Makefile.am @@ -66,6 +66,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 @@ -86,4 +96,19 @@ tests_unit_test_linked_SOURCES = tests/unit/test_linked.c tests/fuzz/stubs.c \ sql.c util.c snmp.c locks.c poller.c nft_popen.c php.c ping.c 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 locks.c poller.c nft_popen.c php.c ping.c 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 diff --git a/configure.ac b/configure.ac index df11158b..561b5dde 100644 --- a/configure.ac +++ b/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([[ diff --git a/tests/regression/test_child_process_safety.sh b/tests/regression/test_child_process_safety.sh index 0d6d603c..18c5f53c 100755 --- a/tests/regression/test_child_process_safety.sh +++ b/tests/regression/test_child_process_safety.sh @@ -39,3 +39,26 @@ if grep -q 'waitpid(phpp->php_pid, &wstatus, 0)' php.c; then 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 '^\s+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" + +exit 0 diff --git a/tests/unit/php_test_server.c b/tests/unit/php_test_server.c new file mode 100644 index 00000000..6bd04335 --- /dev/null +++ b/tests/unit/php_test_server.c @@ -0,0 +1,32 @@ +#include +#include +#include + +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; +} diff --git a/tests/unit/test_php_runtime.c b/tests/unit/test_php_runtime.c new file mode 100644 index 00000000..7424cebf --- /dev/null +++ b/tests/unit/test_php_runtime.c @@ -0,0 +1,949 @@ +/* Production-linked PHP Script Server runtime tests. + * + * These use a real child, real close-on-exec pipes and the shipped php.c. + * They deliberately avoid a Cacti/PHP installation while covering the hot + * init -> command -> response -> close lifecycle and deterministic failures. + */ + +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "common.h" +#include "spine.h" +#include "php.h" + +static pthread_mutex_t spawn_barrier_mutex = PTHREAD_MUTEX_INITIALIZER; +static pthread_cond_t spawn_barrier_cond = PTHREAD_COND_INITIALIZER; +static int block_shell_spawn; +static int block_php_spawn; +static int shell_spawn_reached; +static int release_shell_spawn; +static int fail_next_dup; +static int fail_duplicated_cloexec; +static __thread int duplicated_fd = -1; +static int fail_next_setsigdefault; +static int track_spawnattr_destroy; +static int spawnattr_destroyed; +static int php_spawn_calls; +static int fail_php_spawn_call; +static int fail_next_sigmask; +static int track_write; +static int write_calls; + +int __real_pthread_sigmask(int how, const sigset_t *set, sigset_t *oldset); +int __wrap_pthread_sigmask(int how, const sigset_t *set, sigset_t *oldset) { + if (fail_next_sigmask && how == SIG_BLOCK) { + fail_next_sigmask = FALSE; + return EAGAIN; + } + return __real_pthread_sigmask(how, set, oldset); +} + +ssize_t __real_write(int fd, const void *buffer, size_t length); +ssize_t __wrap_write(int fd, const void *buffer, size_t length) { + if (track_write) write_calls++; + return __real_write(fd, buffer, length); +} + +int __real_dup(int fd); +int __wrap_dup(int fd) { + int result; + if (fail_next_dup) { + fail_next_dup = FALSE; + errno = EMFILE; + return -1; + } + result = __real_dup(fd); + duplicated_fd = result; + return result; +} + +int __real_fcntl(int fd, int command, ...); +int __wrap_fcntl(int fd, int command, ...) { + va_list args; + int argument; + + if (fail_duplicated_cloexec && fd == duplicated_fd && command == F_SETFD) { + fail_duplicated_cloexec = FALSE; + errno = EIO; + return -1; + } + if (command == F_GETFD) return __real_fcntl(fd, command); + va_start(args, command); + argument = va_arg(args, int); + va_end(args); + return __real_fcntl(fd, command, argument); +} + +int __real_posix_spawn(pid_t *pid, const char *path, + const posix_spawn_file_actions_t *actions, + const posix_spawnattr_t *attributes, + char *const argv[], char *const envp[]); + +int __wrap_posix_spawn(pid_t *pid, const char *path, + const posix_spawn_file_actions_t *actions, + const posix_spawnattr_t *attributes, + char *const argv[], char *const envp[]) { + if (strcmp(path, PHP_TEST_SERVER_PATH) == 0) { + php_spawn_calls++; + if (php_spawn_calls == fail_php_spawn_call) return EIO; + } + if ((block_shell_spawn && strcmp(path, "/bin/sh") == 0) || + (block_php_spawn && strcmp(path, PHP_TEST_SERVER_PATH) == 0)) { + pthread_mutex_lock(&spawn_barrier_mutex); + shell_spawn_reached = TRUE; + pthread_cond_broadcast(&spawn_barrier_cond); + while (!release_shell_spawn) { + pthread_cond_wait(&spawn_barrier_cond, &spawn_barrier_mutex); + } + pthread_mutex_unlock(&spawn_barrier_mutex); + } + + return __real_posix_spawn(pid, path, actions, attributes, argv, envp); +} + +int __real_posix_spawnattr_setsigdefault(posix_spawnattr_t *attr, const sigset_t *defaults); +int __wrap_posix_spawnattr_setsigdefault(posix_spawnattr_t *attr, const sigset_t *defaults) { + if (fail_next_setsigdefault) { + fail_next_setsigdefault = FALSE; + return EIO; + } + return __real_posix_spawnattr_setsigdefault(attr, defaults); +} + +int __real_posix_spawnattr_destroy(posix_spawnattr_t *attr); +int __wrap_posix_spawnattr_destroy(posix_spawnattr_t *attr) { + if (track_spawnattr_destroy) spawnattr_destroyed = TRUE; + return __real_posix_spawnattr_destroy(attr); +} + +static int php_setup(void **state) { + int i; + + (void) state; + memset(&set, 0, sizeof(set)); + init_mutexes(); + signal(SIGPIPE, SIG_IGN); + + php_processes = calloc(MAX_PHP_SERVERS, sizeof(*php_processes)); + assert_non_null(php_processes); + for (i = 0; i < MAX_PHP_SERVERS; i++) { + php_processes[i].php_state = PHP_BUSY; + php_processes[i].php_pid = -1; + php_processes[i].php_read_fd = -1; + php_processes[i].php_write_fd = -1; + } + + set.php_servers = 2; + set.script_timeout = 1; + set.cacti_version = 1300; + set.poller_id = 1; + set.log_destination = LOGDEST_STDOUT; + block_shell_spawn = FALSE; + block_php_spawn = FALSE; + shell_spawn_reached = FALSE; + release_shell_spawn = FALSE; + fail_next_dup = FALSE; + fail_duplicated_cloexec = FALSE; + duplicated_fd = -1; + fail_next_setsigdefault = FALSE; + track_spawnattr_destroy = FALSE; + spawnattr_destroyed = FALSE; + php_spawn_calls = 0; + fail_php_spawn_call = 0; + fail_next_sigmask = FALSE; + track_write = FALSE; + write_calls = 0; + snprintf(set.path_php, sizeof(set.path_php), "%s", PHP_TEST_SERVER_PATH); + snprintf(set.path_php_server, sizeof(set.path_php_server), "%s", "normal"); + return 0; +} + +static int php_teardown(void **state) { + int i; + + (void) state; + if (php_processes != NULL) { + for (i = 0; i < MAX_PHP_SERVERS; i++) { + if (php_processes[i].php_pid > 1) { + php_close(i); + } else { + if (php_processes[i].php_read_fd > STDERR_FILENO) + close(php_processes[i].php_read_fd); + if (php_processes[i].php_write_fd > STDERR_FILENO && + php_processes[i].php_write_fd != php_processes[i].php_read_fd) + close(php_processes[i].php_write_fd); + } + } + free(php_processes); + php_processes = NULL; + } + return 0; +} + +static void test_round_robin_wraps_at_server_count(void **state) { + (void) state; + php_processes[0].php_state = PHP_READY; + php_processes[0].php_pid = 10; + php_processes[0].php_read_fd = 10; + php_processes[0].php_write_fd = 11; + php_processes[1].php_state = PHP_READY; + php_processes[1].php_pid = 12; + php_processes[1].php_read_fd = 12; + php_processes[1].php_write_fd = 13; + set.php_current_server = set.php_servers; + assert_int_equal(php_get_process(), 0); + assert_int_equal(php_get_process(), 1); + assert_int_equal(php_get_process(), 0); + php_processes[0].php_pid = php_processes[0].php_read_fd = php_processes[0].php_write_fd = -1; + php_processes[1].php_pid = php_processes[1].php_read_fd = php_processes[1].php_write_fd = -1; +} + +static void test_round_robin_rejects_failed_slots(void **state) { + (void) state; + snprintf(set.path_php, sizeof(set.path_php), "%s", "/does/not/exist/spine-php-test"); + assert_int_equal(php_get_process(), -1); +} + +static void test_round_robin_returns_a_healthy_slot_when_all_are_contended(void **state) { + int i; + (void) state; + + for (i = 0; i < set.php_servers; i++) { + php_processes[i].php_state = PHP_READY; + php_processes[i].php_pid = 100 + i; + php_processes[i].php_read_fd = 20 + (i * 2); + php_processes[i].php_write_fd = 21 + (i * 2); + thread_mutex_lock(LOCK_PHP_PROC_0 + i); + } + + set.php_current_server = 0; + assert_int_equal(php_get_process(), 0); + + for (i = 0; i < set.php_servers; i++) { + thread_mutex_unlock(LOCK_PHP_PROC_0 + i); + php_processes[i].php_pid = -1; + php_processes[i].php_read_fd = -1; + php_processes[i].php_write_fd = -1; + } +} + +static void *hold_php_slot(void *arg) { + int slot = *(int *)arg; + + thread_mutex_lock(LOCK_PHP_PROC_0 + slot); + pthread_mutex_lock(&spawn_barrier_mutex); + shell_spawn_reached = TRUE; + pthread_cond_broadcast(&spawn_barrier_cond); + while (!release_shell_spawn) { + pthread_cond_wait(&spawn_barrier_cond, &spawn_barrier_mutex); + } + pthread_mutex_unlock(&spawn_barrier_mutex); + thread_mutex_unlock(LOCK_PHP_PROC_0 + slot); + return NULL; +} + +static void test_dead_slot_is_recovered_while_another_slot_is_contended(void **state) { + pthread_t holder; + int held_slot = 0; + int selected; + (void) state; + + php_processes[0].php_state = PHP_READY; + php_processes[0].php_pid = 100; + php_processes[0].php_read_fd = 20; + php_processes[0].php_write_fd = 21; + set.php_current_server = 0; + assert_int_equal(pthread_create(&holder, NULL, hold_php_slot, &held_slot), 0); + pthread_mutex_lock(&spawn_barrier_mutex); + while (!shell_spawn_reached) { + pthread_cond_wait(&spawn_barrier_cond, &spawn_barrier_mutex); + } + pthread_mutex_unlock(&spawn_barrier_mutex); + + selected = php_get_process(); + assert_int_equal(selected, 1); + assert_int_equal(php_processes[1].php_state, PHP_READY); + + pthread_mutex_lock(&spawn_barrier_mutex); + release_shell_spawn = TRUE; + pthread_cond_broadcast(&spawn_barrier_cond); + pthread_mutex_unlock(&spawn_barrier_mutex); + assert_int_equal(pthread_join(holder, NULL), 0); + php_processes[0].php_pid = -1; + php_processes[0].php_read_fd = -1; + php_processes[0].php_write_fd = -1; +} + +static void test_failed_recovery_falls_back_to_the_contended_slot(void **state) { + pthread_t holder; + int held_slot = 0; + int selected; + (void) state; + + php_processes[0].php_state = PHP_READY; + php_processes[0].php_pid = 100; + php_processes[0].php_read_fd = 20; + php_processes[0].php_write_fd = 21; + set.php_current_server = 0; + snprintf(set.path_php, sizeof(set.path_php), "%s", "/does/not/exist/spine-php-test"); + assert_int_equal(pthread_create(&holder, NULL, hold_php_slot, &held_slot), 0); + pthread_mutex_lock(&spawn_barrier_mutex); + while (!shell_spawn_reached) { + pthread_cond_wait(&spawn_barrier_cond, &spawn_barrier_mutex); + } + pthread_mutex_unlock(&spawn_barrier_mutex); + + selected = php_get_process(); + assert_int_equal(selected, 0); + + pthread_mutex_lock(&spawn_barrier_mutex); + release_shell_spawn = TRUE; + pthread_cond_broadcast(&spawn_barrier_cond); + pthread_mutex_unlock(&spawn_barrier_mutex); + assert_int_equal(pthread_join(holder, NULL), 0); + php_processes[0].php_pid = -1; + php_processes[0].php_read_fd = -1; + php_processes[0].php_write_fd = -1; +} + +static void noop_sigpipe_handler(int signal_number) { + (void) signal_number; +} + +static void test_broken_pipe_with_runtime_sigpipe_handler_does_not_block(void **state) { + struct sigaction action; + int pdes[2]; + pid_t child; + int status; + int attempts; + (void) state; + + assert_int_equal(pipe(pdes), 0); + close(pdes[0]); + child = fork(); + assert_true(child >= 0); + if (child == 0) { + memset(&action, 0, sizeof(action)); + action.sa_handler = noop_sigpipe_handler; + sigemptyset(&action.sa_mask); + if (sigaction(SIGPIPE, &action, NULL) != 0) _exit(2); + errno = 0; + if (php_write_no_sigpipe_for_test(pdes[1], "x", 1) != -1 || errno != EPIPE) + _exit(3); + _exit(0); + } + close(pdes[1]); + for (attempts = 0; attempts < 100; attempts++) { + pid_t waited = waitpid(child, &status, WNOHANG); + if (waited == child) break; + assert_true(waited == 0 || (waited < 0 && errno == EINTR)); + usleep(10000); + } + if (attempts == 100) { + kill(child, SIGKILL); + waitpid(child, &status, 0); + fail_msg("SIGPIPE-protected write blocked for more than one second"); + } + assert_true(WIFEXITED(status)); + assert_int_equal(WEXITSTATUS(status), 0); +} + +static void test_sigmask_failure_prevents_the_pipe_write(void **state) { + (void) state; + + fail_next_sigmask = TRUE; + track_write = TRUE; + errno = 0; + assert_int_equal(php_write_no_sigpipe_for_test(-1, "x", 1), -1); + assert_int_equal(errno, EAGAIN); + assert_int_equal(write_calls, 0); + track_write = FALSE; +} + +static void test_preexisting_pending_sigpipe_is_preserved(void **state) { + struct sigaction saved_action; + struct sigaction default_action; + sigset_t blocked; + sigset_t old_mask; + sigset_t pending; + int pdes[2]; + int received_signal = 0; + int result; + int saved_errno; + int pending_before; + int pending_after; + + (void) state; + assert_int_equal(sigaction(SIGPIPE, NULL, &saved_action), 0); + sigemptyset(&blocked); + sigaddset(&blocked, SIGPIPE); + assert_int_equal(pthread_sigmask(SIG_BLOCK, &blocked, &old_mask), 0); + memset(&default_action, 0, sizeof(default_action)); + default_action.sa_handler = SIG_DFL; + sigemptyset(&default_action.sa_mask); + assert_int_equal(sigaction(SIGPIPE, &default_action, NULL), 0); + assert_int_equal(raise(SIGPIPE), 0); + assert_int_equal(sigpending(&pending), 0); + pending_before = sigismember(&pending, SIGPIPE); + + assert_int_equal(pipe(pdes), 0); + close(pdes[0]); + errno = 0; + result = (int)php_write_no_sigpipe_for_test(pdes[1], "x", 1); + saved_errno = errno; + assert_int_equal(sigpending(&pending), 0); + pending_after = sigismember(&pending, SIGPIPE); + close(pdes[1]); + assert_int_equal(sigwait(&blocked, &received_signal), 0); + assert_int_equal(sigaction(SIGPIPE, &saved_action, NULL), 0); + assert_int_equal(pthread_sigmask(SIG_SETMASK, &old_mask, NULL), 0); + + assert_true(pending_before); + assert_int_equal(result, -1); + assert_int_equal(saved_errno, EPIPE); + assert_true(pending_after); + assert_int_equal(received_signal, SIGPIPE); +} + +static void test_spawnattr_sigpipe_failure_destroys_initialized_attr(void **state) { + posix_spawnattr_t attr; + (void) state; + + fail_next_setsigdefault = TRUE; + track_spawnattr_destroy = TRUE; + errno = 0; + assert_int_equal(spine_spawnattr_sigpipe_default(&attr), -1); + assert_int_equal(errno, EIO); + assert_true(spawnattr_destroyed); + track_spawnattr_destroy = FALSE; +} + +static void test_full_script_server_lifecycle(void **state) { + char *result; + + (void) state; + assert_int_equal(php_init(0), TRUE); + assert_int_equal(php_processes[0].php_state, PHP_READY); + assert_true(php_processes[0].php_pid > 1); + assert_true(php_processes[0].php_read_fd >= 0); + assert_true(php_processes[0].php_write_fd >= 0); + + result = php_cmd("poll 7", 0); + assert_non_null(result); + assert_string_equal(result, "42\n"); + free(result); + + php_close(0); + assert_int_equal(php_processes[0].php_pid, -1); + assert_int_equal(php_processes[0].php_read_fd, -1); + assert_int_equal(php_processes[0].php_write_fd, -1); +} + +static void test_php_child_restores_sigpipe_default(void **state) { + (void) state; + snprintf(set.path_php_server, sizeof(set.path_php_server), "%s", "check-sigpipe"); + assert_int_equal(php_init(0), TRUE); + assert_int_equal(php_processes[0].php_state, PHP_READY); +} + +static void test_script_server_lifecycle_with_stdio_closed(void **state) { + int saved_stdin; + int saved_stdout; + int init_result; + char *result = NULL; + + (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); + init_result = php_init(0); + if (init_result == TRUE) { + result = php_cmd("poll 7", 0); + php_close(0); + } + + dup2(saved_stdin, STDIN_FILENO); + dup2(saved_stdout, STDOUT_FILENO); + close(saved_stdin); + close(saved_stdout); + + assert_int_equal(init_result, TRUE); + assert_non_null(result); + assert_string_equal(result, "42\n"); + free(result); +} + +struct popen_thread_result { + int fd; +}; + +static void *open_script_while_blocked(void *arg) { + struct popen_thread_result *result = arg; + result->fd = nft_popen("printf concurrent-visible", "r"); + return NULL; +} + +static void test_php_spawn_does_not_inherit_an_nft_collision_descriptor(void **state) { + struct popen_thread_result opened = { .fd = -1 }; + pthread_t thread; + struct pollfd pfd; + char output[128] = {0}; + ssize_t n; + int total = 0; + int reached_eof = FALSE; + int saved_stdin; + int saved_stdout; + int init_result; + + (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); + + block_shell_spawn = TRUE; + assert_int_equal(pthread_create(&thread, NULL, open_script_while_blocked, &opened), 0); + pthread_mutex_lock(&spawn_barrier_mutex); + while (!shell_spawn_reached) { + pthread_cond_wait(&spawn_barrier_cond, &spawn_barrier_mutex); + } + pthread_mutex_unlock(&spawn_barrier_mutex); + + /* The nft_popen collision duplicate exists but has not spawned yet. A PHP + * child started in this window must not inherit that pipe write end. */ + init_result = php_init(0); + + pthread_mutex_lock(&spawn_barrier_mutex); + release_shell_spawn = TRUE; + pthread_cond_broadcast(&spawn_barrier_cond); + pthread_mutex_unlock(&spawn_barrier_mutex); + assert_int_equal(pthread_join(thread, NULL), 0); + + if (opened.fd >= 0) { + pfd.fd = opened.fd; + pfd.events = POLLIN; + while (poll(&pfd, 1, 2000) > 0) { + n = read(opened.fd, output + total, sizeof(output) - 1 - (size_t)total); + if (n == 0) { + reached_eof = TRUE; + break; + } + if (n < 0) break; + total += (int)n; + if ((size_t)total >= sizeof(output) - 1) break; + } + output[total] = '\0'; + } + + if (init_result == TRUE) php_close(0); + if (opened.fd >= 0) nft_pclose(opened.fd); + dup2(saved_stdin, STDIN_FILENO); + dup2(saved_stdout, STDOUT_FILENO); + close(saved_stdin); + close(saved_stdout); + + assert_int_equal(init_result, TRUE); + assert_true(opened.fd >= 0); + assert_non_null(strstr(output, "concurrent-visible")); + assert_true(reached_eof); +} + +static void test_init_marks_an_unexpected_handshake_busy(void **state) { + (void) state; + snprintf(set.path_php_server, sizeof(set.path_php_server), "%s", "bad-start"); + assert_int_equal(php_init(0), TRUE); + assert_int_equal(php_processes[0].php_state, PHP_BUSY); +} + +static void test_busy_handshake_is_recovered_on_the_next_poll(void **state) { + char *result; + int process; + pid_t failed_pid; + + (void) state; + snprintf(set.path_php_server, sizeof(set.path_php_server), "%s", "bad-start"); + assert_int_equal(php_init(0), TRUE); + assert_int_equal(php_processes[0].php_state, PHP_BUSY); + failed_pid = php_processes[0].php_pid; + assert_true(failed_pid > 1); + assert_true(php_processes[0].php_read_fd >= 0); + assert_true(php_processes[0].php_write_fd >= 0); + + snprintf(set.path_php_server, sizeof(set.path_php_server), "%s", "normal"); + process = php_get_process(); + assert_int_equal(process, 0); + assert_true(php_processes[0].php_pid > 1); + assert_int_not_equal(php_processes[0].php_pid, failed_pid); + errno = 0; + assert_int_equal(waitpid(failed_pid, NULL, WNOHANG), -1); + assert_int_equal(errno, ECHILD); + result = php_cmd("poll 7", process); + assert_non_null(result); + assert_string_equal(result, "42\n"); + free(result); +} + +struct php_thread_result { + int process; + char *command_result; +}; + +static int command_finished; + +static void *get_php_process_thread(void *arg) { + struct php_thread_result *result = arg; + result->process = php_get_process(); + return NULL; +} + +static void *run_php_command_thread(void *arg) { + struct php_thread_result *result = arg; + result->command_result = php_cmd("poll 7", 0); + pthread_mutex_lock(&spawn_barrier_mutex); + command_finished = TRUE; + pthread_cond_broadcast(&spawn_barrier_cond); + pthread_mutex_unlock(&spawn_barrier_mutex); + return NULL; +} + +static void test_recovery_and_command_share_the_slot_lock(void **state) { + struct php_thread_result recovery = { .process = -1, .command_result = NULL }; + struct php_thread_result command = { .process = -1, .command_result = NULL }; + pthread_t recovery_thread; + pthread_t command_thread; + (void) state; + + set.php_servers = 1; + block_php_spawn = TRUE; + command_finished = FALSE; + assert_int_equal(pthread_create(&recovery_thread, NULL, get_php_process_thread, &recovery), 0); + + pthread_mutex_lock(&spawn_barrier_mutex); + while (!shell_spawn_reached) { + pthread_cond_wait(&spawn_barrier_cond, &spawn_barrier_mutex); + } + pthread_mutex_unlock(&spawn_barrier_mutex); + + /* A blocked handshake must not retain the process-global round-robin lock. */ + assert_int_equal(thread_mutex_trylock(LOCK_PHP), 0); + thread_mutex_unlock(LOCK_PHP); + + assert_int_equal(pthread_create(&command_thread, NULL, run_php_command_thread, &command), 0); + usleep(20000); + pthread_mutex_lock(&spawn_barrier_mutex); + assert_false(command_finished); + release_shell_spawn = TRUE; + pthread_cond_broadcast(&spawn_barrier_cond); + pthread_mutex_unlock(&spawn_barrier_mutex); + + assert_int_equal(pthread_join(recovery_thread, NULL), 0); + assert_int_equal(pthread_join(command_thread, NULL), 0); + assert_int_equal(recovery.process, 0); + assert_non_null(command.command_result); + assert_string_equal(command.command_result, "42\n"); + free(command.command_result); +} + +static void test_recovery_attempts_only_one_failed_slot_per_call(void **state) { + double started; + double elapsed; + (void) state; + + set.php_servers = 3; + set.script_timeout = 1; + snprintf(set.path_php_server, sizeof(set.path_php_server), "%s", "silent"); + started = get_time_as_double(); + assert_int_equal(php_get_process(), -1); + elapsed = get_time_as_double() - started; + + assert_true(elapsed < 2.5); +} + +static void test_child_exit_during_startup_fails_closed_without_restart(void **state) { + (void) state; + snprintf(set.path_php_server, sizeof(set.path_php_server), "%s", "exit-before-start"); + assert_int_equal(php_init(0), TRUE); + assert_int_equal(php_processes[0].php_state, PHP_BUSY); +} + +static void test_php_init_later_failure_preserves_earlier_server(void **state) { + pid_t first_pid; + int first_read_fd; + int first_write_fd; + (void) state; + + set.php_servers = 2; + fail_php_spawn_call = 2; + assert_int_equal(php_init(PHP_INIT), FALSE); + + first_pid = php_processes[0].php_pid; + first_read_fd = php_processes[0].php_read_fd; + first_write_fd = php_processes[0].php_write_fd; + assert_true(first_pid > 1); + assert_true(first_read_fd >= 0); + assert_true(first_write_fd >= 0); + assert_int_equal(php_processes[0].php_state, PHP_READY); + assert_int_equal(php_processes[0].php_pid, first_pid); + assert_int_equal(php_processes[0].php_read_fd, first_read_fd); + assert_int_equal(php_processes[0].php_write_fd, first_write_fd); + assert_int_equal(php_processes[1].php_pid, -1); + assert_int_equal(php_processes[1].php_read_fd, -1); + assert_int_equal(php_processes[1].php_write_fd, -1); + assert_int_equal(php_processes[1].php_state, PHP_BUSY); +} + +static void assert_stdio_collision_failure_is_clean(int fail_dup, int fail_cloexec) { + int saved_stdin = dup(STDIN_FILENO); + int result; + assert_true(saved_stdin >= 0); + close(STDIN_FILENO); + fail_next_dup = fail_dup; + fail_duplicated_cloexec = fail_cloexec; + result = php_init(0); + dup2(saved_stdin, STDIN_FILENO); + close(saved_stdin); + assert_int_equal(result, FALSE); + assert_int_equal(php_processes[0].php_pid, -1); + assert_int_equal(php_processes[0].php_read_fd, -1); + assert_int_equal(php_processes[0].php_write_fd, -1); +} + +static void test_stdio_collision_dup_failure_is_clean(void **state) { + (void) state; + assert_stdio_collision_failure_is_clean(TRUE, FALSE); +} + +static void test_stdio_collision_cloexec_failure_is_clean(void **state) { + (void) state; + assert_stdio_collision_failure_is_clean(FALSE, TRUE); +} + +static void test_init_timeout_does_not_recurse(void **state) { + (void) state; + set.script_timeout = 0; + snprintf(set.path_php_server, sizeof(set.path_php_server), "%s", "silent"); + assert_int_equal(php_init(0), TRUE); + assert_int_equal(php_processes[0].php_state, PHP_BUSY); +} + +static void test_spawn_failure_releases_every_resource(void **state) { + (void) state; + /* calloc-like zeroes reproduce the process-wide initialization that used to + * let a failed slot masquerade as stdin. php_init() must replace them. */ + memset(&php_processes[0], 0, sizeof(php_processes[0])); + snprintf(set.path_php, sizeof(set.path_php), "%s", "/does/not/exist/spine-php-test"); + assert_int_equal(php_init(0), FALSE); + assert_int_equal(php_processes[0].php_pid, -1); + assert_int_equal(php_processes[0].php_read_fd, -1); + assert_int_equal(php_processes[0].php_write_fd, -1); +} + +static void test_command_rejects_a_writable_poisoned_slot(void **state) { + char *result; + int fd; + + (void) state; + fd = open("/dev/null", O_RDWR); + assert_true(fd >= 0); + memset(&php_processes[0], 0, sizeof(php_processes[0])); + php_processes[0].php_write_fd = fd; + + result = php_cmd("poll 9", 0); + assert_non_null(result); + assert_string_equal(result, "U"); + free(result); + close(fd); + php_processes[0].php_read_fd = php_processes[0].php_write_fd = -1; +} + +static void test_readpipe_rejects_fd_at_fd_setsize(void **state) { + char command[] = "test"; + char *result; + int oversized_fd; + int pdes[2]; + + (void) state; + assert_int_equal(pipe(pdes), 0); + oversized_fd = fcntl(pdes[0], F_DUPFD, FD_SETSIZE); + assert_true(oversized_fd >= FD_SETSIZE); + close(pdes[0]); + close(pdes[1]); + snprintf(set.path_php, sizeof(set.path_php), "%s", "/does/not/exist/spine-php-test"); + php_processes[0].php_state = PHP_READY; + php_processes[0].php_read_fd = oversized_fd; + result = php_readpipe(0, command); + assert_non_null(result); + assert_string_equal(result, "U"); + free(result); + assert_int_not_equal(php_processes[0].php_state, PHP_READY); + assert_int_equal(php_processes[0].php_read_fd, -1); + assert_int_equal(php_get_process(), -1); +} + +static void test_startup_read_rejects_fd_at_fd_setsize_without_restart(void **state) { + char command[] = "INIT"; + char *result; + int oversized_fd; + int pdes[2]; + (void) state; + + assert_int_equal(pipe(pdes), 0); + oversized_fd = fcntl(pdes[0], F_DUPFD, FD_SETSIZE); + assert_true(oversized_fd >= FD_SETSIZE); + close(pdes[0]); + close(pdes[1]); + php_processes[0].php_state = PHP_READY; + php_processes[0].php_read_fd = oversized_fd; + php_processes[0].php_write_fd = -1; + + result = php_read_result_for_test(0, command, FALSE); + assert_non_null(result); + assert_string_equal(result, "U"); + free(result); + assert_int_equal(php_processes[0].php_state, PHP_BUSY); + assert_int_equal(php_processes[0].php_read_fd, oversized_fd); + assert_int_equal(php_spawn_calls, 0); + close(oversized_fd); + php_processes[0].php_read_fd = -1; +} + +static void test_command_retires_fd_at_fd_setsize(void **state) { + char *result; + int oversized_fd; + int pdes[2]; + + (void) state; + assert_int_equal(pipe(pdes), 0); + oversized_fd = fcntl(pdes[0], F_DUPFD, FD_SETSIZE); + assert_true(oversized_fd >= FD_SETSIZE); + close(pdes[0]); + + php_processes[0].php_pid = fork(); + assert_true(php_processes[0].php_pid >= 0); + if (php_processes[0].php_pid == 0) { + pause(); + _exit(0); + } + set.php_servers = 1; + snprintf(set.path_php, sizeof(set.path_php), "%s", "/does/not/exist/spine-php-test"); + php_processes[0].php_state = PHP_READY; + php_processes[0].php_read_fd = oversized_fd; + php_processes[0].php_write_fd = pdes[1]; + + result = php_cmd("poll 9", 0); + assert_non_null(result); + assert_string_equal(result, "U"); + free(result); + assert_int_not_equal(php_processes[0].php_state, PHP_READY); + assert_int_equal(php_processes[0].php_pid, -1); + assert_int_equal(php_processes[0].php_read_fd, -1); + assert_int_equal(php_processes[0].php_write_fd, -1); + assert_int_equal(php_get_process(), -1); +} + +static void test_readpipe_rejects_an_oversized_response(void **state) { + char payload[RESULTS_BUFFER]; + char command[] = "test"; + char *result; + int pdes[2]; + + (void) state; + memset(payload, 'x', sizeof(payload)); + assert_int_equal(pipe(pdes), 0); + assert_int_equal(write(pdes[1], payload, sizeof(payload)), (ssize_t)sizeof(payload)); + close(pdes[1]); + php_processes[0].php_read_fd = pdes[0]; + + result = php_readpipe(0, command); + assert_non_null(result); + assert_string_equal(result, "U"); + free(result); + close(pdes[0]); + php_processes[0].php_read_fd = -1; +} + +static void test_command_gives_up_after_three_failed_writes(void **state) { + struct sigaction saved_sigpipe; + struct sigaction default_sigpipe; + char *result; + int pdes[2]; + + (void) state; + assert_int_equal(pipe(pdes), 0); + close(pdes[0]); + php_processes[0].php_state = PHP_READY; + php_processes[0].php_pid = fork(); + assert_true(php_processes[0].php_pid >= 0); + if (php_processes[0].php_pid == 0) { + pause(); + _exit(0); + } + php_processes[0].php_read_fd = pdes[0]; + php_processes[0].php_write_fd = pdes[1]; + snprintf(set.path_php, sizeof(set.path_php), "%s", "/does/not/exist/spine-php-test"); + assert_int_equal(sigaction(SIGPIPE, NULL, &saved_sigpipe), 0); + memset(&default_sigpipe, 0, sizeof(default_sigpipe)); + default_sigpipe.sa_handler = SIG_DFL; + assert_int_equal(sigaction(SIGPIPE, &default_sigpipe, NULL), 0); + + result = php_cmd("poll 9", 0); + sigaction(SIGPIPE, &saved_sigpipe, NULL); + assert_non_null(result); + assert_string_equal(result, "U"); + free(result); +} + +int main(void) { + const struct CMUnitTest tests[] = { + cmocka_unit_test_setup_teardown(test_round_robin_wraps_at_server_count, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_round_robin_rejects_failed_slots, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_round_robin_returns_a_healthy_slot_when_all_are_contended, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_dead_slot_is_recovered_while_another_slot_is_contended, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_failed_recovery_falls_back_to_the_contended_slot, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_broken_pipe_with_runtime_sigpipe_handler_does_not_block, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_sigmask_failure_prevents_the_pipe_write, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_preexisting_pending_sigpipe_is_preserved, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_spawnattr_sigpipe_failure_destroys_initialized_attr, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_full_script_server_lifecycle, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_php_child_restores_sigpipe_default, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_script_server_lifecycle_with_stdio_closed, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_php_spawn_does_not_inherit_an_nft_collision_descriptor, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_init_marks_an_unexpected_handshake_busy, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_busy_handshake_is_recovered_on_the_next_poll, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_recovery_and_command_share_the_slot_lock, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_recovery_attempts_only_one_failed_slot_per_call, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_child_exit_during_startup_fails_closed_without_restart, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_php_init_later_failure_preserves_earlier_server, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_stdio_collision_dup_failure_is_clean, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_stdio_collision_cloexec_failure_is_clean, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_init_timeout_does_not_recurse, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_spawn_failure_releases_every_resource, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_command_rejects_a_writable_poisoned_slot, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_readpipe_rejects_fd_at_fd_setsize, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_startup_read_rejects_fd_at_fd_setsize_without_restart, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_command_retires_fd_at_fd_setsize, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_readpipe_rejects_an_oversized_response, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_command_gives_up_after_three_failed_writes, php_setup, php_teardown), + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +} From a321c4be90fd6098be5f3ee43cce52f3c6788c28 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sat, 5 Sep 2026 18:24:39 -0700 Subject: [PATCH 06/15] fix(child): initialize spawned SIGPIPE disposition Share the reviewed spawn-attribute helper between nft_popen and the PHP script server without pulling the overlapping nft_pclose ownership rewrite from #597. Signed-off-by: Thomas Vincent --- nft_popen.c | 28 ++++++++++++++++++++++++++++ nft_popen.h | 6 ++++++ 2 files changed, 34 insertions(+) diff --git a/nft_popen.c b/nft_popen.c index a7934566..971c0619 100644 --- a/nft_popen.c +++ b/nft_popen.c @@ -225,6 +225,34 @@ int spine_open_pipe_cloexec(int pdes[2]) { 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 */ diff --git a/nft_popen.h b/nft_popen.h index d53daf08..032f376b 100644 --- a/nft_popen.h +++ b/nft_popen.h @@ -49,6 +49,8 @@ ****************************************************************************** */ +#include + /*! * 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 @@ -115,6 +117,10 @@ extern int spine_set_cloexec(int fd); */ 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 * From bcfd6c7329cb9f34abb4102905fae92cfaadd3ba Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sat, 5 Sep 2026 18:25:31 -0700 Subject: [PATCH 07/15] test: use the platform ICMP unreachable spelling Map the BSD ICMP_UNREACH name to the Linux spelling used by the shared linked-object regression suite. Signed-off-by: Thomas Vincent --- tests/unit/test_linked.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/unit/test_linked.c b/tests/unit/test_linked.c index 59e8e2f8..fb1a6325 100644 --- a/tests/unit/test_linked.c +++ b/tests/unit/test_linked.c @@ -24,6 +24,10 @@ #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 From cffe716a4f328916cbe06f6096f561ef9e315965 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sat, 5 Sep 2026 18:26:23 -0700 Subject: [PATCH 08/15] test(child): align the structural guard with shared helpers Check the shared close-on-exec helper and bounded PHP handshake without pulling the poll_host and ICMP assertions assigned to other replacement branches. Signed-off-by: Thomas Vincent --- tests/regression/test_child_process_safety.sh | 59 ++++++++++++++----- 1 file changed, 43 insertions(+), 16 deletions(-) diff --git a/tests/regression/test_child_process_safety.sh b/tests/regression/test_child_process_safety.sh index 18c5f53c..33f804af 100755 --- a/tests/regression/test_child_process_safety.sh +++ b/tests/regression/test_child_process_safety.sh @@ -1,41 +1,51 @@ #!/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 ' 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" @@ -61,4 +71,21 @@ printf '%s\n' "$php_init_body" | grep -cE '^\s+return FALSE;' | grep -qx '1' || 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 'if (allow_restart) {' || + fail "php_read_result() must gate the server restart on allow_restart" + exit 0 From 6635f6dd312066083ce099358b891673a2a960ba Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 6 Sep 2026 00:30:58 -0700 Subject: [PATCH 09/15] fix(process): preserve close-on-exec across stdio collisions --- nft_popen.c | 28 ++++++++-- nft_popen.h | 4 ++ tests/regression/test_child_process_safety.sh | 2 +- tests/unit/test_linked.c | 40 +++++++++++++ tests/unit/test_php_runtime.c | 56 ++++++++++++++++++- 5 files changed, 124 insertions(+), 6 deletions(-) diff --git a/nft_popen.c b/nft_popen.c index 971c0619..b55530d9 100644 --- a/nft_popen.c +++ b/nft_popen.c @@ -185,6 +185,24 @@ int spine_set_cloexec(int fd) { 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 * @@ -420,8 +438,10 @@ int nft_popen(const char * command, const char * type) { * 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. dup() the end to a fresh descriptor, which - * does not carry the flag, and let the child dup2 from that. */ + * 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) { @@ -430,7 +450,7 @@ int nft_popen(const char * command, const char * type) { if (twoway) posix_spawn_file_actions_adddup2(&fa, STDOUT_FILENO, STDIN_FILENO); } else { - inherit_fd = dup(pdes[1]); + 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))); @@ -448,7 +468,7 @@ int nft_popen(const char * command, const char * type) { posix_spawn_file_actions_adddup2(&fa, pdes[0], STDIN_FILENO); posix_spawn_file_actions_addclose(&fa, pdes[0]); } else { - inherit_fd = dup(pdes[0]); + 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))); diff --git a/nft_popen.h b/nft_popen.h index 032f376b..63f86da5 100644 --- a/nft_popen.h +++ b/nft_popen.h @@ -105,6 +105,10 @@ extern int nft_pclose(int fd); */ 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 * diff --git a/tests/regression/test_child_process_safety.sh b/tests/regression/test_child_process_safety.sh index 33f804af..54382176 100755 --- a/tests/regression/test_child_process_safety.sh +++ b/tests/regression/test_child_process_safety.sh @@ -65,7 +65,7 @@ awk '/^int spine_open_pipe_cloexec/,/^}/' nft_popen.c | # 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 '^\s+return FALSE;' | grep -qx '1' || +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:' || diff --git a/tests/unit/test_linked.c b/tests/unit/test_linked.c index fb1a6325..0e51bd22 100644 --- a/tests/unit/test_linked.c +++ b/tests/unit/test_linked.c @@ -691,6 +691,44 @@ static void test_cloexec_pipe_is_a_working_pipe(void **state) { 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_abandoned_children_are_swept_and_capacity_is_bounded(void **state) { + pid_t pid; + int i; + int status; + + (void) state; + pid = fork(); + assert_true(pid >= 0); + if (pid == 0) { + pause(); + _exit(0); + } + + for (i = 0; i < NFT_ABANDONED_MAX + 1; i++) + nft_abandon_child(pid, "unit test"); + assert_int_equal(nft_abandoned_pending(), NFT_ABANDONED_MAX); + assert_int_equal(kill(pid, SIGKILL), 0); + assert_int_equal(waitpid(pid, &status, 0), pid); + 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. */ @@ -828,10 +866,12 @@ int main(void) { 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_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_abandoned_children_are_swept_and_capacity_is_bounded), }; return cmocka_run_group_tests(tests, NULL, NULL); diff --git a/tests/unit/test_php_runtime.c b/tests/unit/test_php_runtime.c index 7424cebf..fff415c2 100644 --- a/tests/unit/test_php_runtime.c +++ b/tests/unit/test_php_runtime.c @@ -32,6 +32,7 @@ static int shell_spawn_reached; static int release_shell_spawn; static int fail_next_dup; static int fail_duplicated_cloexec; +static int fail_next_cloexec; static __thread int duplicated_fd = -1; static int fail_next_setsigdefault; static int track_spawnattr_destroy; @@ -75,7 +76,9 @@ int __wrap_fcntl(int fd, int command, ...) { va_list args; int argument; - if (fail_duplicated_cloexec && fd == duplicated_fd && command == F_SETFD) { + if ((fail_next_cloexec || (fail_duplicated_cloexec && fd == duplicated_fd)) && + command == F_SETFD) { + fail_next_cloexec = FALSE; fail_duplicated_cloexec = FALSE; errno = EIO; return -1; @@ -157,6 +160,7 @@ static int php_setup(void **state) { release_shell_spawn = FALSE; fail_next_dup = FALSE; fail_duplicated_cloexec = FALSE; + fail_next_cloexec = FALSE; duplicated_fd = -1; fail_next_setsigdefault = FALSE; track_spawnattr_destroy = FALSE; @@ -737,6 +741,53 @@ static void test_stdio_collision_cloexec_failure_is_clean(void **state) { assert_stdio_collision_failure_is_clean(FALSE, TRUE); } +static void assert_nft_collision_failure_is_clean(const char *type, int fail_dup, int fail_cloexec) { + int saved_stdin = dup(STDIN_FILENO); + int saved_stdout = dup(STDOUT_FILENO); + int result; + + assert_true(saved_stdin >= 0); + assert_true(saved_stdout >= 0); + close(STDIN_FILENO); + close(STDOUT_FILENO); + fail_next_dup = fail_dup; + fail_duplicated_cloexec = fail_cloexec; + result = nft_popen("exit 0", type); + dup2(saved_stdin, STDIN_FILENO); + dup2(saved_stdout, STDOUT_FILENO); + close(saved_stdin); + close(saved_stdout); + assert_int_equal(result, -1); +} + +static void test_nft_read_collision_cloexec_failure_is_clean(void **state) { + (void) state; + assert_nft_collision_failure_is_clean("r", FALSE, TRUE); +} + +static void test_nft_write_collision_cloexec_failure_is_clean(void **state) { + (void) state; + assert_nft_collision_failure_is_clean("w", FALSE, TRUE); +} + +static void test_cloexec_pipe_failure_releases_both_descriptors(void **state) { + int pdes[2] = {-2, -2}; + int first_reused; + int second_reused; + + (void) state; + fail_next_cloexec = TRUE; + assert_false(spine_open_pipe_cloexec(pdes)); + assert_int_equal(pdes[0], -1); + assert_int_equal(pdes[1], -1); + first_reused = open("/dev/null", O_RDONLY); + second_reused = open("/dev/null", O_RDONLY); + assert_true(first_reused >= 0); + assert_true(second_reused >= 0); + close(first_reused); + close(second_reused); +} + static void test_init_timeout_does_not_recurse(void **state) { (void) state; set.script_timeout = 0; @@ -935,6 +986,9 @@ int main(void) { cmocka_unit_test_setup_teardown(test_php_init_later_failure_preserves_earlier_server, php_setup, php_teardown), cmocka_unit_test_setup_teardown(test_stdio_collision_dup_failure_is_clean, php_setup, php_teardown), cmocka_unit_test_setup_teardown(test_stdio_collision_cloexec_failure_is_clean, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_nft_read_collision_cloexec_failure_is_clean, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_nft_write_collision_cloexec_failure_is_clean, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_cloexec_pipe_failure_releases_both_descriptors, php_setup, php_teardown), cmocka_unit_test_setup_teardown(test_init_timeout_does_not_recurse, php_setup, php_teardown), cmocka_unit_test_setup_teardown(test_spawn_failure_releases_every_resource, php_setup, php_teardown), cmocka_unit_test_setup_teardown(test_command_rejects_a_writable_poisoned_slot, php_setup, php_teardown), From fb6b3ca93dace538de6c05aef7b775ac6ead905a Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 6 Sep 2026 00:53:54 -0700 Subject: [PATCH 10/15] fix(php): initialize script-server slots as invalid --- error.c | 13 +++++++++---- php.c | 14 ++++++++++++++ php.h | 1 + spine.c | 5 +++-- tests/unit/test_php_runtime.c | 24 ++++++++++++++++-------- 5 files changed, 43 insertions(+), 14 deletions(-) diff --git a/error.c b/error.c index 7c6c5a45..a34f894f 100644 --- a/error.c +++ b/error.c @@ -180,7 +180,9 @@ void install_spine_signal_handler(void) { sa.sa_handler = spine_sigpipe_handler; sigemptyset(&sa.sa_mask); sa.sa_flags = SA_RESTART; - sigaction(SIGPIPE, &sa, NULL); + 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); @@ -212,12 +214,15 @@ void uninstall_spine_signal_handler(void) { struct sigaction sa; void (*ohandler)(int); - sigaction(SIGPIPE, NULL, &sa); - if (sa.sa_handler == spine_sigpipe_handler) { + 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; - sigaction(SIGPIPE, &sa, NULL); + 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) { diff --git a/php.c b/php.c index a595eac4..380c6609 100644 --- a/php.c +++ b/php.c @@ -75,6 +75,20 @@ static void php_process_unlock(int php_process) { thread_mutex_unlock(LOCK_PHP_PROC_0 + 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 diff --git a/php.h b/php.h index 6c94f534..5aa3fa9e 100644 --- a/php.h +++ b/php.h @@ -38,6 +38,7 @@ 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); diff --git a/spine.c b/spine.c index 4ae21ce0..a1a0990f 100644 --- a/spine.c +++ b/spine.c @@ -246,9 +246,10 @@ 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)); diff --git a/tests/unit/test_php_runtime.c b/tests/unit/test_php_runtime.c index fff415c2..7119d7f3 100644 --- a/tests/unit/test_php_runtime.c +++ b/tests/unit/test_php_runtime.c @@ -133,8 +133,6 @@ int __wrap_posix_spawnattr_destroy(posix_spawnattr_t *attr) { } static int php_setup(void **state) { - int i; - (void) state; memset(&set, 0, sizeof(set)); init_mutexes(); @@ -142,12 +140,7 @@ static int php_setup(void **state) { php_processes = calloc(MAX_PHP_SERVERS, sizeof(*php_processes)); assert_non_null(php_processes); - for (i = 0; i < MAX_PHP_SERVERS; i++) { - php_processes[i].php_state = PHP_BUSY; - php_processes[i].php_pid = -1; - php_processes[i].php_read_fd = -1; - php_processes[i].php_write_fd = -1; - } + php_processes_initialize(php_processes, MAX_PHP_SERVERS); set.php_servers = 2; set.script_timeout = 1; @@ -215,6 +208,20 @@ static void test_round_robin_wraps_at_server_count(void **state) { php_processes[1].php_pid = php_processes[1].php_read_fd = php_processes[1].php_write_fd = -1; } +static void test_process_slot_initialization_invalidates_zero_descriptors(void **state) { + php_t slots[3] = {{0}}; + int i; + + (void) state; + php_processes_initialize(slots, 3); + for (i = 0; i < 3; i++) { + assert_int_equal(slots[i].php_state, PHP_BUSY); + assert_int_equal(slots[i].php_pid, -1); + assert_int_equal(slots[i].php_read_fd, -1); + assert_int_equal(slots[i].php_write_fd, -1); + } +} + static void test_round_robin_rejects_failed_slots(void **state) { (void) state; snprintf(set.path_php, sizeof(set.path_php), "%s", "/does/not/exist/spine-php-test"); @@ -966,6 +973,7 @@ static void test_command_gives_up_after_three_failed_writes(void **state) { int main(void) { const struct CMUnitTest tests[] = { cmocka_unit_test_setup_teardown(test_round_robin_wraps_at_server_count, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_process_slot_initialization_invalidates_zero_descriptors, php_setup, php_teardown), cmocka_unit_test_setup_teardown(test_round_robin_rejects_failed_slots, php_setup, php_teardown), cmocka_unit_test_setup_teardown(test_round_robin_returns_a_healthy_slot_when_all_are_contended, php_setup, php_teardown), cmocka_unit_test_setup_teardown(test_dead_slot_is_recovered_while_another_slot_is_contended, php_setup, php_teardown), From c1c0d2f5df145ace3b3e0b78cbdc4957de0e3db8 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 6 Sep 2026 01:05:09 -0700 Subject: [PATCH 11/15] fix(process): preserve current child stdio redirects --- nft_popen.c | 11 +++++++++- tests/unit/test_linked.c | 43 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/nft_popen.c b/nft_popen.c index b55530d9..677b361c 100644 --- a/nft_popen.c +++ b/nft_popen.c @@ -482,8 +482,17 @@ int nft_popen(const char * command, const char * type) { } /* 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__) diff --git a/tests/unit/test_linked.c b/tests/unit/test_linked.c index 0e51bd22..22ae1c63 100644 --- a/tests/unit/test_linked.c +++ b/tests/unit/test_linked.c @@ -708,6 +708,48 @@ static void test_duplicated_descriptor_is_close_on_exec(void **state) { 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 pid; int i; @@ -867,6 +909,7 @@ int main(void) { 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), From 7b440b25a2c240e43b2670c675c395f1fad5cb30 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 6 Sep 2026 01:08:24 -0700 Subject: [PATCH 12/15] fix(php): retain unreaped children for later sweeping --- php.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/php.c b/php.c index 380c6609..029ef892 100644 --- a/php.c +++ b/php.c @@ -778,7 +778,7 @@ int php_init(int php_process) { 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; @@ -796,12 +796,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 @@ -813,7 +813,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) @@ -882,7 +882,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; } From 6e7fcb0b57596f612ddf5dafe245b70f4faf13d2 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 6 Sep 2026 01:24:29 -0700 Subject: [PATCH 13/15] fix(php): retire desynchronized script-server reads --- php.c | 90 +++++++++++-------- tests/regression/test_child_process_safety.sh | 6 +- tests/unit/test_linked.c | 35 +++++--- tests/unit/test_php_runtime.c | 49 ++++++++-- 4 files changed, 124 insertions(+), 56 deletions(-) diff --git a/php.c b/php.c index 029ef892..cc6f4019 100644 --- a/php.c +++ b/php.c @@ -75,6 +75,23 @@ 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; @@ -156,7 +173,7 @@ char *php_cmd(const char *php_command, int php_process) { 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 strdup("U"); + return php_undefined_result(); } /* pad command with CR-LF */ @@ -173,7 +190,7 @@ char *php_cmd(const char *php_command, int php_process) { 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 strdup("U"); + return php_undefined_result(); } /* send command to the script server */ @@ -194,7 +211,7 @@ char *php_cmd(const char *php_command, int php_process) { /* allocated only once the retry budget is spent: a successful retry reassigns result_string below and would orphan an earlier copy */ - result_string = strdup("U"); + result_string = php_undefined_result(); } else { /* read the result from the php_command */ result_string = php_readpipe(php_process, command); @@ -364,11 +381,7 @@ static char *php_read_result(int php_process, char *command, int allow_restart) * 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_processes[php_process].php_state = PHP_BUSY; - if (allow_restart) { - php_close(php_process); - php_init(php_process); - } + php_fail_read(php_process, allow_restart); return result_string; } @@ -420,28 +433,22 @@ static char *php_read_result(int php_process, char *command, int allow_restart) break; } - SET_UNDEFINED(result_string); + SET_UNDEFINED(result_string); + php_fail_read(php_process, allow_restart); - /* kill script server because it is misbehaving */ - if (allow_restart) { - php_close(php_process); - php_init(php_process); - } - break; + 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); + SET_UNDEFINED(result_string); + php_fail_read(php_process, allow_restart); + break; + default: + { + int read_ok = TRUE; - /* kill script server because it is misbehaving */ - if (allow_restart) { - php_close(php_process); - php_init(php_process); - } - break; - default: - if (FD_ISSET(php_processes[php_process].php_read_fd, &fds)) { + if (FD_ISSET(php_processes[php_process].php_read_fd, &fds)) { bptr = result_string; while (1) { @@ -449,17 +456,19 @@ static char *php_read_result(int php_process, char *command, int allow_restart) size_t used = (size_t)(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; + 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; } size_t space = (size_t)RESULTS_BUFFER - 1 - used; i = read(php_processes[php_process].php_read_fd, bptr, space); - if (i <= 0) { - SET_UNDEFINED(result_string); - break; + if (i <= 0) { + SET_UNDEFINED(result_string); + read_ok = FALSE; + break; } bptr += i; @@ -470,17 +479,24 @@ static char *php_read_result(int php_process, char *command, int allow_restart) } 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; + 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); - } + SPINE_LOG(("ERROR: SS[%i] The FD was not set as expected", php_process)); + SET_UNDEFINED(result_string); + read_ok = FALSE; + } - 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); + } + } } return result_string; diff --git a/tests/regression/test_child_process_safety.sh b/tests/regression/test_child_process_safety.sh index 54382176..4e3918f3 100755 --- a/tests/regression/test_child_process_safety.sh +++ b/tests/regression/test_child_process_safety.sh @@ -85,7 +85,11 @@ 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_read_result() must gate the server restart on allow_restart" + fail "php_fail_read() must gate the server restart on allow_restart" exit 0 diff --git a/tests/unit/test_linked.c b/tests/unit/test_linked.c index 22ae1c63..7a3d4af2 100644 --- a/tests/unit/test_linked.c +++ b/tests/unit/test_linked.c @@ -751,23 +751,38 @@ static void test_existing_pipe_on_stdout_does_not_close_a_new_child_redirect(voi } static void test_abandoned_children_are_swept_and_capacity_is_bounded(void **state) { - pid_t pid; + pid_t pids[NFT_ABANDONED_MAX + 1]; int i; + int created = 0; int status; (void) state; - pid = fork(); - assert_true(pid >= 0); - if (pid == 0) { - pause(); - _exit(0); + 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(); } - for (i = 0; i < NFT_ABANDONED_MAX + 1; i++) - nft_abandon_child(pid, "unit test"); assert_int_equal(nft_abandoned_pending(), NFT_ABANDONED_MAX); - assert_int_equal(kill(pid, SIGKILL), 0); - assert_int_equal(waitpid(pid, &status, 0), pid); + 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); } diff --git a/tests/unit/test_php_runtime.c b/tests/unit/test_php_runtime.c index 7119d7f3..d9222d08 100644 --- a/tests/unit/test_php_runtime.c +++ b/tests/unit/test_php_runtime.c @@ -17,6 +17,7 @@ #include #include #include +#include #include #include @@ -42,6 +43,32 @@ static int fail_php_spawn_call; static int fail_next_sigmask; static int track_write; static int write_calls; +static struct rlimit saved_nofile; +static int restore_nofile; + +static int duplicate_at_fdsetsize(int fd) { + struct rlimit limit; + struct rlimit raised; + int duplicate; + + if (getrlimit(RLIMIT_NOFILE, &limit) == 0 && limit.rlim_cur <= FD_SETSIZE && + limit.rlim_max > FD_SETSIZE) { + raised = limit; + raised.rlim_cur = FD_SETSIZE + 1; + if (raised.rlim_cur > raised.rlim_max) + raised.rlim_cur = raised.rlim_max; + if (setrlimit(RLIMIT_NOFILE, &raised) == 0) { + saved_nofile = limit; + restore_nofile = TRUE; + } + } + + duplicate = fcntl(fd, F_DUPFD, FD_SETSIZE); + if (duplicate < FD_SETSIZE) + skip(); + + return duplicate; +} int __real_pthread_sigmask(int how, const sigset_t *set, sigset_t *oldset); int __wrap_pthread_sigmask(int how, const sigset_t *set, sigset_t *oldset) { @@ -163,6 +190,7 @@ static int php_setup(void **state) { fail_next_sigmask = FALSE; track_write = FALSE; write_calls = 0; + restore_nofile = FALSE; snprintf(set.path_php, sizeof(set.path_php), "%s", PHP_TEST_SERVER_PATH); snprintf(set.path_php_server, sizeof(set.path_php_server), "%s", "normal"); return 0; @@ -187,6 +215,10 @@ static int php_teardown(void **state) { free(php_processes); php_processes = NULL; } + if (restore_nofile) { + (void)setrlimit(RLIMIT_NOFILE, &saved_nofile); + restore_nofile = FALSE; + } return 0; } @@ -841,8 +873,7 @@ static void test_readpipe_rejects_fd_at_fd_setsize(void **state) { (void) state; assert_int_equal(pipe(pdes), 0); - oversized_fd = fcntl(pdes[0], F_DUPFD, FD_SETSIZE); - assert_true(oversized_fd >= FD_SETSIZE); + oversized_fd = duplicate_at_fdsetsize(pdes[0]); close(pdes[0]); close(pdes[1]); snprintf(set.path_php, sizeof(set.path_php), "%s", "/does/not/exist/spine-php-test"); @@ -865,8 +896,7 @@ static void test_startup_read_rejects_fd_at_fd_setsize_without_restart(void **st (void) state; assert_int_equal(pipe(pdes), 0); - oversized_fd = fcntl(pdes[0], F_DUPFD, FD_SETSIZE); - assert_true(oversized_fd >= FD_SETSIZE); + oversized_fd = duplicate_at_fdsetsize(pdes[0]); close(pdes[0]); close(pdes[1]); php_processes[0].php_state = PHP_READY; @@ -891,8 +921,7 @@ static void test_command_retires_fd_at_fd_setsize(void **state) { (void) state; assert_int_equal(pipe(pdes), 0); - oversized_fd = fcntl(pdes[0], F_DUPFD, FD_SETSIZE); - assert_true(oversized_fd >= FD_SETSIZE); + oversized_fd = duplicate_at_fdsetsize(pdes[0]); close(pdes[0]); php_processes[0].php_pid = fork(); @@ -935,8 +964,12 @@ static void test_readpipe_rejects_an_oversized_response(void **state) { assert_non_null(result); assert_string_equal(result, "U"); free(result); - close(pdes[0]); - php_processes[0].php_read_fd = -1; + assert_int_equal(php_processes[0].php_state, PHP_READY); + assert_true(php_processes[0].php_pid > 1); + result = php_cmd("poll 7", 0); + assert_non_null(result); + assert_string_equal(result, "42\n"); + free(result); } static void test_command_gives_up_after_three_failed_writes(void **state) { From aba501b409313cb0d4abb0d73d3781b563a793f8 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 6 Sep 2026 01:41:35 -0700 Subject: [PATCH 14/15] fix(process): revalidate restarts and terminate gracefully --- nft_popen.c | 19 ++++++--- php.c | 78 +++++++++++++++++++---------------- spine.c | 3 ++ tests/unit/test_linked.c | 16 +++++++ tests/unit/test_php_runtime.c | 52 +++++++++++++++++++++++ 5 files changed, 126 insertions(+), 42 deletions(-) diff --git a/nft_popen.c b/nft_popen.c index 677b361c..75f0e81d 100644 --- a/nft_popen.c +++ b/nft_popen.c @@ -646,18 +646,25 @@ nft_pclose(int fd) pthread_setcancelstate(cancel_state, NULL); - switch (spine_reap_child_bounded(cur->pid, &pstat, NFT_PCLOSE_TERM_ATTEMPTS)) { + /* 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, SIGKILL); - if (spine_reap_child_bounded(cur->pid, &pstat, NFT_PCLOSE_KILL_ATTEMPTS) == 0) { + (void)kill(cur->pid, SIGTERM); + if (spine_reap_child_bounded(cur->pid, &pstat, NFT_PCLOSE_TERM_ATTEMPTS) == 0) { pid = cur->pid; } else { - nft_abandon_child(cur->pid, "kill budget expired"); - errno = ETIMEDOUT; - pid = -1; + (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: diff --git a/php.c b/php.c index cc6f4019..4a0520bf 100644 --- a/php.c +++ b/php.c @@ -181,9 +181,12 @@ char *php_cmd(const char *php_command, int php_process) { php_process_lock(php_process); - /* Validate under the same per-slot lock that protects close/restart. A + 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. */ + * 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 || @@ -194,7 +197,6 @@ char *php_cmd(const char *php_command, int php_process) { } /* send command to the script server */ - retry: 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 */ @@ -202,10 +204,9 @@ char *php_cmd(const char *php_command, int php_process) { 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; } @@ -350,6 +351,7 @@ static char *php_read_result(int php_process, char *command, int allow_restart) double end_time = 0; double remaining_usec = 0; char *result_string; + int response_timeout; ssize_t i; char *cp; @@ -364,7 +366,11 @@ static char *php_read_result(int php_process, char *command, int allow_restart) 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 @@ -406,8 +412,8 @@ static char *php_read_result(int php_process, char *command, int allow_restart) 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); @@ -433,59 +439,58 @@ static char *php_read_result(int php_process, char *command, int allow_restart) break; } - SET_UNDEFINED(result_string); - php_fail_read(php_process, allow_restart); - - break; + SET_UNDEFINED(result_string); + 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); - php_fail_read(php_process, allow_restart); - break; - default: - { + SET_UNDEFINED(result_string); + php_fail_read(php_process, allow_restart); + break; + default: + { int read_ok = TRUE; if (FD_ISSET(php_processes[php_process].php_read_fd, &fds)) { - bptr = result_string; + bptr = result_string; - while (1) { - /* reserve one byte for the trailing '\0' written below */ - size_t used = (size_t)(bptr - result_string); + while (1) { + /* reserve one byte for the trailing '\0' written below */ + size_t used = (size_t)(bptr - result_string); - if (used >= RESULTS_BUFFER - 1) { + 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; - } + } - size_t space = (size_t)RESULTS_BUFFER - 1 - used; - i = read(php_processes[php_process].php_read_fd, bptr, space); + size_t space = (size_t)RESULTS_BUFFER - 1 - used; + i = read(php_processes[php_process].php_read_fd, bptr, space); if (i <= 0) { SET_UNDEFINED(result_string); read_ok = FALSE; break; - } + } - bptr += i; - *bptr = '\0'; /* make what we've got into a string */ + bptr += i; + *bptr = '\0'; /* make what we've got into a string */ - if ((cp = strstr(result_string,"\n")) != 0) { - break; - } + if ((cp = strstr(result_string,"\n")) != 0) { + break; + } - if (bptr >= result_string + RESULTS_BUFFER - 1) { + 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 { + } else { SPINE_LOG(("ERROR: SS[%i] The FD was not set as expected", php_process)); SET_UNDEFINED(result_string); read_ok = FALSE; @@ -496,7 +501,8 @@ static char *php_read_result(int php_process, char *command, int allow_restart) } else { php_fail_read(php_process, allow_restart); } - } + } + break; } return result_string; diff --git a/spine.c b/spine.c index a1a0990f..e3ea5e24 100644 --- a/spine.c +++ b/spine.c @@ -253,6 +253,9 @@ int main(int argc, char *argv[]) { /* 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; diff --git a/tests/unit/test_linked.c b/tests/unit/test_linked.c index 7a3d4af2..d3741346 100644 --- a/tests/unit/test_linked.c +++ b/tests/unit/test_linked.c @@ -878,6 +878,21 @@ static void test_reap_reports_an_already_reaped_child(void **state) { 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[] = { @@ -929,6 +944,7 @@ int main(void) { 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), }; diff --git a/tests/unit/test_php_runtime.c b/tests/unit/test_php_runtime.c index d9222d08..c3b4ba2b 100644 --- a/tests/unit/test_php_runtime.c +++ b/tests/unit/test_php_runtime.c @@ -972,6 +972,56 @@ static void test_readpipe_rejects_an_oversized_response(void **state) { free(result); } +static void prepare_broken_ready_slot(void) { + int pdes[2]; + + assert_int_equal(pipe(pdes), 0); + close(pdes[0]); + php_processes[0].php_pid = fork(); + assert_true(php_processes[0].php_pid >= 0); + if (php_processes[0].php_pid == 0) { + pause(); + _exit(0); + } + php_processes[0].php_state = PHP_READY; + php_processes[0].php_read_fd = open("/dev/null", O_RDONLY); + assert_true(php_processes[0].php_read_fd >= 0); + php_processes[0].php_write_fd = pdes[1]; + track_write = TRUE; +} + +static void test_failed_write_stops_when_restart_handshake_is_not_ready(void **state) { + char *result; + + (void) state; + prepare_broken_ready_slot(); + snprintf(set.path_php_server, sizeof(set.path_php_server), "%s", "bad-start"); + result = php_cmd("poll 9", 0); + track_write = FALSE; + assert_non_null(result); + assert_string_equal(result, "U"); + free(result); + assert_int_equal(php_spawn_calls, 1); + assert_int_equal(write_calls, 2); + assert_int_equal(php_processes[0].php_state, PHP_BUSY); +} + +static void test_failed_write_stops_when_restart_spawn_fails(void **state) { + char *result; + + (void) state; + prepare_broken_ready_slot(); + fail_php_spawn_call = 1; + result = php_cmd("poll 9", 0); + track_write = FALSE; + assert_non_null(result); + assert_string_equal(result, "U"); + free(result); + assert_int_equal(php_spawn_calls, 1); + assert_int_equal(write_calls, 2); + assert_int_equal(php_processes[0].php_pid, -1); +} + static void test_command_gives_up_after_three_failed_writes(void **state) { struct sigaction saved_sigpipe; struct sigaction default_sigpipe; @@ -1037,6 +1087,8 @@ int main(void) { cmocka_unit_test_setup_teardown(test_startup_read_rejects_fd_at_fd_setsize_without_restart, php_setup, php_teardown), cmocka_unit_test_setup_teardown(test_command_retires_fd_at_fd_setsize, php_setup, php_teardown), cmocka_unit_test_setup_teardown(test_readpipe_rejects_an_oversized_response, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_failed_write_stops_when_restart_handshake_is_not_ready, php_setup, php_teardown), + cmocka_unit_test_setup_teardown(test_failed_write_stops_when_restart_spawn_fails, php_setup, php_teardown), cmocka_unit_test_setup_teardown(test_command_gives_up_after_three_failed_writes, php_setup, php_teardown), }; From 79d7b5153e4872c40312c2fe0c29174ab0c8566d Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 6 Sep 2026 02:44:03 -0700 Subject: [PATCH 15/15] test(process): exercise real PHP server shutdown Signed-off-by: Thomas Vincent --- .github/workflows/integration.yml | 5 +- Dockerfile | 2 + tests/integration/test_script_server_reap.sh | 48 ++++++++++++-------- tests/snmpv3/cacti/script_server.php | 26 +++++++++++ tests/snmpv3/docker-compose.yml | 1 + 5 files changed, 63 insertions(+), 19 deletions(-) create mode 100644 tests/snmpv3/cacti/script_server.php diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 1ad5bd82..8bf8baef 100644 --- a/.github/workflows/integration.yml +++ b/.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: | diff --git a/Dockerfile b/Dockerfile index d373ff8e..afb12ef2 100644 --- a/Dockerfile +++ b/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/* diff --git a/tests/integration/test_script_server_reap.sh b/tests/integration/test_script_server_reap.sh index 112c36a1..b24bf7e5 100755 --- a/tests/integration/test_script_server_reap.sh +++ b/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 # --------------------------------------------------------------------------- diff --git a/tests/snmpv3/cacti/script_server.php b/tests/snmpv3/cacti/script_server.php new file mode 100644 index 00000000..049988e7 --- /dev/null +++ b/tests/snmpv3/cacti/script_server.php @@ -0,0 +1,26 @@ +