nghttpx: Rewrite ocsp without thread

Since libev handles SIGCHLD, using waitpid in separate thread to wait
for the completion of fetch-ocsp-response script process is undefined.
This commit rewrite ocsp handling code so that it utilizes libev
ev_child watcher and perform ocsp update without thread.

src/shrpx.cc

@@ -576,8 +576,6 @@ void refresh_cb(struct ev_loop *loop, ev_timer *w, int revents) {
       (!worker || worker->get_worker_stat()->num_connections == 0)) {
     ev_break(loop);
   }
-
-  conn_handler->handle_ocsp_completion();
 }
 } // namespace
 
@@ -748,7 +746,7 @@ int event_loop() {
   ev_timer_again(loop, &refresh_timer);
 
   if (!get_config()->upstream_no_tls && !get_config()->no_ocsp) {
-    conn_handler->update_ocsp_async();
+    conn_handler->proceed_next_cert_ocsp();
   }
 
   if (LOG_ENABLED(INFO)) {
@@ -758,7 +756,7 @@ int event_loop() {
   ev_run(loop, 0);
 
   conn_handler->join_worker();
-  conn_handler->join_ocsp_thread();
+  conn_handler->cancel_ocsp_update();
 
   return 0;
 }
@@ -2142,7 +2140,6 @@ int main(int argc, char **argv) {
   memset(&act, 0, sizeof(struct sigaction));
   act.sa_handler = SIG_IGN;
   sigaction(SIGPIPE, &act, nullptr);
-  sigaction(SIGCHLD, &act, nullptr);
 
   event_loop();
 

src/shrpx_connection_handler.cc

@@ -25,9 +25,14 @@
 #include "shrpx_connection_handler.h"
 
 #include <unistd.h>
+#include <sys/types.h>
+#include <sys/wait.h>
 
 #include <cerrno>
 #include <thread>
+#ifndef NOTHREADS
+#include <spawn.h>
+#endif // !NOTHREADS
 
 #include "shrpx_client_handler.h"
 #include "shrpx_ssl.h"
@@ -67,7 +72,23 @@ void ocsp_cb(struct ev_loop *loop, ev_timer *w, int revent) {
     return;
   }
 
-  h->update_ocsp_async();
+  h->proceed_next_cert_ocsp();
+}
+} // namespace
+
+namespace {
+void ocsp_read_cb(struct ev_loop *loop, ev_io *w, int revent) {
+  auto h = static_cast<ConnectionHandler *>(w->data);
+
+  h->read_ocsp_chunk();
+}
+} // namespace
+
+namespace {
+void ocsp_chld_cb(struct ev_loop *loop, ev_child *w, int revent) {
+  auto h = static_cast<ConnectionHandler *>(w->data);
+
+  h->handle_ocsp_complete();
 }
 } // namespace
 
@@ -79,6 +100,17 @@ ConnectionHandler::ConnectionHandler(struct ev_loop *loop)
 
   ev_timer_init(&ocsp_timer_, ocsp_cb, 0., 0.);
   ocsp_timer_.data = this;
+
+  ev_io_init(&ocsp_.rev, ocsp_read_cb, -1, EV_READ);
+  ocsp_.rev.data = this;
+
+  ev_child_init(&ocsp_.chldev, ocsp_chld_cb, 0, 0);
+  ocsp_.chldev.data = this;
+
+  ocsp_.next = 0;
+  ocsp_.fd = -1;
+
+  reset_ocsp();
 }
 
 ConnectionHandler::~ConnectionHandler() {
@@ -315,80 +347,190 @@ bool ConnectionHandler::get_graceful_shutdown() const {
   return graceful_shutdown_;
 }
 
-namespace {
+void ConnectionHandler::cancel_ocsp_update() {
-void update_ocsp_ssl_ctx(SSL_CTX *ssl_ctx) {
+  if (ocsp_.pid == 0) {
+    return;
+  }
+
+  kill(ocsp_.pid, SIGTERM);
+}
+
+// inspired by h2o_read_command function from h2o project:
+// https://github.com/h2o/h2o
+int ConnectionHandler::start_ocsp_update(const char *cert_file) {
+#ifndef NOTHREADS
+  int rv;
+  int pfd[2];
+
+  assert(!ev_is_active(&ocsp_.rev));
+  assert(!ev_is_active(&ocsp_.chldev));
+
+  char *const argv[] = {
+      const_cast<char *>(get_config()->fetch_ocsp_response_file.get()),
+      const_cast<char *>(cert_file), nullptr};
+  char *const envp[] = {nullptr};
+
+#ifdef O_CLOEXEC
+  if (pipe2(pfd, O_CLOEXEC) == -1) {
+    return -1;
+  }
+#else  // !O_CLOEXEC
+  if (pipe(pfd) == -1) {
+    return -1;
+  }
+  util::make_socket_closeonexec(pfd[0]);
+  util::make_socket_closeonexec(pfd[1]);
+#endif // !O_CLOEXEC
+
+  auto closer = defer([&pfd]() {
+    if (pfd[0] != -1) {
+      close(pfd[0]);
+    }
+
+    if (pfd[1] != -1) {
+      close(pfd[1]);
+    }
+  });
+
+  // posix_spawn family functions are really interesting.  They makes
+  // fork + dup2 + execve pattern easier.
+
+  posix_spawn_file_actions_t file_actions;
+
+  if (posix_spawn_file_actions_init(&file_actions) != 0) {
+    return -1;
+  }
+
+  auto file_actions_del =
+      defer(posix_spawn_file_actions_destroy, &file_actions);
+
+  if (posix_spawn_file_actions_adddup2(&file_actions, pfd[1], 1) != 0) {
+    return -1;
+  }
+
+  if (posix_spawn_file_actions_addclose(&file_actions, pfd[0]) != 0) {
+    return -1;
+  }
+
+  rv = posix_spawn(&ocsp_.pid, argv[0], &file_actions, nullptr, argv, envp);
+  if (rv != 0) {
+    LOG(WARN) << "Cannot execute ocsp query command: " << argv[0]
+              << ", errno=" << rv;
+    return -1;
+  }
+
+  close(pfd[1]);
+  pfd[1] = -1;
+
+  ocsp_.fd = pfd[0];
+  pfd[0] = -1;
+
+  util::make_socket_nonblocking(ocsp_.fd);
+  ev_io_set(&ocsp_.rev, ocsp_.fd, EV_READ);
+  ev_io_start(loop_, &ocsp_.rev);
+
+  ev_child_set(&ocsp_.chldev, ocsp_.pid, 0);
+  ev_child_start(loop_, &ocsp_.chldev);
+#endif // !NOTHREADS
+  return 0;
+}
+
+void ConnectionHandler::read_ocsp_chunk() {
+  std::array<uint8_t, 4096> buf;
+  for (;;) {
+    ssize_t n;
+    while ((n = read(ocsp_.fd, buf.data(), buf.size())) == -1 && errno == EINTR)
+      ;
+
+    if (n == -1) {
+      if (errno == EAGAIN || errno == EWOULDBLOCK) {
+        return;
+      }
+      auto error = errno;
+      LOG(WARN) << "Reading from ocsp query command failed: errno=" << error;
+      ocsp_.error = error;
+
+      break;
+    }
+
+    if (n == 0) {
+      break;
+    }
+
+    std::copy_n(std::begin(buf), n, std::back_inserter(ocsp_.resp));
+  }
+
+  ev_io_stop(loop_, &ocsp_.rev);
+}
+
+void ConnectionHandler::handle_ocsp_complete() {
+  ev_io_stop(loop_, &ocsp_.rev);
+  ev_child_stop(loop_, &ocsp_.chldev);
+
+  auto rstatus = ocsp_.chldev.rstatus;
+  auto status = WEXITSTATUS(rstatus);
+  if (ocsp_.error || !WIFEXITED(rstatus) || status != 0) {
+    LOG(WARN) << "ocsp query command failed: error=" << ocsp_.error
+              << ", rstatus=" << rstatus << ", status=" << status;
+    ++ocsp_.next;
+    proceed_next_cert_ocsp();
+    return;
+  }
+
+  assert(ocsp_.next < all_ssl_ctx_.size());
+
+  auto ssl_ctx = all_ssl_ctx_[ocsp_.next];
+  auto tls_ctx_data =
+      static_cast<ssl::TLSContextData *>(SSL_CTX_get_app_data(ssl_ctx));
+
+  if (LOG_ENABLED(INFO)) {
+    LOG(INFO) << "ocsp update for " << tls_ctx_data->cert_file
+              << " finished successfully";
+  }
+
+  {
+    std::lock_guard<std::mutex> g(tls_ctx_data->mu);
+    tls_ctx_data->ocsp_data = std::move(ocsp_.resp);
+  }
+
+  ++ocsp_.next;
+  proceed_next_cert_ocsp();
+}
+
+void ConnectionHandler::reset_ocsp() {
+  if (ocsp_.fd != -1) {
+    close(ocsp_.fd);
+  }
+
+  ocsp_.fd = -1;
+  ocsp_.pid = 0;
+  ocsp_.error = 0;
+  ocsp_.resp = std::vector<uint8_t>();
+}
+
+void ConnectionHandler::proceed_next_cert_ocsp() {
+  for (;;) {
+    reset_ocsp();
+    if (ocsp_.next == all_ssl_ctx_.size()) {
+      ocsp_.next = 0;
+      // We have updated all ocsp response, and schedule next update.
+      ev_timer_set(&ocsp_timer_, get_config()->ocsp_update_interval, 0.);
+      ev_timer_start(loop_, &ocsp_timer_);
+      return;
+    }
+
+    auto ssl_ctx = all_ssl_ctx_[ocsp_.next];
     auto tls_ctx_data =
         static_cast<ssl::TLSContextData *>(SSL_CTX_get_app_data(ssl_ctx));
     auto cert_file = tls_ctx_data->cert_file;
 
-  std::vector<uint8_t> out;
+    if (start_ocsp_update(cert_file) != 0) {
-  if (ssl::get_ocsp_response(out, cert_file) != 0) {
+      ++ocsp_.next;
-    LOG(WARN) << "ocsp update for " << cert_file << " failed";
+      continue;
-    return;
     }
 
-  if (LOG_ENABLED(INFO)) {
+    break;
-    LOG(INFO) << "ocsp update for " << cert_file << " finished successfully";
   }
-
-  std::lock_guard<std::mutex> g(tls_ctx_data->mu);
-  tls_ctx_data->ocsp_data = std::move(out);
-}
-} // namespace
-
-void ConnectionHandler::update_ocsp() {
-  for (auto ssl_ctx : all_ssl_ctx_) {
-    update_ocsp_ssl_ctx(ssl_ctx);
-  }
-}
-
-void ConnectionHandler::update_ocsp_async() {
-#ifndef NOTHREADS
-  ocsp_result_ = std::async(std::launch::async, [this]() {
-    // Log files are managed per thread.  We have to open log files
-    // for this thread.  We don't reopen log files in this thread when
-    // signal is received.  This is future TODO.
-    reopen_log_files();
-    auto closer = defer([]() {
-      auto lgconf = log_config();
-      if (lgconf->accesslog_fd != -1) {
-        close(lgconf->accesslog_fd);
-      }
-      if (lgconf->errorlog_fd != -1) {
-        close(lgconf->errorlog_fd);
-      }
-    });
-
-    update_ocsp();
-  });
-#endif // !NOTHREADS
-}
-
-void ConnectionHandler::handle_ocsp_completion() {
-#ifndef NOTHREADS
-  if (!ocsp_result_.valid()) {
-    return;
-  }
-
-  if (ocsp_result_.wait_for(std::chrono::seconds(0)) !=
-      std::future_status::ready) {
-    return;
-  }
-
-  ocsp_result_.get();
-
-  ev_timer_set(&ocsp_timer_, get_config()->ocsp_update_interval, 0.);
-  ev_timer_start(loop_, &ocsp_timer_);
-#endif // !NOTHREADS
-}
-
-void ConnectionHandler::join_ocsp_thread() {
-#ifndef NOTHREADS
-  if (!ocsp_result_.valid()) {
-    return;
-  }
-  ocsp_result_.get();
-#endif // !NOTHREADS
 }
 
 } // namespace shrpx

src/shrpx_connection_handler.h

@@ -32,9 +32,6 @@
 
 #include <memory>
 #include <vector>
-#ifndef NOTHREADS
-#include <future>
-#endif // !NOTHREADS
 
 #include <openssl/ssl.h>
 
@@ -51,6 +48,22 @@ class Worker;
 struct WorkerStat;
 struct TicketKeys;
 
+struct OCSPUpdateContext {
+  // ocsp response buffer
+  std::vector<uint8_t> resp;
+  // index to ConnectionHandler::all_ssl_ctx_, which points to next
+  // SSL_CTX to update ocsp response cache.
+  size_t next;
+  ev_child chldev;
+  ev_io rev;
+  // fd to read response from fetch-ocsp-response script
+  int fd;
+  // errno encountered while processing response
+  int error;
+  // pid of forked fetch-ocsp-response script process
+  pid_t pid;
+};
+
 class ConnectionHandler {
 public:
   ConnectionHandler(struct ev_loop *loop);
@@ -79,23 +92,25 @@ public:
   void set_graceful_shutdown(bool f);
   bool get_graceful_shutdown() const;
   void join_worker();
-  // Updates OCSP response cache for all server side SSL_CTX object
+
-  void update_ocsp();
+  // Cancels ocsp update process
-  // Just like update_ocsp(), but performed in new thread.  Call
+  void cancel_ocsp_update();
-  // handle_ocsp_completion() to handle its completion and scheduling
+  // Starts ocsp update for certficate |cert_file|.
-  // next update.
+  int start_ocsp_update(const char *cert_file);
-  void update_ocsp_async();
+  // Reads incoming data from ocsp update process
-  // Handles asynchronous OCSP update completion and schedules next
+  void read_ocsp_chunk();
+  // Handles the completion of one ocsp update
+  void handle_ocsp_complete();
+  // Resets ocsp_;
+  void reset_ocsp();
+  // Proceeds to the next certificate's ocsp update.  If all
+  // certificates' ocsp update has been done, schedule next ocsp
   // update.
-  void handle_ocsp_completion();
+  void proceed_next_cert_ocsp();
-  // Waits for OCSP thread finishes if it is still running.
-  void join_ocsp_thread();
 
 private:
-#ifndef NOTHREADS
-  std::future<void> ocsp_result_;
-#endif // !NOTHREADS
   std::vector<SSL_CTX *> all_ssl_ctx_;
+  OCSPUpdateContext ocsp_;
   // Worker instances when multi threaded mode (-nN, N >= 2) is used.
   std::vector<std::unique_ptr<Worker>> workers_;
   // Worker instance used when single threaded mode (-n1) is used.

src/shrpx_ssl.cc

@@ -29,10 +29,6 @@
 #include <netinet/tcp.h>
 #include <pthread.h>
 #include <sys/types.h>
-#include <sys/wait.h>
-#ifndef NOTHREADS
-#include <spawn.h>
-#endif // !NOTHREADS
 
 #include <vector>
 #include <string>
@@ -1025,114 +1021,6 @@ CertLookupTree *create_cert_lookup_tree() {
   return new ssl::CertLookupTree();
 }
 
-namespace {
-// inspired by h2o_read_command function from h2o project:
-// https://github.com/h2o/h2o
-int exec_read_stdout(std::vector<uint8_t> &out, char *const *argv,
-                     char *const *envp) {
-#ifndef NOTHREADS
-  int rv;
-  int pfd[2];
-
-#ifdef O_CLOEXEC
-  if (pipe2(pfd, O_CLOEXEC) == -1) {
-    return -1;
-  }
-#else  // !O_CLOEXEC
-  if (pipe(pfd) == -1) {
-    return -1;
-  }
-  util::make_socket_closeonexec(pfd[0]);
-  util::make_socket_closeonexec(pfd[1]);
-#endif // !O_CLOEXEC
-
-  auto closer = defer([pfd]() {
-    close(pfd[0]);
-
-    if (pfd[1] != -1) {
-      close(pfd[1]);
-    }
-  });
-
-  // posix_spawn family functions are really interesting.  They makes
-  // fork + dup2 + execve pattern easier.
-
-  posix_spawn_file_actions_t file_actions;
-
-  if (posix_spawn_file_actions_init(&file_actions) != 0) {
-    return -1;
-  }
-
-  auto file_actions_del =
-      defer(posix_spawn_file_actions_destroy, &file_actions);
-
-  if (posix_spawn_file_actions_adddup2(&file_actions, pfd[1], 1) != 0) {
-    return -1;
-  }
-
-  if (posix_spawn_file_actions_addclose(&file_actions, pfd[0]) != 0) {
-    return -1;
-  }
-
-  pid_t pid;
-  rv = posix_spawn(&pid, argv[0], &file_actions, nullptr, argv, envp);
-  if (rv != 0) {
-    LOG(WARN) << "Cannot execute ocsp query command: " << argv[0]
-              << ", errno=" << rv;
-    return -1;
-  }
-
-  close(pfd[1]);
-  pfd[1] = -1;
-
-  std::array<uint8_t, 4096> buf;
-  for (;;) {
-    ssize_t n;
-    while ((n = read(pfd[0], buf.data(), buf.size())) == -1 && errno == EINTR)
-      ;
-
-    if (n == -1) {
-      auto error = errno;
-      LOG(WARN) << "Reading from ocsp query command failed: errno=" << error;
-      return -1;
-    }
-
-    if (n == 0) {
-      break;
-    }
-
-    std::copy_n(std::begin(buf), n, std::back_inserter(out));
-  }
-
-  int status;
-  if (waitpid(pid, &status, 0) == -1) {
-    auto error = errno;
-    LOG(WARN) << "waitpid for ocsp query command failed: errno=" << error;
-    return -1;
-  }
-
-  if (!WIFEXITED(status)) {
-    LOG(WARN) << "ocsp query command did not exit normally: " << status;
-    return -1;
-  }
-#endif // !NOTHREADS
-  return 0;
-}
-} // namespace
-
-int get_ocsp_response(std::vector<uint8_t> &out, const char *cert_file) {
-  char *const argv[] = {
-      const_cast<char *>(get_config()->fetch_ocsp_response_file.get()),
-      const_cast<char *>(cert_file), nullptr};
-  char *const envp[] = {nullptr};
-
-  if (exec_read_stdout(out, argv, envp) != 0 || out.empty()) {
-    return -1;
-  }
-
-  return 0;
-}
-
 } // namespace ssl
 
 } // namespace shrpx

src/shrpx_ssl.h

@@ -170,8 +170,6 @@ SSL_CTX *setup_client_ssl_context();
 // this function returns nullptr.
 CertLookupTree *create_cert_lookup_tree();
 
-int get_ocsp_response(std::vector<uint8_t> &out, const char *cert_file);
-
 } // namespace ssl
 
 } // namespace shrpx