nghttpx: Fix improper signal handling

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

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();
+  rv = shrpx_signal_block_all(&oldset);
-
+  if (rv != 0) {
-  if (pid == -1) {
     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<char *[]>(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<const char *> 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) {
+  if (pid == -1) {
-  case -1:
     return -1;
-  case 0:
-    // worker process (child)
-    return 0;
   }
 
   ssv.worker_process_pid = pid;
 
-  auto signals =
+  constexpr auto signals =
       std::array<int, 5>{{REOPEN_LOG_SIGNAL, EXEC_BINARY_SIGNAL,
                           GRACEFUL_SHUTDOWN_SIGNAL, SIGINT, SIGTERM}};
   auto sigevs = std::array<ev_signal, signals.size()>();
@@ -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;
   }

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();
+  sigset_t oldset;
-  if (pid == -1) {
+
+  rv = shrpx_signal_block_all(&oldset);
+  if (rv != 0) {
     auto error = errno;
-    LOG(WARN) << "Could not execute ocsp query command for " << cert_file
+    LOG(ERROR) << "Blocking all signals failed: " << strerror(error);
-              << ": " << argv[0] << ", fork() failed, errno=" << 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]
+      LOG(ERROR) << "Could not execute ocsp query command: " << argv[0]
                  << ", execve() faild, errno=" << error;
-      _Exit(EXIT_FAILURE);
+      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;
 

src/shrpx_process.h

@@ -27,17 +27,11 @@
 
 #include "shrpx.h"
 
-#include <signal.h>
-
 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

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 <cerrno>
+
+#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 <typename Signals>
+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<int, 1>{{SIGPIPE}};
+} // namespace
+
+namespace {
+constexpr auto worker_proc_ign_signals = std::array<int, 4>{
+    {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

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 <signal.h>
+
+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

src/shrpx_worker_process.cc

@@ -422,24 +422,37 @@ 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();
-  } else {
-    rv = conn_handler.create_worker_thread(get_config()->num_worker);
-  }
-
     if (rv != 0) {
       return -1;
     }
+  } else {
+#ifndef NOTHREADS
+    sigset_t set;
+    sigemptyset(&set);
+    sigaddset(&set, SIGCHLD);
+
+    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();