From d247470da2e035d1a6eaea7d2c2796b6051bd909 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 9 Apr 2015 00:25:47 +0900 Subject: [PATCH 1/4] 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 | 7 +- src/shrpx_connection_handler.cc | 266 ++++++++++++++++++++++++-------- src/shrpx_connection_handler.h | 47 ++++-- src/shrpx_ssl.cc | 112 -------------- src/shrpx_ssl.h | 2 - 5 files changed, 237 insertions(+), 197 deletions(-) diff --git a/src/shrpx.cc b/src/shrpx.cc index 9b4d88b4..20b9456b 100644 --- a/src/shrpx.cc +++ b/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(); diff --git a/src/shrpx_connection_handler.cc b/src/shrpx_connection_handler.cc index 87bf9336..1021929d 100644 --- a/src/shrpx_connection_handler.cc +++ b/src/shrpx_connection_handler.cc @@ -25,9 +25,14 @@ #include "shrpx_connection_handler.h" #include +#include +#include #include #include +#ifndef NOTHREADS +#include +#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(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(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 update_ocsp_ssl_ctx(SSL_CTX *ssl_ctx) { +void ConnectionHandler::cancel_ocsp_update() { + 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(get_config()->fetch_ocsp_response_file.get()), + const_cast(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 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_CTX_get_app_data(ssl_ctx)); - auto cert_file = tls_ctx_data->cert_file; - - std::vector out; - if (ssl::get_ocsp_response(out, cert_file) != 0) { - LOG(WARN) << "ocsp update for " << cert_file << " failed"; - return; - } if (LOG_ENABLED(INFO)) { - LOG(INFO) << "ocsp update for " << cert_file << " finished successfully"; + LOG(INFO) << "ocsp update for " << tls_ctx_data->cert_file + << " finished successfully"; } - std::lock_guard 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); + { + std::lock_guard g(tls_ctx_data->mu); + tls_ctx_data->ocsp_data = std::move(ocsp_.resp); } + + ++ocsp_.next; + proceed_next_cert_ocsp(); } -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); - } - }); +void ConnectionHandler::reset_ocsp() { + if (ocsp_.fd != -1) { + close(ocsp_.fd); + } - update_ocsp(); - }); -#endif // !NOTHREADS + ocsp_.fd = -1; + ocsp_.pid = 0; + ocsp_.error = 0; + ocsp_.resp = std::vector(); } -void ConnectionHandler::handle_ocsp_completion() { -#ifndef NOTHREADS - if (!ocsp_result_.valid()) { - return; +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_CTX_get_app_data(ssl_ctx)); + auto cert_file = tls_ctx_data->cert_file; + + if (start_ocsp_update(cert_file) != 0) { + ++ocsp_.next; + continue; + } + + break; } - - 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 diff --git a/src/shrpx_connection_handler.h b/src/shrpx_connection_handler.h index c5f6e80b..d8c328c5 100644 --- a/src/shrpx_connection_handler.h +++ b/src/shrpx_connection_handler.h @@ -32,9 +32,6 @@ #include #include -#ifndef NOTHREADS -#include -#endif // !NOTHREADS #include @@ -51,6 +48,22 @@ class Worker; struct WorkerStat; struct TicketKeys; +struct OCSPUpdateContext { + // ocsp response buffer + std::vector 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(); - // Just like update_ocsp(), but performed in new thread. Call - // handle_ocsp_completion() to handle its completion and scheduling - // next update. - void update_ocsp_async(); - // Handles asynchronous OCSP update completion and schedules next + + // Cancels ocsp update process + void cancel_ocsp_update(); + // Starts ocsp update for certficate |cert_file|. + int start_ocsp_update(const char *cert_file); + // Reads incoming data from ocsp update process + 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(); - // Waits for OCSP thread finishes if it is still running. - void join_ocsp_thread(); + void proceed_next_cert_ocsp(); private: -#ifndef NOTHREADS - std::future ocsp_result_; -#endif // !NOTHREADS std::vector all_ssl_ctx_; + OCSPUpdateContext ocsp_; // Worker instances when multi threaded mode (-nN, N >= 2) is used. std::vector> workers_; // Worker instance used when single threaded mode (-n1) is used. diff --git a/src/shrpx_ssl.cc b/src/shrpx_ssl.cc index d7a55a97..cb27749a 100644 --- a/src/shrpx_ssl.cc +++ b/src/shrpx_ssl.cc @@ -29,10 +29,6 @@ #include #include #include -#include -#ifndef NOTHREADS -#include -#endif // !NOTHREADS #include #include @@ -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 &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 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 &out, const char *cert_file) { - char *const argv[] = { - const_cast(get_config()->fetch_ocsp_response_file.get()), - const_cast(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 diff --git a/src/shrpx_ssl.h b/src/shrpx_ssl.h index 8888723c..5f808cb2 100644 --- a/src/shrpx_ssl.h +++ b/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 &out, const char *cert_file); - } // namespace ssl } // namespace shrpx From 09c485e71258721ba6c792289087541d8b3c4ddb Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 9 Apr 2015 00:43:03 +0900 Subject: [PATCH 2/4] nghttpx: Eliminate 1 second refresh timer --- src/shrpx.cc | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/src/shrpx.cc b/src/shrpx.cc index 20b9456b..d7bd884b 100644 --- a/src/shrpx.cc +++ b/src/shrpx.cc @@ -555,7 +555,9 @@ void graceful_shutdown_signal_cb(struct ev_loop *loop, ev_signal *w, conn_handler->graceful_shutdown_worker(); - if (get_config()->num_worker == 1) { + if (get_config()->num_worker == 1 && + conn_handler->get_single_worker()->get_worker_stat()->num_connections > + 0) { return; } @@ -565,20 +567,6 @@ void graceful_shutdown_signal_cb(struct ev_loop *loop, ev_signal *w, } } // namespace -namespace { -void refresh_cb(struct ev_loop *loop, ev_timer *w, int revents) { - auto conn_handler = static_cast(w->data); - auto worker = conn_handler->get_single_worker(); - - // In multi threaded mode (get_config()->num_worker > 1), we have to - // wait for event notification to workers to finish. - if (get_config()->num_worker == 1 && conn_handler->get_graceful_shutdown() && - (!worker || worker->get_worker_stat()->num_connections == 0)) { - ev_break(loop); - } -} -} // namespace - namespace { void renew_ticket_key_cb(struct ev_loop *loop, ev_timer *w, int revents) { auto conn_handler = static_cast(w->data); @@ -740,11 +728,6 @@ int event_loop() { graceful_shutdown_sig.data = conn_handler.get(); ev_signal_start(loop, &graceful_shutdown_sig); - ev_timer refresh_timer; - ev_timer_init(&refresh_timer, refresh_cb, 0., 1.); - refresh_timer.data = conn_handler.get(); - ev_timer_again(loop, &refresh_timer); - if (!get_config()->upstream_no_tls && !get_config()->no_ocsp) { conn_handler->proceed_next_cert_ocsp(); } From bc53c8161603518360bbc73803519de5391823a9 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 9 Apr 2015 00:55:22 +0900 Subject: [PATCH 3/4] nghttpx: Replace posix_spawn functions with fork + dup2 + execve Although posx_spawn is very convenient and useful, we have platform which don't have these functions (e.g., Android NDK r10d). --- src/shrpx_connection_handler.cc | 43 +++++++++++++++------------------ 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/src/shrpx_connection_handler.cc b/src/shrpx_connection_handler.cc index 1021929d..c325e1ae 100644 --- a/src/shrpx_connection_handler.cc +++ b/src/shrpx_connection_handler.cc @@ -30,9 +30,6 @@ #include #include -#ifndef NOTHREADS -#include -#endif // !NOTHREADS #include "shrpx_client_handler.h" #include "shrpx_ssl.h" @@ -392,36 +389,34 @@ int ConnectionHandler::start_ocsp_update(const char *cert_file) { } }); - // 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) { + auto pid = fork(); + if (pid == -1) { + auto error = errno; + LOG(WARN) << "Could not execute ocsp query command: " << argv[0] + << ", fork() failed, errno=" << error; return -1; } - auto file_actions_del = - defer(posix_spawn_file_actions_destroy, &file_actions); + if (pid == 0) { + // child process + dup2(pfd[1], 1); + close(pfd[0]); - 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; + 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); + } + // unreachable } + // parent process close(pfd[1]); pfd[1] = -1; + ocsp_.pid = pid; ocsp_.fd = pfd[0]; pfd[0] = -1; From b8739308025edded01a96f97a73d822dbafc840f Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 9 Apr 2015 00:59:43 +0900 Subject: [PATCH 4/4] nghttpx: Now ocsp works without threads --- src/shrpx.cc | 7 ------- src/shrpx_connection_handler.cc | 3 +-- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/src/shrpx.cc b/src/shrpx.cc index d7bd884b..e225a121 100644 --- a/src/shrpx.cc +++ b/src/shrpx.cc @@ -2015,12 +2015,6 @@ int main(int argc, char **argv) { } if (!get_config()->upstream_no_tls && !get_config()->no_ocsp) { -#ifdef NOTHREADS - mod_config()->no_ocsp = true; - LOG(WARN) - << "OCSP stapling has been disabled since it requires threading but" - "threading disabled at build time."; -#else // !NOTHREADS struct stat buf; if (stat(get_config()->fetch_ocsp_response_file.get(), &buf) != 0) { mod_config()->no_ocsp = true; @@ -2028,7 +2022,6 @@ int main(int argc, char **argv) { << get_config()->fetch_ocsp_response_file.get() << " not found. OCSP stapling has been disabled."; } -#endif // !NOTHREADS } if (get_config()->downstream_addrs.empty()) { diff --git a/src/shrpx_connection_handler.cc b/src/shrpx_connection_handler.cc index c325e1ae..1fdbc867 100644 --- a/src/shrpx_connection_handler.cc +++ b/src/shrpx_connection_handler.cc @@ -355,7 +355,6 @@ void ConnectionHandler::cancel_ocsp_update() { // 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]; @@ -426,7 +425,7 @@ int ConnectionHandler::start_ocsp_update(const char *cert_file) { ev_child_set(&ocsp_.chldev, ocsp_.pid, 0); ev_child_start(loop_, &ocsp_.chldev); -#endif // !NOTHREADS + return 0; }