Fix race in remap table refcount during reload (10.2.x)

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<UrlRewrite *> rewrite_table;
+extern AtomicSharedPtr<UrlRewrite> 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);

include/proxy/http/HttpSM.h

@@ -33,6 +33,7 @@
 
 #include <string_view>
 #include <optional>
+#include <memory>
 
 #include "tscore/ink_platform.h"
 #include "iocore/eventsystem/EventSystem.h"
@@ -306,7 +307,7 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
 
   // 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<UrlRewrite> m_remap;
 
   History<HISTORY_DEFAULT_SIZE> history;
   NetVConnection *

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<std::string, UrlMappingPathIndex *>;
   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
   {

include/tsutil/AtomicSharedPtr.h

@@ -0,0 +1,84 @@
+/** @file
+
+  Atomic wrapper around std::shared_ptr with the C++20
+  std::atomic<std::shared_ptr<T>> 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 <atomic>
+#include <memory>
+
+// Use the C++20 std::atomic<std::shared_ptr<T>> 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 <class T> using AtomicSharedPtr = std::atomic<std::shared_ptr<T>>;
+
+#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 T> class AtomicSharedPtr
+{
+public:
+  AtomicSharedPtr() noexcept = default;
+  AtomicSharedPtr(std::shared_ptr<T> desired) noexcept : _p(std::move(desired)) {}
+
+  AtomicSharedPtr(const AtomicSharedPtr &)            = delete;
+  AtomicSharedPtr &operator=(const AtomicSharedPtr &) = delete;
+
+  std::shared_ptr<T>
+  load(std::memory_order order = std::memory_order_seq_cst) const noexcept
+  {
+    return std::atomic_load_explicit(&_p, order);
+  }
+
+  void
+  store(std::shared_ptr<T> desired, std::memory_order order = std::memory_order_seq_cst) noexcept
+  {
+    std::atomic_store_explicit(&_p, std::move(desired), order);
+  }
+
+  std::shared_ptr<T>
+  exchange(std::shared_ptr<T> 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<T> _p;
+};
+
+#pragma GCC diagnostic pop
+
+#endif

src/api/InkAPI.cc

@@ -5068,7 +5068,7 @@ TSHttpTxnNextHopNamedStrategyGet(TSHttpTxn txnp, const char *name)
 
   auto sm = reinterpret_cast<HttpSM const *>(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

src/proxy/ReverseProxy.cc

@@ -29,8 +29,10 @@
 
 #include "tscore/ink_platform.h"
 #include "tscore/Filenames.h"
+#include "tscore/TSSystemState.h"
 #include <dlfcn.h>
 #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<ProxyMutex> reconfig_mutex;
 
 DbgCtl dbg_ctl_url_rewrite{"url_rewrite"};
 
+// Steers UrlRewriteDeleter to inline-delete; see shutdown_url_rewrite().
+std::atomic<bool> 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<UrlRewrite *>         rewrite_table       = nullptr;
+AtomicSharedPtr<UrlRewrite>       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<UrlRewrite>();
 
   // 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<UrlRewrite>(initial_table.release(), UrlRewriteDeleter{}), std::memory_order_release);
   RecRegisterConfigUpdateCb("proxy.config.reverse_proxy.enabled", url_rewrite_CB, (void *)REVERSE_CHANGED);
 
   return 0;
@@ -145,33 +179,27 @@ 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<UrlRewrite>();
 
   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<UrlRewrite>(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);
     return true;
   } 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;
 }

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