diff --git a/nft_popen.c b/nft_popen.c index d4d2fd1a..edb4f0c0 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 @@ -101,6 +103,92 @@ static pthread_mutex_t ListMutex = PTHREAD_MUTEX_INITIALIZER; static void close_cleanup(void *); +/* nft_pclose() must not block a poller thread indefinitely. A script that + writes its value and then lingers, or that ignores SIGPIPE, would otherwise + pin the thread across polling cycles while holding its available_scripts + token. Poll with WNOHANG, then escalate to SIGKILL. */ +#define NFT_PCLOSE_REAP_USEC 50000 +#define NFT_PCLOSE_TERM_ATTEMPTS 100 +#define NFT_PCLOSE_KILL_ATTEMPTS 20 + +int spine_set_cloexec(int fd) { + int flags; + + flags = fcntl(fd, F_GETFD); + if (flags < 0) { + return -1; + } + + return fcntl(fd, F_SETFD, flags | FD_CLOEXEC); +} + +/*! \fn static int open_pipe_cloexec(int pdes[2]) + * \brief open a pipe whose descriptors are not inherited across exec + * + * nft_popen() creates the pipe before taking ListMutex, so a second thread + * can spawn while these descriptors are live. Without close-on-exec that + * child holds the first thread's write end, the first thread never sees EOF, + * and it blocks to script_timeout for a data source that answered. + * + * pipe2(pdes, O_CLOEXEC) would set the flag atomically, but it needs + * _GNU_SOURCE on glibc and spine defines no feature macro, so the fcntl() + * pair stays. It leaves a window between the two calls, which is narrower + * than none. + * + * \return TRUE on success, FALSE with the descriptors closed on failure + */ +int spine_open_pipe_cloexec(int pdes[2]) { + if (pipe(pdes) < 0) { + return FALSE; + } + + if (spine_set_cloexec(pdes[0]) != 0 || spine_set_cloexec(pdes[1]) != 0) { + (void)close(pdes[0]); + (void)close(pdes[1]); + return FALSE; + } + + return TRUE; +} + +/*! \fn static int reap_child_bounded(pid_t pid, int *pstat, int attempts) + * \return 0 when reaped, 1 when still running after attempts, -1 on error + */ +int spine_reap_child_bounded(pid_t pid, int *pstat, int attempts) { + int attempt; + pid_t waited; + + for (attempt = 0; attempt < attempts; attempt++) { + do { + waited = waitpid(pid, pstat, WNOHANG); + } while (waited < 0 && errno == EINTR); + + if (waited == pid) { + return 0; + } + + if (waited < 0 && errno == ECHILD) { + /* someone else reaped it, so no status is available */ + *pstat = 0; + return 0; + } + + if (waited < 0) { + return -1; + } + + /* The delay is load-bearing: without it the attempts are spent in + nanoseconds and SIGKILL lands before the child can exit. */ + #ifndef SOLAR_THREAD + usleep(NFT_PCLOSE_REAP_USEC); + #else + sleep(1); + #endif + } + + return 1; +} + /*! ------------------------------------------------------------------------------ * * nft_popen @@ -154,7 +242,7 @@ int nft_popen(const char * command, const char * type) { } } - if (pipe(pdes) < 0) + if (!spine_open_pipe_cloexec(pdes)) return -1; /* Disable thread cancellation from this point forward. */ @@ -356,8 +444,23 @@ nft_pclose(int fd) cur->fd = -1; /* Prevent the fd being closed twice. */ - do { pid = waitpid(cur->pid, &pstat, 0); - } while (pid == -1 && errno == EINTR); + switch (spine_reap_child_bounded(cur->pid, &pstat, NFT_PCLOSE_TERM_ATTEMPTS)) { + case 0: + pid = cur->pid; + break; + case 1: + (void)kill(cur->pid, SIGKILL); + if (spine_reap_child_bounded(cur->pid, &pstat, NFT_PCLOSE_KILL_ATTEMPTS) == 0) { + pid = cur->pid; + } else { + errno = ETIMEDOUT; + pid = -1; + } + break; + default: + pid = -1; + break; + } pthread_cleanup_pop(1); /* Execute the cleanup handler. */ 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 71371452..700b89d2 100644 --- a/tests/unit/test_linked.c +++ b/tests/unit/test_linked.c @@ -19,6 +19,12 @@ #include "spine.h" #include "util.h" #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; @@ -457,7 +463,143 @@ static void test_is_debug_device_matches_only_listed_ids(void **state) { debug_devices = saved; } +/* --------------------------------------------------------------------------- + * Child process hardening (nft_popen.c) + * + * PR #542 removed the close-on-exec and bounded-reap code PR #557 had just + * added, and nothing failed, because the only guard was a shell script that + * grepped the source and was deleted in the same commit. These exercise the + * behaviour against the shipped object instead. + * ------------------------------------------------------------------------- */ + +static void test_cloexec_is_set_on_both_pipe_ends(void **state) { + int pdes[2]; + int i; + + (void) state; + + assert_true(spine_open_pipe_cloexec(pdes)); + + for (i = 0; i < 2; i++) { + int flags = fcntl(pdes[i], F_GETFD); + + assert_true(flags >= 0); + assert_true((flags & FD_CLOEXEC) != 0); + } + + close(pdes[0]); + close(pdes[1]); +} + +static void test_cloexec_pipe_is_a_working_pipe(void **state) { + int pdes[2]; + char buf[8]; + + (void) state; + + assert_true(spine_open_pipe_cloexec(pdes)); + assert_int_equal(write(pdes[1], "ok", 2), 2); + assert_int_equal(read(pdes[0], buf, sizeof(buf)), 2); + assert_memory_equal(buf, "ok", 2); + + close(pdes[0]); + close(pdes[1]); +} + +/* The descriptor must not survive an exec. A child that inherits the write end + keeps the pipe open, so the polling thread never sees EOF and blocks to + script_timeout for a data source that already answered. */ +static void test_pipe_is_not_inherited_across_exec(void **state) { + int pdes[2]; + int status; + pid_t pid; + char fdarg[32]; + + (void) state; + + assert_true(spine_open_pipe_cloexec(pdes)); + snprintf(fdarg, sizeof(fdarg), "/proc/self/fd/%d", pdes[1]); + + pid = fork(); + assert_true(pid >= 0); + + if (pid == 0) { + /* exits 0 when the descriptor survived exec, 1 when it did not */ + execl("/bin/sh", "sh", "-c", "test -e \"$0\"", fdarg, (char *) NULL); + _exit(127); + } + + assert_int_equal(waitpid(pid, &status, 0), pid); + assert_true(WIFEXITED(status)); + assert_int_equal(WEXITSTATUS(status), 1); + + close(pdes[0]); + close(pdes[1]); +} + +static void test_reap_returns_still_running_rather_than_blocking(void **state) { + int pstat = 0; + int status; + pid_t pid; + + (void) state; + + pid = fork(); + assert_true(pid >= 0); + + if (pid == 0) { + pause(); + _exit(0); + } + + /* the shipped code blocked here forever; two attempts must come back */ + assert_int_equal(spine_reap_child_bounded(pid, &pstat, 2), 1); + + assert_int_equal(kill(pid, SIGKILL), 0); + assert_int_equal(waitpid(pid, &status, 0), pid); +} + +static void test_reap_collects_an_exited_child(void **state) { + int pstat = 0; + pid_t pid; + + (void) state; + + pid = fork(); + assert_true(pid >= 0); + + if (pid == 0) { + _exit(3); + } + + assert_int_equal(spine_reap_child_bounded(pid, &pstat, 20), 0); + assert_true(WIFEXITED(pstat)); + assert_int_equal(WEXITSTATUS(pstat), 3); +} + +static void test_reap_reports_an_already_reaped_child(void **state) { + int pstat = 99; + int status; + pid_t pid; + + (void) state; + + pid = fork(); + assert_true(pid >= 0); + + if (pid == 0) { + _exit(0); + } + + assert_int_equal(waitpid(pid, &status, 0), pid); + + /* ECHILD: someone else took the status, which is success with none */ + assert_int_equal(spine_reap_child_bounded(pid, &pstat, 2), 0); + assert_int_equal(pstat, 0); +} + int main(void) { + const struct CMUnitTest tests[] = { cmocka_unit_test(test_strncopy_truncates_within_the_buffer), cmocka_unit_test(test_strncopy_copies_a_short_source_whole), @@ -496,6 +638,12 @@ int main(void) { cmocka_unit_test(test_get_date_format_clamps_an_out_of_range_format), cmocka_unit_test(test_get_date_format_covers_each_supported_format), cmocka_unit_test(test_is_debug_device_matches_only_listed_ids), + cmocka_unit_test(test_cloexec_is_set_on_both_pipe_ends), + cmocka_unit_test(test_cloexec_pipe_is_a_working_pipe), + cmocka_unit_test(test_pipe_is_not_inherited_across_exec), + cmocka_unit_test(test_reap_returns_still_running_rather_than_blocking), + cmocka_unit_test(test_reap_collects_an_exited_child), + cmocka_unit_test(test_reap_reports_an_already_reaped_child), }; return cmocka_run_group_tests(tests, NULL, NULL);