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);