From 0422f8a8446d8451026816a7ebee2437bc62c400 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Fri, 24 Aug 2018 22:15:43 +0900 Subject: [PATCH] nghttpx: Fix worker process crash with neverbleed write error --- src/shrpx_connection_handler.cc | 24 ++++----- src/shrpx_connection_handler.h | 5 +- src/shrpx_worker_process.cc | 93 ++++++++++++++++----------------- 3 files changed, 58 insertions(+), 64 deletions(-) diff --git a/src/shrpx_connection_handler.cc b/src/shrpx_connection_handler.cc index 89c8b4cc..0f69c098 100644 --- a/src/shrpx_connection_handler.cc +++ b/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 nb) { - nb_ = std::move(nb); -} - -neverbleed_t *ConnectionHandler::get_neverbleed() const { return nb_.get(); } - +void ConnectionHandler::set_neverbleed(neverbleed_t *nb) { nb_ = nb; } #endif // HAVE_NEVERBLEED void ConnectionHandler::handle_serial_event() { diff --git a/src/shrpx_connection_handler.h b/src/shrpx_connection_handler.h index 65de76fa..8a002440 100644 --- a/src/shrpx_connection_handler.h +++ b/src/shrpx_connection_handler.h @@ -160,8 +160,7 @@ public: const std::vector &get_indexed_ssl_ctx(size_t idx) const; #ifdef HAVE_NEVERBLEED - void set_neverbleed(std::unique_ptr nb); - neverbleed_t *get_neverbleed() const; + void set_neverbleed(neverbleed_t *nb); #endif // HAVE_NEVERBLEED // Send SerialEvent SEV_REPLACE_DOWNSTREAM to this object. @@ -210,7 +209,7 @@ private: struct ev_loop *loop_; std::vector> acceptors_; #ifdef HAVE_NEVERBLEED - std::unique_ptr nb_; + neverbleed_t *nb_; #endif // HAVE_NEVERBLEED ev_timer disable_acceptor_timer_; ev_timer ocsp_timer_; diff --git a/src/shrpx_worker_process.cc b/src/shrpx_worker_process.cc index f1c67726..8c92ce06 100644 --- a/src/shrpx_worker_process.cc +++ b/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(loop, gen); for (auto &addr : config->conn.listener.addrs) { - conn_handler.add_acceptor(make_unique(&addr, &conn_handler)); + conn_handler->add_acceptor( + make_unique(&addr, conn_handler.get())); } #ifdef HAVE_NEVERBLEED - { - std::array nb_errbuf; - auto nb = make_unique(); - if (neverbleed_init(nb.get(), nb_errbuf.data()) != 0) { - LOG(FATAL) << "neverbleed_init failed: " << nb_errbuf.data(); - return -1; - } - - LOG(NOTICE) << "neverbleed process [" << nb->daemon_pid << "] spawned"; - - conn_handler.set_neverbleed(std::move(nb)); + std::array nb_errbuf; + auto nb = make_unique(); + if (neverbleed_init(nb.get(), nb_errbuf.data()) != 0) { + LOG(FATAL) << "neverbleed_init failed: " << nb_errbuf.data(); + return -1; } - 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( &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.disable_acceptor(); + conn_handler->set_enable_acceptor_on_ocsp_completion(true); + 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); - if (rv != 0) { - auto error = errno; - LOG(ERROR) << "Could not send signal to neverbleed daemon: errno=" - << error; - } + rv = kill(nb->daemon_pid, SIGTERM); + if (rv != 0) { + auto error = errno; + LOG(ERROR) << "Could not send signal to neverbleed daemon: errno=" << error; + } - while ((rv = waitpid(nb->daemon_pid, nullptr, 0)) == -1 && errno == EINTR) - ; - if (rv == -1) { - auto error = errno; - LOG(ERROR) << "Error occurred while we were waiting for the completion " - "of neverbleed process: errno=" - << error; - } + while ((rv = waitpid(nb->daemon_pid, nullptr, 0)) == -1 && errno == EINTR) + ; + if (rv == -1) { + auto error = errno; + LOG(ERROR) << "Error occurred while we were waiting for the completion " + "of neverbleed process: errno=" + << error; } #endif // HAVE_NEVERBLEED