From fe8946ddc7c081eb0b2f376fc99279121a3f2a8b Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Mon, 16 Sep 2019 22:25:06 +0900 Subject: [PATCH] nghttpx: Fix bug that mruby is incorrectly shared between backends Previously, mruby context is wrongly shared by multiple patterns if the underlying SharedDownstreamAddr is shared by multiple DownstreamAddrGroups. This commit fixes it. --- src/shrpx_downstream.cc | 6 +++--- src/shrpx_http2_upstream.cc | 4 ++-- src/shrpx_https_upstream.cc | 4 ++-- src/shrpx_worker.cc | 31 +++++++++++++++++-------------- src/shrpx_worker.h | 6 +++--- 5 files changed, 27 insertions(+), 24 deletions(-) diff --git a/src/shrpx_downstream.cc b/src/shrpx_downstream.cc index 5e27fde0..dc7762d0 100644 --- a/src/shrpx_downstream.cc +++ b/src/shrpx_downstream.cc @@ -193,7 +193,7 @@ Downstream::~Downstream() { if (dconn_) { const auto &group = dconn_->get_downstream_addr_group(); if (group) { - const auto &mruby_ctx = group->mruby_ctx; + const auto &mruby_ctx = group->shared_addr->mruby_ctx; mruby_ctx->delete_downstream(this); } } @@ -231,7 +231,7 @@ void Downstream::detach_downstream_connection() { #ifdef HAVE_MRUBY const auto &group = dconn_->get_downstream_addr_group(); if (group) { - const auto &mruby_ctx = group->mruby_ctx; + const auto &mruby_ctx = group->shared_addr->mruby_ctx; mruby_ctx->delete_downstream(this); } #endif // HAVE_MRUBY @@ -256,7 +256,7 @@ std::unique_ptr Downstream::pop_downstream_connection() { const auto &group = dconn_->get_downstream_addr_group(); if (group) { - const auto &mruby_ctx = group->mruby_ctx; + const auto &mruby_ctx = group->shared_addr->mruby_ctx; mruby_ctx->delete_downstream(this); } #endif // HAVE_MRUBY diff --git a/src/shrpx_http2_upstream.cc b/src/shrpx_http2_upstream.cc index f19c65e4..9b9e5523 100644 --- a/src/shrpx_http2_upstream.cc +++ b/src/shrpx_http2_upstream.cc @@ -492,7 +492,7 @@ void Http2Upstream::initiate_downstream(Downstream *downstream) { #ifdef HAVE_MRUBY const auto &group = dconn_ptr->get_downstream_addr_group(); if (group) { - const auto &mruby_ctx = group->mruby_ctx; + const auto &mruby_ctx = group->shared_addr->mruby_ctx; if (mruby_ctx->run_on_request_proc(downstream) != 0) { if (error_reply(downstream, 500) != 0) { rst_stream(downstream, NGHTTP2_INTERNAL_ERROR); @@ -1665,7 +1665,7 @@ int Http2Upstream::on_downstream_header_complete(Downstream *downstream) { auto dconn = downstream->get_downstream_connection(); const auto &group = dconn->get_downstream_addr_group(); if (group) { - const auto &dmruby_ctx = group->mruby_ctx; + const auto &dmruby_ctx = group->shared_addr->mruby_ctx; if (dmruby_ctx->run_on_response_proc(downstream) != 0) { if (error_reply(downstream, 500) != 0) { diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc index e4e5fbc2..34a5504b 100644 --- a/src/shrpx_https_upstream.cc +++ b/src/shrpx_https_upstream.cc @@ -481,7 +481,7 @@ int htp_hdrs_completecb(llhttp_t *htp) { #ifdef HAVE_MRUBY const auto &group = dconn_ptr->get_downstream_addr_group(); if (group) { - const auto &dmruby_ctx = group->mruby_ctx; + const auto &dmruby_ctx = group->shared_addr->mruby_ctx; if (dmruby_ctx->run_on_request_proc(downstream) != 0) { resp.http_status = 500; @@ -1087,7 +1087,7 @@ int HttpsUpstream::on_downstream_header_complete(Downstream *downstream) { assert(dconn); const auto &group = dconn->get_downstream_addr_group(); if (group) { - const auto &dmruby_ctx = group->mruby_ctx; + const auto &dmruby_ctx = group->shared_addr->mruby_ctx; if (dmruby_ctx->run_on_response_proc(downstream) != 0) { error_reply(500); diff --git a/src/shrpx_worker.cc b/src/shrpx_worker.cc index cef4dfc7..cc0254d8 100644 --- a/src/shrpx_worker.cc +++ b/src/shrpx_worker.cc @@ -79,11 +79,12 @@ using DownstreamKey = size_t, Proto, uint32_t, uint32_t, uint32_t, bool, bool, bool, bool>>, bool, SessionAffinity, StringRef, StringRef, - SessionAffinityCookieSecure, int64_t, int64_t>; + SessionAffinityCookieSecure, int64_t, int64_t, StringRef>; namespace { -DownstreamKey create_downstream_key( - const std::shared_ptr &shared_addr) { +DownstreamKey +create_downstream_key(const std::shared_ptr &shared_addr, + const StringRef &mruby_file) { DownstreamKey dkey; auto &addrs = std::get<0>(dkey); @@ -117,6 +118,7 @@ DownstreamKey create_downstream_key( auto &timeout = shared_addr->timeout; std::get<6>(dkey) = timeout.read; std::get<7>(dkey) = timeout.write; + std::get<8>(dkey) = mruby_file; return dkey; } @@ -231,16 +233,6 @@ void Worker::replace_downstream_config( dst = std::make_shared(); dst->pattern = ImmutableString{std::begin(src.pattern), std::end(src.pattern)}; -#ifdef HAVE_MRUBY - auto mruby_ctx_it = shared_mruby_ctxs.find(src.mruby_file); - if (mruby_ctx_it == std::end(shared_mruby_ctxs)) { - dst->mruby_ctx = mruby::create_mruby_context(src.mruby_file); - assert(dst->mruby_ctx); - shared_mruby_ctxs.emplace(src.mruby_file, dst->mruby_ctx); - } else { - dst->mruby_ctx = (*mruby_ctx_it).second; - } -#endif // HAVE_MRUBY auto shared_addr = std::make_shared(); @@ -297,10 +289,21 @@ void Worker::replace_downstream_config( loop_, cl_ssl_ctx_, this, &dst_addr, randgen_); } +#ifdef HAVE_MRUBY + auto mruby_ctx_it = shared_mruby_ctxs.find(src.mruby_file); + if (mruby_ctx_it == std::end(shared_mruby_ctxs)) { + shared_addr->mruby_ctx = mruby::create_mruby_context(src.mruby_file); + assert(shared_addr->mruby_ctx); + shared_mruby_ctxs.emplace(src.mruby_file, shared_addr->mruby_ctx); + } else { + shared_addr->mruby_ctx = (*mruby_ctx_it).second; + } +#endif // HAVE_MRUBY + // share the connection if patterns have the same set of backend // addresses. - auto dkey = create_downstream_key(shared_addr); + auto dkey = create_downstream_key(shared_addr, src.mruby_file); auto it = addr_groups_indexer.find(dkey); if (it == std::end(addr_groups_indexer)) { diff --git a/src/shrpx_worker.h b/src/shrpx_worker.h index 12013e5a..dfa98582 100644 --- a/src/shrpx_worker.h +++ b/src/shrpx_worker.h @@ -206,6 +206,9 @@ struct SharedDownstreamAddr { // Bunch of session affinity hash. Only used if affinity == // SessionAffinity::IP. std::vector affinity_hash; +#ifdef HAVE_MRUBY + std::shared_ptr mruby_ctx; +#endif // HAVE_MRUBY // Configuration for session affinity AffinityConfig affinity; // Session affinity @@ -230,9 +233,6 @@ struct DownstreamAddrGroup { ImmutableString pattern; std::shared_ptr shared_addr; -#ifdef HAVE_MRUBY - std::shared_ptr mruby_ctx; -#endif // HAVE_MRUBY // true if this group is no longer used for new request. If this is // true, the connection made using one of address in shared_addr // must not be pooled.