From 49bb7ba4703141f81e39d40fb4cb79388155cad2 Mon Sep 17 00:00:00 2001 From: Leif Hedstrom Date: Wed, 27 May 2026 16:43:58 -0600 Subject: [PATCH 1/2] Fix race in remap table refcount during reload The global remap table's load and acquire ran as two unsynchronized steps, while config reload swapped the table and immediately released the old one without a mutex. A reader preempted between load and acquire could revive a table whose refcount the reload had just driven to zero, after the deleter was already scheduled. Retire the bespoke acquire/release refcount on UrlRewrite and let std::atomic> (via the new AtomicSharedPtr helper) own the publish-and-replace. Each transaction snapshots the current table into HttpSM::m_remap on session start; reload exchange()s in a new shared_ptr and drops its ref, so the old table destructs only after the last in-flight HttpSM releases its snapshot. Add shutdown_url_rewrite() to drain and inhibit further drops so plugin doneInstance() runs while this_ethread() is still valid. Co-authored-by: Masaori Koshiba --- include/proxy/ReverseProxy.h | 12 +++- include/proxy/http/HttpSM.h | 3 +- include/proxy/http/remap/UrlRewrite.h | 25 +------- include/tsutil/AtomicSharedPtr.h | 84 +++++++++++++++++++++++++++ src/api/InkAPI.cc | 2 +- src/proxy/ReverseProxy.cc | 76 ++++++++++++++++-------- src/proxy/http/HttpSM.cc | 19 +++--- src/proxy/http/HttpTransact.cc | 4 +- src/traffic_server/traffic_server.cc | 3 + 9 files changed, 162 insertions(+), 66 deletions(-) create mode 100644 include/tsutil/AtomicSharedPtr.h diff --git a/include/proxy/ReverseProxy.h b/include/proxy/ReverseProxy.h index 201327d8aa2..de18033c8d5 100644 --- a/include/proxy/ReverseProxy.h +++ b/include/proxy/ReverseProxy.h @@ -43,12 +43,14 @@ #include "proxy/http/remap/UrlMapping.h" #include "mgmt/config/ConfigContext.h" +#include "tsutil/AtomicSharedPtr.h" + #define EMPTY_PORT_MAPPING (int32_t) ~0 class url_mapping; struct host_hdr_info; -extern std::atomic rewrite_table; +extern AtomicSharedPtr rewrite_table; // API Functions int init_reverse_proxy(); @@ -61,4 +63,10 @@ bool reloadUrlRewrite(ConfigContext ctx); bool urlRewriteVerify(); void init_remap_volume_host_records(); -int url_rewrite_CB(const char *name, RecDataT data_type, RecData data, void *cookie); + +// Synchronously drops rewrite_table. Call from a Continuation context +// before TSSystemState::shut_down_event_system() so plugin doneInstance() +// has this_ethread() for TSMutexLock. +void shutdown_url_rewrite(); + +int url_rewrite_CB(const char *name, RecDataT data_type, RecData data, void *cookie); diff --git a/include/proxy/http/HttpSM.h b/include/proxy/http/HttpSM.h index fc3e1252452..802fb94aa70 100644 --- a/include/proxy/http/HttpSM.h +++ b/include/proxy/http/HttpSM.h @@ -33,6 +33,7 @@ #include #include +#include #include "tscore/ink_platform.h" #include "iocore/eventsystem/EventSystem.h" @@ -306,7 +307,7 @@ class HttpSM : public Continuation, public PluginUserArgs // This unfortunately can't go into the t_state, because of circular dependencies. We could perhaps refactor // this, with a lot of work, but this is easier for now. - UrlRewrite *m_remap = nullptr; + std::shared_ptr m_remap; History history; NetVConnection * diff --git a/include/proxy/http/remap/UrlRewrite.h b/include/proxy/http/remap/UrlRewrite.h index cfe46817d02..e2c3604b554 100644 --- a/include/proxy/http/remap/UrlRewrite.h +++ b/include/proxy/http/remap/UrlRewrite.h @@ -24,7 +24,6 @@ #pragma once -#include "iocore/eventsystem/Freer.h" #include "mgmt/config/ConfigContext.h" #include "proxy/http/remap/UrlMapping.h" #include "proxy/http/remap/UrlMappingPathIndex.h" @@ -57,12 +56,12 @@ enum class mapping_type { /** * **/ -class UrlRewrite : public RefCountObjInHeap +class UrlRewrite { public: using URLTable = std::unordered_map; UrlRewrite() = default; - ~UrlRewrite() override; + ~UrlRewrite(); /** Retrieve the configured ACL matching policy. * @@ -93,26 +92,6 @@ class UrlRewrite : public RefCountObjInHeap void SetReverseFlag(int flag); void Print() const; - // The UrlRewrite object is-a RefCountObj, but this is a convenience to make it clear that we - // don't delete() these objects directly, but via the release() method only. - UrlRewrite * - acquire() - { - this->refcount_inc(); - return this; - } - - void - release() - { - if (0 == this->refcount_dec()) { - // Delete this on an ET_TASK thread, which avoids doing potentially slow things on an ET_NET thread. - static DbgCtl dc{"url_rewrite"}; - Dbg(dc, "Deleting old configuration immediately"); - new_Deleter(this, 0); - } - } - bool is_valid() const { diff --git a/include/tsutil/AtomicSharedPtr.h b/include/tsutil/AtomicSharedPtr.h new file mode 100644 index 00000000000..c7138a8e575 --- /dev/null +++ b/include/tsutil/AtomicSharedPtr.h @@ -0,0 +1,84 @@ +/** @file + + Atomic wrapper around std::shared_ptr with the C++20 + std::atomic> API. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#pragma once + +#include +#include + +// Use the C++20 std::atomic> specialization when the +// standard library provides it, otherwise fall back to the pre-C++20 +// std::atomic_*_explicit free-function overloads on shared_ptr. The +// fallback exists for libstdc++ < 12 and libc++ < 14, which predate the +// specialization. When those toolchains are no longer supported, delete +// the #else branch and the surrounding #if; call sites do not change. +#if defined(__cpp_lib_atomic_shared_ptr) && __cpp_lib_atomic_shared_ptr >= 201711L + +template using AtomicSharedPtr = std::atomic>; + +#else + +// Belt-and-suspenders: on the toolchains that take this branch (libstdc++ +// < 12, libc++ < 16) the free-function overloads are not yet marked +// [[deprecated]], so the suppression below is usually a no-op. It +// matters only if someone forces the fallback on a modern library (e.g. +// -D__cpp_lib_atomic_shared_ptr=0) or compiles against a library that +// ships the deprecation markers ahead of the specialization. +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" + +template class AtomicSharedPtr +{ +public: + AtomicSharedPtr() noexcept = default; + AtomicSharedPtr(std::shared_ptr desired) noexcept : _p(std::move(desired)) {} + + AtomicSharedPtr(const AtomicSharedPtr &) = delete; + AtomicSharedPtr &operator=(const AtomicSharedPtr &) = delete; + + std::shared_ptr + load(std::memory_order order = std::memory_order_seq_cst) const noexcept + { + return std::atomic_load_explicit(&_p, order); + } + + void + store(std::shared_ptr desired, std::memory_order order = std::memory_order_seq_cst) noexcept + { + std::atomic_store_explicit(&_p, std::move(desired), order); + } + + std::shared_ptr + exchange(std::shared_ptr desired, std::memory_order order = std::memory_order_seq_cst) noexcept + { + return std::atomic_exchange_explicit(&_p, std::move(desired), order); + } + +private: + std::shared_ptr _p; +}; + +#pragma GCC diagnostic pop + +#endif diff --git a/src/api/InkAPI.cc b/src/api/InkAPI.cc index 5500b44a6c8..0cfcdfa0f10 100644 --- a/src/api/InkAPI.cc +++ b/src/api/InkAPI.cc @@ -5068,7 +5068,7 @@ TSHttpTxnNextHopNamedStrategyGet(TSHttpTxn txnp, const char *name) auto sm = reinterpret_cast(txnp); - sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap) == TS_SUCCESS); + sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap.get()) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap->strategyFactory) == TS_SUCCESS); // HttpSM has a reference count handle to UrlRewrite which has a diff --git a/src/proxy/ReverseProxy.cc b/src/proxy/ReverseProxy.cc index 8368ad515e8..8600013624f 100644 --- a/src/proxy/ReverseProxy.cc +++ b/src/proxy/ReverseProxy.cc @@ -29,8 +29,10 @@ #include "tscore/ink_platform.h" #include "tscore/Filenames.h" +#include "tscore/TSSystemState.h" #include #include "iocore/cache/Cache.h" +#include "iocore/eventsystem/Freer.h" #include "proxy/ReverseProxy.h" #include "mgmt/config/ConfigContextDiags.h" #include "mgmt/config/ConfigRegistry.h" @@ -49,12 +51,44 @@ Ptr reconfig_mutex; DbgCtl dbg_ctl_url_rewrite{"url_rewrite"}; +// Steers UrlRewriteDeleter to inline-delete; see shutdown_url_rewrite(). +std::atomic rewrite_table_shutdown{false}; + +// Defer teardown to ET_TASK; UrlRewrite destruction can be slow. +struct UrlRewriteDeleter { + void + operator()(UrlRewrite *p) const noexcept + { + if (!p) { + return; + } + if (rewrite_table_shutdown.load(std::memory_order_acquire) || TSSystemState::is_event_system_shut_down()) { + // Leak; plugin teardown is unsafe post-shutdown. + return; + } + // new_Deleter allocates; fall back to inline delete so we don't escape noexcept. + try { + new_Deleter(p, 0); + } catch (...) { + delete p; + } + } +}; + } // end anonymous namespace // Global Ptrs -std::atomic rewrite_table = nullptr; +AtomicSharedPtr rewrite_table; thread_local PluginThreadContext *pluginThreadContext = nullptr; +void +shutdown_url_rewrite() +{ + // Drain before flag: this ref destructs normally; later drops leak. + rewrite_table.exchange(nullptr); + rewrite_table_shutdown.store(true, std::memory_order_release); +} + // Tokens for the Callback function #define FILE_CHANGED 0 #define REVERSE_CHANGED 1 @@ -71,10 +105,9 @@ static void init_table_volume_host_records(UrlRewrite &table); int init_reverse_proxy() { - ink_assert(rewrite_table.load() == nullptr); - reconfig_mutex = new_ProxyMutex(); - auto *initial_table = new UrlRewrite(); - initial_table->acquire(); + ink_assert(rewrite_table.load(std::memory_order_acquire) == nullptr); + reconfig_mutex = new_ProxyMutex(); + auto initial_table = std::make_unique(); // Register with ConfigRegistry BEFORE load() so that remap.config is in // FileManager's bindings when .include directives call configFileChild() @@ -100,7 +133,8 @@ init_reverse_proxy() init_table_volume_host_records(*initial_table); } - rewrite_table.store(initial_table, std::memory_order_release); + // Publish: shared_ptr semantics replace the prior bespoke acquire()/release() refcount on UrlRewrite. + rewrite_table.store(std::shared_ptr(initial_table.release(), UrlRewriteDeleter{}), std::memory_order_release); RecRegisterConfigUpdateCb("proxy.config.reverse_proxy.enabled", url_rewrite_CB, (void *)REVERSE_CHANGED); return 0; @@ -145,25 +179,19 @@ reloadUrlRewrite(ConfigContext ctx) std::string msg_buffer; msg_buffer.reserve(1024); - UrlRewrite *newTable, *oldTable; CfgLoadLog(ctx, DL_Note, "%s loading ...", ts::filename::REMAP); Dbg(dbg_ctl_url_rewrite, "%s updated, reloading...", ts::filename::REMAP); - newTable = new UrlRewrite(); + auto newTable = std::make_unique(); if (newTable->load(ctx)) { swoc::bwprint(msg_buffer, "{} finished loading", ts::filename::REMAP); - // Hold at least one lease, until we reload the configuration - newTable->acquire(); - - // Swap configurations - oldTable = rewrite_table.exchange(newTable); - - ink_assert(oldTable != nullptr); - - // Release the old one - oldTable->release(); + // Atomic publish: an old reader's shared_ptr keeps the prior table alive until its last + // ref is dropped; new readers see the new table. The prior race between load() and + // acquire() on the bespoke refcount cannot revive a table whose refcount was driven to + // zero, because there is no separate refcount. + rewrite_table.exchange(std::shared_ptr(newTable.release(), UrlRewriteDeleter{}), std::memory_order_acq_rel); Dbg(dbg_ctl_url_rewrite, "%s", msg_buffer.c_str()); CfgLoadComplete(ctx, "%s finished loading", ts::filename::REMAP); @@ -171,7 +199,7 @@ reloadUrlRewrite(ConfigContext ctx) } else { swoc::bwprint(msg_buffer, "{} failed to load", ts::filename::REMAP); - delete newTable; + // newTable is a unique_ptr; falling out of scope deletes it. Dbg(dbg_ctl_url_rewrite, "%s", msg_buffer.c_str()); CfgLoadFail(ctx, "%s failed to load", ts::filename::REMAP); return false; @@ -233,25 +261,23 @@ init_remap_volume_host_records() return; } - UrlRewrite *table = rewrite_table.load(std::memory_order_acquire); + auto table = rewrite_table.load(std::memory_order_acquire); if (!table) { return; } - table->acquire(); - if (table->is_valid()) { init_table_volume_host_records(*table); } - - table->release(); } int url_rewrite_CB(const char * /* name ATS_UNUSED */, RecDataT /* data_type ATS_UNUSED */, RecData data, void * /* cookie ATS_UNUSED */) { - rewrite_table.load()->SetReverseFlag(data.rec_int); + if (auto table = rewrite_table.load(std::memory_order_acquire); table != nullptr) { + table->SetReverseFlag(data.rec_int); + } return 0; } diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index 9a623ffa9e4..4a07eef4b79 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -285,13 +285,7 @@ HttpSM::~HttpSM() // coverity[exn_spec_violation] - release() only does ref counting and delete on POD types HttpConfig::release(t_state.http_config_param); - - // m_remap->release() can allocate (new_Deleter), so catch potential bad_alloc - try { - m_remap->release(); - } catch (...) { - Error("Exception in ~HttpSM during m_remap->release"); - } + m_remap.reset(); // coverity[exn_spec_violation] - cancel_pending_action() cancels pending cache work and clears tracked pointers cache_sm.cancel_pending_action(); @@ -334,8 +328,9 @@ HttpSM::init(bool from_early_data) t_state.state_machine = this; t_state.http_config_param = HttpConfig::acquire(); - // Acquire a lease on the global remap / rewrite table (stupid global name ...) - m_remap = rewrite_table.load()->acquire(); + // Snapshot the global remap / rewrite table. shared_ptr keeps it alive across the txn + // even if reload swaps the global pointer concurrently. + m_remap = rewrite_table.load(std::memory_order_acquire); // Simply point to the global config for the time being, no need to copy this // entire struct if nothing is going to change it. @@ -4517,7 +4512,7 @@ HttpSM::state_remap_request(int event, void * /* data ATS_UNUSED */) case EVENT_REMAP_COMPLETE: { pending_action = nullptr; SMDbg(dbg_ctl_url_rewrite, "completed processor-based remapping request"); - t_state.url_remap_success = remapProcessor.finish_remap(&t_state, m_remap); + t_state.url_remap_success = remapProcessor.finish_remap(&t_state, m_remap.get()); call_transact_and_set_next_state(nullptr); break; } @@ -4594,7 +4589,7 @@ HttpSM::do_remap_request(bool run_inline) { SMDbg(dbg_ctl_http_seq, "Remapping request"); SMDbg(dbg_ctl_url_rewrite, "Starting a possible remapping for request"); - bool ret = remapProcessor.setup_for_remap(&t_state, m_remap); + bool ret = remapProcessor.setup_for_remap(&t_state, m_remap.get()); check_sni_host(); @@ -8182,7 +8177,7 @@ HttpSM::set_next_state() case HttpTransact::StateMachineAction_t::REMAP_REQUEST: { do_remap_request(true); /* run inline */ SMDbg(dbg_ctl_url_rewrite, "completed inline remapping request"); - t_state.url_remap_success = remapProcessor.finish_remap(&t_state, m_remap); + t_state.url_remap_success = remapProcessor.finish_remap(&t_state, m_remap.get()); if (t_state.next_action == HttpTransact::StateMachineAction_t::SEND_ERROR_CACHE_NOOP && t_state.transact_return_point == nullptr) { // It appears that we can now set the next_action to error and transact_return_point to nullptr when diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc index 8180f845a22..5c0bf0f5172 100644 --- a/src/proxy/http/HttpTransact.cc +++ b/src/proxy/http/HttpTransact.cc @@ -1534,7 +1534,7 @@ HttpTransact::handleIfRedirect(State *s) mapping_type answer; URL redirect_url; - answer = request_url_remap_redirect(&s->hdr_info.client_request, &redirect_url, s->state_machine->m_remap); + answer = request_url_remap_redirect(&s->hdr_info.client_request, &redirect_url, s->state_machine->m_remap.get()); if ((answer == mapping_type::PERMANENT_REDIRECT) || (answer == mapping_type::TEMPORARY_REDIRECT)) { s->remap_redirect = redirect_url.string_get_ref(nullptr); if (answer == mapping_type::TEMPORARY_REDIRECT) { @@ -8239,7 +8239,7 @@ HttpTransact::build_response(State *s, HTTPHdr *base_response, HTTPHdr *outgoing // process reverse mappings on the location header // TS-1364: do this regardless of response code - response_url_remap(outgoing_response, s->state_machine->m_remap); + response_url_remap(outgoing_response, s->state_machine->m_remap.get()); if (s->http_config_param->enable_http_stats) { HttpTransactHeaders::generate_and_set_squid_codes(outgoing_response, s->via_string, &s->squid_codes); diff --git a/src/traffic_server/traffic_server.cc b/src/traffic_server/traffic_server.cc index 241c3850ca3..76e9c62cd95 100644 --- a/src/traffic_server/traffic_server.cc +++ b/src/traffic_server/traffic_server.cc @@ -92,6 +92,7 @@ extern "C" int plock(int); #include "records/RecordsConfig.h" #include "iocore/eventsystem/RecProcess.h" #include "proxy/Transform.h" +#include "proxy/ReverseProxy.h" #include "iocore/eventsystem/ConfigProcessor.h" #include "mgmt/config/ConfigContextDiags.h" #include "mgmt/config/ConfigRegistry.h" @@ -296,6 +297,8 @@ struct AutoStopCont : public Continuation { // Push buffered log entries into the preproc queue before shutdown. Log::flush_all_objects(); + shutdown_url_rewrite(); + TSSystemState::shut_down_event_system(); // Wake preproc threads to drain remaining log buffers before exit. From 7c8842ffccfb258c50518107e0bcc70725546351 Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Tue, 16 Jun 2026 22:18:21 -0500 Subject: [PATCH 2/2] Guard remap against shutdown table teardown Late shutdown can drop the global remap table before all net-thread work has stopped accepting or initializing transactions. A transaction created in that window can reach remap with an empty table lease and crash while dereferencing it. Addresses the following crash: ``` (gdb) bt #0 RemapProcessor::setup_for_remap (this=, s=0x7f84ff3a2100, table=0x0) at /src/proxy/http/remap/RemapProcessor.cc:46 Backtrace stopped: Cannot access memory at address 0x7f9ba11f8418 (gdb) l 41 RemapProcessor::setup_for_remap(HttpTransact::State *s, UrlRewrite *table) 42 { 43 Dbg(dbg_ctl_url_rewrite, "setting up for remap: %p", s); 44 URL *request_url = nullptr; 45 bool mapping_found = false; 46 HTTPHdr *request_header = &s->hdr_info.client_request; 47 char **redirect_url = &s->remap_redirect; 48 const char *request_host; 49 int request_host_len; 50 int request_port; (gdb) ``` This keeps the shutdown-window null table as a quiet defensive remap miss. The guard runs before setup or finish dereferences the table, leaves in-flight transactions that already hold a remap lease untouched, and avoids warning or metric churn for a condition expected only while the process exits. This intentionally leaves shutdown admission ordering unchanged. A broader admission-gate fix can be evaluated separately from this cheap consumer backstop. --- src/proxy/http/remap/RemapProcessor.cc | 16 ++++++++++++++-- src/proxy/http/unit_tests/test_HttpTransact.cc | 10 ++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/proxy/http/remap/RemapProcessor.cc b/src/proxy/http/remap/RemapProcessor.cc index 550af9360e6..22cfddff932 100644 --- a/src/proxy/http/remap/RemapProcessor.cc +++ b/src/proxy/http/remap/RemapProcessor.cc @@ -50,8 +50,13 @@ RemapProcessor::setup_for_remap(HttpTransact::State *s, UrlRewrite *table) int request_port; bool proxy_request = false; - s->reverse_proxy = table->reverse_proxy; s->url_map.set(s->hdr_info.client_request.m_heap); + if (unlikely(table == nullptr)) { + Dbg(dbg_ctl_url_rewrite, "no remap table (shutdown in progress); skipping remap"); + return false; + } + + s->reverse_proxy = table->reverse_proxy; ink_assert(redirect_url != nullptr); @@ -158,13 +163,20 @@ RemapProcessor::finish_remap(HttpTransact::State *s, UrlRewrite *table) { url_mapping *map = nullptr; HTTPHdr *request_header = &s->hdr_info.client_request; - URL *request_url = request_header->url_get(); + URL *request_url = nullptr; char **redirect_url = &s->remap_redirect; char tmp_referer_buf[4096], tmp_redirect_buf[4096], tmp_buf[2048]; int tmp; int from_len; referer_info *ri; + if (unlikely(table == nullptr)) { + Dbg(dbg_ctl_url_rewrite, "no remap table (shutdown in progress); skipping remap completion"); + return false; + } + + request_url = request_header->url_get(); + map = s->url_map.getMapping(); if (nullptr == map) { Dbg(dbg_ctl_url_rewrite, "Could not find corresponding url_mapping for this transaction"); diff --git a/src/proxy/http/unit_tests/test_HttpTransact.cc b/src/proxy/http/unit_tests/test_HttpTransact.cc index af283fb6017..ce4c07e0f81 100644 --- a/src/proxy/http/unit_tests/test_HttpTransact.cc +++ b/src/proxy/http/unit_tests/test_HttpTransact.cc @@ -31,6 +31,7 @@ using namespace std::string_view_literals; #include "tsutil/PostScript.h" #include "proxy/http/HttpTransact.h" +#include "proxy/http/remap/RemapProcessor.h" #include "records/RecordsConfig.h" #include @@ -41,6 +42,15 @@ TEST_CASE("HttpTransact", "[http]") mime_init(); http_init(); + SECTION("RemapProcessor tolerates a missing remap table") + { + HttpTransact::State state; + RemapProcessor processor; + + CHECK_FALSE(processor.setup_for_remap(&state, nullptr)); + CHECK_FALSE(processor.finish_remap(&state, nullptr)); + } + SECTION("HttpTransact::merge_response_header_with_cached_header") { SECTION("Basic")