nghttpx: Fix worker process crash with neverbleed write error

src/shrpx_connection_handler.cc

@@ -115,6 +115,9 @@ ConnectionHandler::ConnectionHandler(struct ev_loop *loop, std::mt19937 &gen)
     : gen_(gen),
       single_worker_(nullptr),
       loop_(loop),
+#ifdef HAVE_NEVERBLEED
+      nb_(nullptr),
+#endif // HAVE_NEVERBLEED
       tls_ticket_key_memcached_get_retry_count_(0),
       tls_ticket_key_memcached_fail_count_(0),
       worker_round_robin_cnt_(get_config()->api.enabled ? 1 : 0),
@@ -205,12 +208,12 @@ int ConnectionHandler::create_single_worker() {
       all_ssl_ctx_, indexed_ssl_ctx_, cert_tree_.get()
 #ifdef HAVE_NEVERBLEED
                                           ,
-      nb_.get()
+      nb_
 #endif // HAVE_NEVERBLEED
   );
   auto cl_ssl_ctx = tls::setup_downstream_client_ssl_context(
 #ifdef HAVE_NEVERBLEED
-      nb_.get()
+      nb_
 #endif // HAVE_NEVERBLEED
   );
 
@@ -227,7 +230,7 @@ int ConnectionHandler::create_single_worker() {
     if (memcachedconf.tls) {
       session_cache_ssl_ctx = tls::create_ssl_client_context(
 #ifdef HAVE_NEVERBLEED
-          nb_.get(),
+          nb_,
 #endif // HAVE_NEVERBLEED
           tlsconf.cacert, memcachedconf.cert_file,
           memcachedconf.private_key_file, nullptr);
@@ -256,12 +259,12 @@ int ConnectionHandler::create_worker_thread(size_t num) {
       all_ssl_ctx_, indexed_ssl_ctx_, cert_tree_.get()
 #  ifdef HAVE_NEVERBLEED
                                           ,
-      nb_.get()
+      nb_
 #  endif // HAVE_NEVERBLEED
   );
   auto cl_ssl_ctx = tls::setup_downstream_client_ssl_context(
 #  ifdef HAVE_NEVERBLEED
-      nb_.get()
+      nb_
 #  endif // HAVE_NEVERBLEED
   );
 
@@ -285,7 +288,7 @@ int ConnectionHandler::create_worker_thread(size_t num) {
     if (memcachedconf.tls) {
       session_cache_ssl_ctx = tls::create_ssl_client_context(
 #  ifdef HAVE_NEVERBLEED
-          nb_.get(),
+          nb_,
 #  endif // HAVE_NEVERBLEED
           tlsconf.cacert, memcachedconf.cert_file,
           memcachedconf.private_key_file, nullptr);
@@ -802,7 +805,7 @@ SSL_CTX *ConnectionHandler::create_tls_ticket_key_memcached_ssl_ctx() {
 
   auto ssl_ctx = tls::create_ssl_client_context(
 #ifdef HAVE_NEVERBLEED
-      nb_.get(),
+      nb_,
 #endif // HAVE_NEVERBLEED
       tlsconf.cacert, memcachedconf.cert_file, memcachedconf.private_key_file,
       nullptr);
@@ -813,12 +816,7 @@ SSL_CTX *ConnectionHandler::create_tls_ticket_key_memcached_ssl_ctx() {
 }
 
 #ifdef HAVE_NEVERBLEED
-void ConnectionHandler::set_neverbleed(std::unique_ptr<neverbleed_t> nb) {
+void ConnectionHandler::set_neverbleed(neverbleed_t *nb) { nb_ = nb; }
-  nb_ = std::move(nb);
-}
-
-neverbleed_t *ConnectionHandler::get_neverbleed() const { return nb_.get(); }
-
 #endif // HAVE_NEVERBLEED
 
 void ConnectionHandler::handle_serial_event() {

src/shrpx_connection_handler.h

@@ -160,8 +160,7 @@ public:
   const std::vector<SSL_CTX *> &get_indexed_ssl_ctx(size_t idx) const;
 
 #ifdef HAVE_NEVERBLEED
-  void set_neverbleed(std::unique_ptr<neverbleed_t> nb);
+  void set_neverbleed(neverbleed_t *nb);
-  neverbleed_t *get_neverbleed() const;
 #endif // HAVE_NEVERBLEED
 
   // Send SerialEvent SEV_REPLACE_DOWNSTREAM to this object.
@@ -210,7 +209,7 @@ private:
   struct ev_loop *loop_;
   std::vector<std::unique_ptr<AcceptHandler>> acceptors_;
 #ifdef HAVE_NEVERBLEED
-  std::unique_ptr<neverbleed_t> nb_;
+  neverbleed_t *nb_;
 #endif // HAVE_NEVERBLEED
   ev_timer disable_acceptor_timer_;
   ev_timer ocsp_timer_;

src/shrpx_worker_process.cc

@@ -411,35 +411,30 @@ int worker_process_event_loop(WorkerProcessConfig *wpconf) {
 
   auto gen = util::make_mt19937();
 
-  ConnectionHandler conn_handler(loop, gen);
+  auto conn_handler = make_unique<ConnectionHandler>(loop, gen);
 
   for (auto &addr : config->conn.listener.addrs) {
-    conn_handler.add_acceptor(make_unique<AcceptHandler>(&addr, &conn_handler));
+    conn_handler->add_acceptor(
+        make_unique<AcceptHandler>(&addr, conn_handler.get()));
   }
 
 #ifdef HAVE_NEVERBLEED
-  {
+  std::array<char, NEVERBLEED_ERRBUF_SIZE> nb_errbuf;
-    std::array<char, NEVERBLEED_ERRBUF_SIZE> nb_errbuf;
+  auto nb = make_unique<neverbleed_t>();
-    auto nb = make_unique<neverbleed_t>();
+  if (neverbleed_init(nb.get(), nb_errbuf.data()) != 0) {
-    if (neverbleed_init(nb.get(), nb_errbuf.data()) != 0) {
+    LOG(FATAL) << "neverbleed_init failed: " << nb_errbuf.data();
-      LOG(FATAL) << "neverbleed_init failed: " << nb_errbuf.data();
+    return -1;
-      return -1;
-    }
-
-    LOG(NOTICE) << "neverbleed process [" << nb->daemon_pid << "] spawned";
-
-    conn_handler.set_neverbleed(std::move(nb));
   }
 
-  auto nb = conn_handler.get_neverbleed();
+  LOG(NOTICE) << "neverbleed process [" << nb->daemon_pid << "] spawned";
+
+  conn_handler->set_neverbleed(nb.get());
 
   ev_child nb_childev;
-  if (nb) {
-    ev_child_init(&nb_childev, nb_child_cb, nb->daemon_pid, 0);
-    nb_childev.data = nullptr;
-    ev_child_start(loop, &nb_childev);
-  }
 
+  ev_child_init(&nb_childev, nb_child_cb, nb->daemon_pid, 0);
+  nb_childev.data = nullptr;
+  ev_child_start(loop, &nb_childev);
 #endif // HAVE_NEVERBLEED
 
   MemchunkPool mcpool;
@@ -453,17 +448,17 @@ int worker_process_event_loop(WorkerProcessConfig *wpconf) {
       SSL_CTX *ssl_ctx = nullptr;
 
       if (memcachedconf.tls) {
-        ssl_ctx = conn_handler.create_tls_ticket_key_memcached_ssl_ctx();
+        ssl_ctx = conn_handler->create_tls_ticket_key_memcached_ssl_ctx();
       }
 
-      conn_handler.set_tls_ticket_key_memcached_dispatcher(
+      conn_handler->set_tls_ticket_key_memcached_dispatcher(
           make_unique<MemcachedDispatcher>(
               &ticketconf.memcached.addr, loop, ssl_ctx,
               StringRef{memcachedconf.host}, &mcpool, gen));
 
       ev_timer_init(&renew_ticket_key_timer, memcached_get_ticket_key_cb, 0.,
                     0.);
-      renew_ticket_key_timer.data = &conn_handler;
+      renew_ticket_key_timer.data = conn_handler.get();
       // Get first ticket keys.
       memcached_get_ticket_key_cb(loop, &renew_ticket_key_timer, 0);
     } else {
@@ -483,14 +478,14 @@ int worker_process_event_loop(WorkerProcessConfig *wpconf) {
         if (!ticket_keys) {
           LOG(WARN) << "Use internal session ticket key generator";
         } else {
-          conn_handler.set_ticket_keys(std::move(ticket_keys));
+          conn_handler->set_ticket_keys(std::move(ticket_keys));
           auto_tls_ticket_key = false;
         }
       }
       if (auto_tls_ticket_key) {
         // Generate new ticket key every 1hr.
         ev_timer_init(&renew_ticket_key_timer, renew_ticket_key_cb, 0., 1_h);
-        renew_ticket_key_timer.data = &conn_handler;
+        renew_ticket_key_timer.data = conn_handler.get();
         ev_timer_again(loop, &renew_ticket_key_timer);
 
         // Generate first session ticket key before running workers.
@@ -500,7 +495,7 @@ int worker_process_event_loop(WorkerProcessConfig *wpconf) {
   }
 
   if (config->single_thread) {
-    rv = conn_handler.create_single_worker();
+    rv = conn_handler->create_single_worker();
     if (rv != 0) {
       return -1;
     }
@@ -518,7 +513,7 @@ int worker_process_event_loop(WorkerProcessConfig *wpconf) {
     }
 #endif // !NOTHREADS
 
-    rv = conn_handler.create_worker_thread(config->num_worker);
+    rv = conn_handler->create_worker_thread(config->num_worker);
     if (rv != 0) {
       return -1;
     }
@@ -535,22 +530,22 @@ int worker_process_event_loop(WorkerProcessConfig *wpconf) {
 
   drop_privileges(
 #ifdef HAVE_NEVERBLEED
-      nb
+      nb.get()
 #endif // HAVE_NEVERBLEED
   );
 
   ev_io ipcev;
   ev_io_init(&ipcev, ipc_readcb, wpconf->ipc_fd, EV_READ);
-  ipcev.data = &conn_handler;
+  ipcev.data = conn_handler.get();
   ev_io_start(loop, &ipcev);
 
   if (tls::upstream_tls_enabled(config->conn) && !config->tls.ocsp.disabled) {
     if (config->tls.ocsp.startup) {
-      conn_handler.set_enable_acceptor_on_ocsp_completion(true);
+      conn_handler->set_enable_acceptor_on_ocsp_completion(true);
-      conn_handler.disable_acceptor();
+      conn_handler->disable_acceptor();
     }
 
-    conn_handler.proceed_next_cert_ocsp();
+    conn_handler->proceed_next_cert_ocsp();
   }
 
   if (LOG_ENABLED(INFO)) {
@@ -559,27 +554,29 @@ int worker_process_event_loop(WorkerProcessConfig *wpconf) {
 
   ev_run(loop, 0);
 
-  conn_handler.cancel_ocsp_update();
+  conn_handler->cancel_ocsp_update();
+
+  // Destroy SSL_CTX held in conn_handler before killing neverbleed
+  // daemon.  Otherwise priv_rsa_finish yields "write error" and
+  // worker process aborts.
+  conn_handler.reset();
 
 #ifdef HAVE_NEVERBLEED
-  if (nb) {
+  assert(nb->daemon_pid > 0);
-    assert(nb->daemon_pid > 0);
 
-    rv = kill(nb->daemon_pid, SIGTERM);
+  rv = kill(nb->daemon_pid, SIGTERM);
-    if (rv != 0) {
+  if (rv != 0) {
-      auto error = errno;
+    auto error = errno;
-      LOG(ERROR) << "Could not send signal to neverbleed daemon: errno="
+    LOG(ERROR) << "Could not send signal to neverbleed daemon: errno=" << error;
-                 << error;
+  }
-    }
 
-    while ((rv = waitpid(nb->daemon_pid, nullptr, 0)) == -1 && errno == EINTR)
+  while ((rv = waitpid(nb->daemon_pid, nullptr, 0)) == -1 && errno == EINTR)
-      ;
+    ;
-    if (rv == -1) {
+  if (rv == -1) {
-      auto error = errno;
+    auto error = errno;
-      LOG(ERROR) << "Error occurred while we were waiting for the completion "
+    LOG(ERROR) << "Error occurred while we were waiting for the completion "
-                    "of neverbleed process: errno="
+                  "of neverbleed process: errno="
-                 << error;
+               << error;
-    }
   }
 #endif // HAVE_NEVERBLEED