From d5df5fb5b8f5e2815405a76ced4b7407988af0d6 Mon Sep 17 00:00:00 2001 From: Matthias Schiffer Date: Sat, 31 May 2014 08:09:36 +0200 Subject: Fix poll race condition on *BSD --- src/async.c | 5 +++- src/async.h | 1 + src/fastd.c | 83 ++++++++++++++++++++++++++++++++----------------------------- src/fastd.h | 10 -------- src/poll.c | 17 +++---------- 5 files changed, 52 insertions(+), 64 deletions(-) diff --git a/src/async.c b/src/async.c index c1c4802..4a2de10 100644 --- a/src/async.c +++ b/src/async.c @@ -114,6 +114,9 @@ void fastd_async_handle(void) { exit_errno("fastd_async_handle: recvmsg"); switch (header.type) { + case ASYNC_TYPE_NOP: + break; + case ASYNC_TYPE_RESOLVE_RETURN: handle_resolve_return((const fastd_async_resolve_return_t *)buf); break; @@ -143,7 +146,7 @@ void fastd_async_enqueue(fastd_async_type_t type, const void *data, size_t len) }; struct msghdr msg = { .msg_iov = vec, - .msg_iovlen = 2, + .msg_iovlen = len ? 2 : 1, }; if (sendmsg(ctx.async_wfd, &msg, 0) < 0) diff --git a/src/async.h b/src/async.h index e3298d4..15fd737 100644 --- a/src/async.h +++ b/src/async.h @@ -38,6 +38,7 @@ /** A type of asynchronous notification */ typedef enum fastd_async_type { + ASYNC_TYPE_NOP, /**< Does nothing (is used to ensure poll returns quickly after a signal has occurred) */ ASYNC_TYPE_RESOLVE_RETURN, /**< A DNS resolver response */ ASYNC_TYPE_VERIFY_RETURN, /**< A on-verify return */ } fastd_async_type_t; diff --git a/src/fastd.c b/src/fastd.c index c8f8e54..0797ad8 100644 --- a/src/fastd.c +++ b/src/fastd.c @@ -30,7 +30,6 @@ */ - #include "fastd.h" #include "async.h" #include "config.h" @@ -66,30 +65,41 @@ fastd_context_t ctx = {}; -volatile bool fastd_sig_hup = false; /**< Is set to true when a SIGHUP is received */ -volatile bool fastd_sig_terminate = false; /**< Is set to true when a SIGTERM, SIGQUIT or SIGINT is received */ -volatile bool fastd_sig_dump = false; /**< Is set to true when a SIGUSR1 is received */ -volatile bool fastd_sig_chld = false; /**< Is set to true when a SIGCHLD is received */ +static volatile bool sighup = false; /**< Is set to true when a SIGHUP is received */ +static volatile bool terminate = false; /**< Is set to true when a SIGTERM, SIGQUIT or SIGINT is received */ +static volatile bool dump = false; /**< Is set to true when a SIGUSR1 is received */ +static volatile bool sigchld = false; /**< Is set to true when a SIGCHLD is received */ -/** SIGHUP handler */ -static void on_sighup(int signo UNUSED) { - fastd_sig_hup = true; -} +/** Signal handler; just saves the signals to be handled later */ +static void on_signal(int signo) { + switch(signo) { + case SIGHUP: + sighup = true; + break; -/** SIGTERM, SIGQUIT and SIGINT handler */ -static void on_terminate(int signo UNUSED) { - fastd_sig_terminate = true; -} + case SIGUSR1: + dump = true; + break; -/** SIGUSR1 handler */ -static void on_sigusr1(int signo UNUSED) { - fastd_sig_dump = true; -} + case SIGCHLD: + sigchld = true; + break; + + case SIGTERM: + case SIGQUIT: + case SIGINT: + terminate = true; + break; -/** SIGCHLD handler */ -static void on_sigchld(int signo UNUSED) { - fastd_sig_chld = true; + default: + return; + } + +#ifndef USE_EPOLL + /* Avoids a race condition between pthread_sigmask() and poll() (FreeBSD doesn't have ppoll() ...) */ + fastd_async_enqueue(ASYNC_TYPE_NOP, NULL, 0); +#endif } /** Installs signal handlers */ @@ -99,15 +109,16 @@ static void init_signals(void) { /* block all signals */ pthread_sigmask(SIG_SETMASK, &set, NULL); - struct sigaction action; - action.sa_flags = 0; + struct sigaction action = {}; sigemptyset(&action.sa_mask); - action.sa_handler = on_sighup; + action.sa_handler = on_signal; if (sigaction(SIGHUP, &action, NULL)) exit_errno("sigaction"); - - action.sa_handler = on_terminate; + if (sigaction(SIGUSR1, &action, NULL)) + exit_errno("sigaction"); + if (sigaction(SIGCHLD, &action, NULL)) + exit_errno("sigaction"); if (sigaction(SIGTERM, &action, NULL)) exit_errno("sigaction"); if (sigaction(SIGQUIT, &action, NULL)) @@ -115,14 +126,6 @@ static void init_signals(void) { if (sigaction(SIGINT, &action, NULL)) exit_errno("sigaction"); - action.sa_handler = on_sigusr1; - if (sigaction(SIGUSR1, &action, NULL)) - exit_errno("sigaction"); - - action.sa_handler = on_sigchld; - if (sigaction(SIGCHLD, &action, NULL)) - exit_errno("sigaction"); - action.sa_handler = SIG_IGN; if (sigaction(SIGPIPE, &action, NULL)) exit_errno("sigaction"); @@ -625,8 +628,8 @@ static inline void reap_zombies(void) { /** The \em real signal handlers */ static inline void handle_signals(void) { - if (fastd_sig_hup) { - fastd_sig_hup = false; + if (sighup) { + sighup = false; pr_info("reconfigure triggered"); @@ -634,13 +637,13 @@ static inline void handle_signals(void) { init_peers(); } - if (fastd_sig_dump) { - fastd_sig_dump = false; + if (dump) { + dump = false; dump_state(); } - if (fastd_sig_chld) { - fastd_sig_chld = false; + if (sigchld) { + sigchld = false; reap_zombies(); } } @@ -699,7 +702,7 @@ static inline void cleanup(void) { int main(int argc, char *argv[]) { init(argc, argv); - while (!fastd_sig_terminate) + while (!terminate) run(); cleanup(); diff --git a/src/fastd.h b/src/fastd.h index eeb2d3d..f69f2f8 100644 --- a/src/fastd.h +++ b/src/fastd.h @@ -292,11 +292,6 @@ struct fastd_string_stack { extern fastd_context_t ctx; extern fastd_config_t conf; -extern volatile bool fastd_sig_hup; -extern volatile bool fastd_sig_terminate; -extern volatile bool fastd_sig_dump; -extern volatile bool fastd_sig_chld; - void fastd_send(const fastd_socket_t *sock, const fastd_peer_address_t *local_addr, const fastd_peer_address_t *remote_addr, fastd_peer_t *peer, fastd_buffer_t buffer, size_t stat_size); void fastd_send_handshake(const fastd_socket_t *sock, const fastd_peer_address_t *local_addr, const fastd_peer_address_t *remote_addr, fastd_peer_t *peer, fastd_buffer_t buffer); @@ -376,11 +371,6 @@ static inline size_t alignto(size_t l, size_t a) { return block_count(l, a)*a; } -/** Returns true if any unhandled signal in currently queued */ -static inline bool fastd_signalled(void) { - return (fastd_sig_hup || fastd_sig_terminate || fastd_sig_dump || fastd_sig_chld); -} - /** Returns the maximum payload size \em fastd is configured to transport */ static inline size_t fastd_max_payload(void) { switch (conf.mode) { diff --git a/src/poll.c b/src/poll.c index a74d249..3756162 100644 --- a/src/poll.c +++ b/src/poll.c @@ -139,7 +139,7 @@ void fastd_poll_handle(void) { struct epoll_event events[16]; int ret = epoll_pwait(ctx.epoll_fd, events, 16, timeout, &set); if (ret < 0 && errno != EINTR) - exit_errno("epoll_wait"); + exit_errno("epoll_pwait"); fastd_update_time(); @@ -253,18 +253,9 @@ void fastd_poll_handle(void) { sigemptyset(&set); pthread_sigmask(SIG_SETMASK, &set, &oldset); - int ret = -1; - - if (!fastd_signalled()) { - /* - There is a race condition here: if a signal occurs after the fastd_signalled() check, but before the poll - call, poll() will be called with its normal timeout, potentially delaying the actual signal handling. On - OpenBSD we could use ppoll() to fix this, but on FreeBSD we're out of luck. - */ - ret = poll(VECTOR_DATA(ctx.pollfds), VECTOR_LEN(ctx.pollfds), timeout); - if (ret < 0 && errno != EINTR) - exit_errno("poll"); - } + int ret = poll(VECTOR_DATA(ctx.pollfds), VECTOR_LEN(ctx.pollfds), timeout); + if (ret < 0 && errno != EINTR) + exit_errno("poll"); pthread_sigmask(SIG_SETMASK, &oldset, NULL); fastd_update_time(); -- cgit v1.2.3