From 389a96483a32b10be76554dd7bb7849547bdebd9 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 24 Sep 2015 23:33:28 +0900 Subject: [PATCH] nghttpx: Fix improper signal handling --- src/Makefile.am | 1 + src/shrpx.cc | 107 ++++++++++++++++++++------ src/shrpx_connection_handler.cc | 47 ++++++++++-- src/shrpx_process.h | 6 -- src/shrpx_signal.cc | 129 ++++++++++++++++++++++++++++++++ src/shrpx_signal.h | 59 +++++++++++++++ src/shrpx_worker_process.cc | 39 ++++++---- 7 files changed, 340 insertions(+), 48 deletions(-) create mode 100644 src/shrpx_signal.cc create mode 100644 src/shrpx_signal.h diff --git a/src/Makefile.am b/src/Makefile.am index 358e9d8d..3635b633 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -127,6 +127,7 @@ NGHTTPX_SRCS = \ shrpx_memcached_result.h \ shrpx_worker_process.cc shrpx_worker_process.h \ shrpx_process.h \ + shrpx_signal.cc shrpx_signal.h \ buffer.h memchunk.h template.h if HAVE_SPDYLAY diff --git a/src/shrpx.cc b/src/shrpx.cc index 3798312a..57d546e4 100644 --- a/src/shrpx.cc +++ b/src/shrpx.cc @@ -81,6 +81,7 @@ #include "shrpx_http2_session.h" #include "shrpx_worker_process.h" #include "shrpx_process.h" +#include "shrpx_signal.h" #include "util.h" #include "app_helper.h" #include "ssl.h" @@ -210,26 +211,55 @@ void save_pid() { namespace { void exec_binary(SignalServer *ssv) { + int rv; + sigset_t oldset; + LOG(NOTICE) << "Executing new binary"; - auto pid = fork(); - - if (pid == -1) { + rv = shrpx_signal_block_all(&oldset); + if (rv != 0) { auto error = errno; - LOG(ERROR) << "fork() failed errno=" << error; + LOG(ERROR) << "Blocking all signals failed: " << strerror(error); + return; } + auto pid = fork(); + if (pid != 0) { + if (pid == -1) { + auto error = errno; + LOG(ERROR) << "fork() failed errno=" << error; + } + + rv = shrpx_signal_set(&oldset); + + if (rv != 0) { + auto error = errno; + LOG(FATAL) << "Restoring signal mask failed: " << strerror(error); + + exit(EXIT_FAILURE); + } + return; } + shrpx_signal_unset_master_proc_ign_handler(); + + rv = shrpx_signal_unblock_all(); + if (rv != 0) { + auto error = errno; + LOG(ERROR) << "Unblocking all signals failed: " << strerror(error); + + exit(EXIT_FAILURE); + } + auto exec_path = util::get_exec_path(get_config()->argc, get_config()->argv, get_config()->cwd); if (!exec_path) { LOG(ERROR) << "Could not resolve the executable path"; - return; + exit(EXIT_FAILURE); } auto argv = make_unique(get_config()->argc + 1); @@ -306,7 +336,7 @@ void exec_binary(SignalServer *ssv) { if (execve(argv[0], argv.get(), envp.get()) == -1) { auto error = errno; LOG(ERROR) << "execve failed: errno=" << error; - _Exit(EXIT_FAILURE); + exit(EXIT_FAILURE); } } } // namespace @@ -647,20 +677,59 @@ void close_env_fd(std::initializer_list envnames) { namespace { pid_t fork_worker_process(SignalServer *ssv) { int rv; - auto pid = fork(); + sigset_t oldset; + + rv = shrpx_signal_block_all(&oldset); + if (rv != 0) { + auto error = errno; + LOG(ERROR) << "Blocking all signals failed: " << strerror(error); - if (pid == -1) { return -1; } + auto pid = fork(); + if (pid == 0) { + ev_loop_fork(EV_DEFAULT); + + shrpx_signal_set_worker_proc_ign_handler(); + + rv = shrpx_signal_unblock_all(); + if (rv != 0) { + auto error = errno; + LOG(FATAL) << "Unblocking all signals failed: " << strerror(error); + + exit(EXIT_FAILURE); + } + close(ssv->ipc_fd[1]); WorkerProcessConfig wpconf{ssv->ipc_fd[0], ssv->server_fd, ssv->server_fd6}; rv = worker_process_event_loop(&wpconf); if (rv != 0) { - LOG(ERROR) << "Worker process returned error"; + LOG(FATAL) << "Worker process returned error"; + + exit(EXIT_FAILURE); } - return 0; + + exit(EXIT_SUCCESS); + } + + // parent process + if (pid == -1) { + auto error = errno; + LOG(ERROR) << "Could not spawn worker process: " << strerror(error); + } + + rv = shrpx_signal_set(&oldset); + if (rv != 0) { + auto error = errno; + LOG(FATAL) << "Restoring signal mask failed: " << strerror(error); + + exit(EXIT_FAILURE); + } + + if (pid == -1) { + return -1; } close(ssv->ipc_fd[0]); @@ -675,6 +744,8 @@ namespace { int event_loop() { int rv; + shrpx_signal_set_master_proc_ign_handler(); + if (get_config()->daemon) { if (call_daemon() == -1) { auto error = errno; @@ -710,8 +781,6 @@ int event_loop() { util::make_socket_closeonexec(fd); } - auto loop = EV_DEFAULT; - if (get_config()->host_unix) { close_env_fd({ENV_LISTENER4_FD, ENV_LISTENER6_FD}); auto fd = create_unix_domain_server_socket(); @@ -746,19 +815,17 @@ int event_loop() { ssv.server_fd6 = fd6; } + auto loop = EV_DEFAULT; + auto pid = fork_worker_process(&ssv); - switch (pid) { - case -1: + if (pid == -1) { return -1; - case 0: - // worker process (child) - return 0; } ssv.worker_process_pid = pid; - auto signals = + constexpr auto signals = std::array{{REOPEN_LOG_SIGNAL, EXEC_BINARY_SIGNAL, GRACEFUL_SHUTDOWN_SIGNAL, SIGINT, SIGTERM}}; auto sigevs = std::array(); @@ -2444,10 +2511,6 @@ int main(int argc, char **argv) { reset_timer(); } - struct sigaction act {}; - act.sa_handler = SIG_IGN; - sigaction(SIGPIPE, &act, nullptr); - if (event_loop() != 0) { return -1; } diff --git a/src/shrpx_connection_handler.cc b/src/shrpx_connection_handler.cc index 76cb51c4..5610b06b 100644 --- a/src/shrpx_connection_handler.cc +++ b/src/shrpx_connection_handler.cc @@ -43,6 +43,7 @@ #include "shrpx_downstream_connection.h" #include "shrpx_accept_handler.h" #include "shrpx_memcached_dispatcher.h" +#include "shrpx_signal.h" #include "util.h" #include "template.h" @@ -432,30 +433,62 @@ int ConnectionHandler::start_ocsp_update(const char *cert_file) { } }); - auto pid = fork(); - if (pid == -1) { + sigset_t oldset; + + rv = shrpx_signal_block_all(&oldset); + if (rv != 0) { auto error = errno; - LOG(WARN) << "Could not execute ocsp query command for " << cert_file - << ": " << argv[0] << ", fork() failed, errno=" << error; + LOG(ERROR) << "Blocking all signals failed: " << strerror(error); + return -1; } + auto pid = fork(); + if (pid == 0) { // child process + shrpx_signal_unset_worker_proc_ign_handler(); + + rv = shrpx_signal_unblock_all(); + if (rv != 0) { + auto error = errno; + LOG(FATAL) << "Unblocking all signals failed: " << strerror(error); + + exit(EXIT_FAILURE); + } + dup2(pfd[1], 1); close(pfd[0]); rv = execve(argv[0], argv, envp); if (rv == -1) { auto error = errno; - LOG(WARN) << "Could not execute ocsp query command: " << argv[0] - << ", execve() faild, errno=" << error; - _Exit(EXIT_FAILURE); + LOG(ERROR) << "Could not execute ocsp query command: " << argv[0] + << ", execve() faild, errno=" << error; + exit(EXIT_FAILURE); } // unreachable } // parent process + if (pid == -1) { + auto error = errno; + LOG(ERROR) << "Could not execute ocsp query command for " << cert_file + << ": " << argv[0] << ", fork() failed, errno=" << error; + } + + rv = shrpx_signal_set(&oldset); + if (rv != 0) { + auto error = errno; + LOG(FATAL) << "Restoring all signals failed: " << strerror(error); + + exit(EXIT_FAILURE); + } + + if (pid == -1) { + return -1; + } + close(pfd[1]); pfd[1] = -1; diff --git a/src/shrpx_process.h b/src/shrpx_process.h index 47fc5b89..d35461b9 100644 --- a/src/shrpx_process.h +++ b/src/shrpx_process.h @@ -27,17 +27,11 @@ #include "shrpx.h" -#include - namespace shrpx { constexpr uint8_t SHRPX_IPC_REOPEN_LOG = 1; constexpr uint8_t SHRPX_IPC_GRACEFUL_SHUTDOWN = 2; -constexpr int REOPEN_LOG_SIGNAL = SIGUSR1; -constexpr int EXEC_BINARY_SIGNAL = SIGUSR2; -constexpr int GRACEFUL_SHUTDOWN_SIGNAL = SIGQUIT; - } // namespace shrpx #endif // SHRPX_PROCESS_H diff --git a/src/shrpx_signal.cc b/src/shrpx_signal.cc new file mode 100644 index 00000000..f689d4fa --- /dev/null +++ b/src/shrpx_signal.cc @@ -0,0 +1,129 @@ +/* + * nghttp2 - HTTP/2 C Library + * + * Copyright (c) 2015 Tatsuhiro Tsujikawa + * + * Permission is hereby granted, free of charge, to any person obtaining + * a copy of this software and associated documentation files (the + * "Software"), to deal in the Software without restriction, including + * without limitation the rights to use, copy, modify, merge, publish, + * distribute, sublicense, and/or sell copies of the Software, and to + * permit persons to whom the Software is furnished to do so, subject to + * the following conditions: + * + * The above copyright notice and this permission notice shall be + * included in all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE + * LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION + * OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +#include "shrpx_signal.h" + +#include + +#include "template.h" + +using namespace nghttp2; + +namespace shrpx { + +int shrpx_signal_block_all(sigset_t *oldset) { + sigset_t newset; + int rv; + + sigfillset(&newset); + +#ifndef NOTHREADS + rv = pthread_sigmask(SIG_SETMASK, &newset, oldset); + + if (rv != 0) { + errno = rv; + return -1; + } + + return 0; +#else // NOTHREADS + return sigprocmask(SIG_SETMASK, &newset, &oldset); +#endif // NOTHREADS +} + +int shrpx_signal_unblock_all() { + sigset_t newset; + int rv; + + sigemptyset(&newset); + +#ifndef NOTHREADS + rv = pthread_sigmask(SIG_SETMASK, &newset, nullptr); + + if (rv != 0) { + errno = rv; + return -1; + } + + return 0; +#else // NOTHREADS + return sigprocmask(SIG_SETMASK, &newset, nullptr); +#endif // NOTHREADS +} + +int shrpx_signal_set(sigset_t *set) { + int rv; + +#ifndef NOTHREADS + rv = pthread_sigmask(SIG_SETMASK, set, nullptr); + + if (rv != 0) { + errno = rv; + return -1; + } + + return 0; +#else // NOTHREADS + return sigprocmask(SIG_SETMASK, set, nullptr); +#endif // NOTHREADS +} + +namespace { +template +void signal_set_handler(void (*handler)(int), Signals &&sigs) { + struct sigaction act {}; + act.sa_handler = handler; + sigemptyset(&act.sa_mask); + for (auto sig : sigs) { + sigaction(sig, &act, nullptr); + } +} +} // namespace + +namespace { +constexpr auto master_proc_ign_signals = std::array{{SIGPIPE}}; +} // namespace + +namespace { +constexpr auto worker_proc_ign_signals = std::array{ + {REOPEN_LOG_SIGNAL, EXEC_BINARY_SIGNAL, GRACEFUL_SHUTDOWN_SIGNAL, SIGPIPE}}; +} // namespace + +void shrpx_signal_set_master_proc_ign_handler() { + signal_set_handler(SIG_IGN, master_proc_ign_signals); +} + +void shrpx_signal_unset_master_proc_ign_handler() { + signal_set_handler(SIG_DFL, master_proc_ign_signals); +} + +void shrpx_signal_set_worker_proc_ign_handler() { + signal_set_handler(SIG_IGN, worker_proc_ign_signals); +} + +void shrpx_signal_unset_worker_proc_ign_handler() { + signal_set_handler(SIG_DFL, worker_proc_ign_signals); +} + +} // namespace shrpx diff --git a/src/shrpx_signal.h b/src/shrpx_signal.h new file mode 100644 index 00000000..dcedd792 --- /dev/null +++ b/src/shrpx_signal.h @@ -0,0 +1,59 @@ +/* + * nghttp2 - HTTP/2 C Library + * + * Copyright (c) 2015 Tatsuhiro Tsujikawa + * + * Permission is hereby granted, free of charge, to any person obtaining + * a copy of this software and associated documentation files (the + * "Software"), to deal in the Software without restriction, including + * without limitation the rights to use, copy, modify, merge, publish, + * distribute, sublicense, and/or sell copies of the Software, and to + * permit persons to whom the Software is furnished to do so, subject to + * the following conditions: + * + * The above copyright notice and this permission notice shall be + * included in all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE + * LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION + * OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +#ifndef SHRPX_SIGNAL_H +#define SHRPX_SIGNAL_H + +#include "shrpx.h" + +#include + +namespace shrpx { + +constexpr int REOPEN_LOG_SIGNAL = SIGUSR1; +constexpr int EXEC_BINARY_SIGNAL = SIGUSR2; +constexpr int GRACEFUL_SHUTDOWN_SIGNAL = SIGQUIT; + +// Blocks all signals. The previous signal mask is stored into +// |oldset| if it is not nullptr. This function returns 0 if it +// succeeds, or -1. The errno will indicate the error. +int shrpx_signal_block_all(sigset_t *oldset); + +// Unblocks all signals. This function returns 0 if it succeeds, or +// -1. The errno will indicate the error. +int shrpx_signal_unblock_all(); + +// Sets signal mask |set|. This function returns 0 if it succeeds, or +// -1. The errno will indicate the error. +int shrpx_signal_set(sigset_t *set); + +void shrpx_signal_set_master_proc_ign_handler(); +void shrpx_signal_unset_master_proc_ign_handler(); + +void shrpx_signal_set_worker_proc_ign_handler(); +void shrpx_signal_unset_worker_proc_ign_handler(); + +} // namespace shrpx + +#endif // SHRPX_SIGNAL_H diff --git a/src/shrpx_worker_process.cc b/src/shrpx_worker_process.cc index 76411c02..bda182dc 100644 --- a/src/shrpx_worker_process.cc +++ b/src/shrpx_worker_process.cc @@ -422,23 +422,36 @@ int worker_process_event_loop(WorkerProcessConfig *wpconf) { int rv; - // It is good to ignore these control signals since user may use - // "killall .. nghttpx". We don't want catch this signal in worker - // process. - struct sigaction act {}; - act.sa_handler = SIG_IGN; - sigaction(REOPEN_LOG_SIGNAL, &act, nullptr); - sigaction(EXEC_BINARY_SIGNAL, &act, nullptr); - sigaction(GRACEFUL_SHUTDOWN_SIGNAL, &act, nullptr); - if (get_config()->num_worker == 1) { rv = conn_handler.create_single_worker(); + if (rv != 0) { + return -1; + } } else { - rv = conn_handler.create_worker_thread(get_config()->num_worker); - } +#ifndef NOTHREADS + sigset_t set; + sigemptyset(&set); + sigaddset(&set, SIGCHLD); - if (rv != 0) { - return -1; + rv = pthread_sigmask(SIG_BLOCK, &set, nullptr); + if (rv != 0) { + LOG(ERROR) << "Blocking SIGCHLD failed: " << strerror(rv); + return -1; + } +#endif // !NOTHREADS + + rv = conn_handler.create_worker_thread(get_config()->num_worker); + if (rv != 0) { + return -1; + } + +#ifndef NOTHREADS + rv = pthread_sigmask(SIG_UNBLOCK, &set, nullptr); + if (rv != 0) { + LOG(ERROR) << "Unblocking SIGCHLD failed: " << strerror(rv); + return -1; + } +#endif // !NOTHREADS } drop_privileges();