Fix race in remap table refcount during reload (10.2.x) - #13454
Closed
cmcfarlen wants to merge 2 commits into
Closed
Fix race in remap table refcount during reload (10.2.x)#13454cmcfarlen wants to merge 2 commits into
cmcfarlen wants to merge 2 commits into
Conversation
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<std::shared_ptr<UrlRewrite>> (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 <masaori@apache.org>
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=<optimized out>, 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.
Contributor
There was a problem hiding this comment.
Pull request overview
Backports the remap-table refcount race fix to 10.2.x by replacing the bespoke UrlRewrite::acquire()/release() lifecycle with an atomic publish/snapshot model using std::shared_ptr, ensuring the remap table stays alive for in-flight transactions across config reloads and shutdown.
Changes:
- Replace the global remap table pointer/refcount with
AtomicSharedPtr<UrlRewrite>and per-transactionstd::shared_ptrsnapshots (HttpSM::m_remap). - Add shutdown sequencing (
shutdown_url_rewrite()) and makeRemapProcessortolerate a missing remap table during the shutdown window. - Add a unit test asserting
RemapProcessorreturns a clean “miss” when invoked with a null remap table.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/traffic_server/traffic_server.cc | Calls shutdown_url_rewrite() during shutdown before flagging event-system shutdown. |
| src/proxy/ReverseProxy.cc | Implements atomic shared_ptr publish/exchange for the global remap table and custom deleter behavior. |
| src/proxy/http/unit_tests/test_HttpTransact.cc | Adds coverage for null-table behavior in RemapProcessor. |
| src/proxy/http/remap/RemapProcessor.cc | Adds null-table guards to avoid crashes during shutdown. |
| src/proxy/http/HttpTransact.cc | Updates remap-related call sites to pass raw pointers from shared_ptr snapshots. |
| src/proxy/http/HttpSM.cc | Snapshots the global remap table into HttpSM::m_remap and drops it via shared_ptr semantics. |
| src/api/InkAPI.cc | Adjusts sanity-check to reflect m_remap now being a shared_ptr. |
| include/tsutil/AtomicSharedPtr.h | Adds AtomicSharedPtr compatibility wrapper for toolchains lacking atomic<shared_ptr> specialization. |
| include/proxy/ReverseProxy.h | Exposes AtomicSharedPtr<UrlRewrite> rewrite_table and declares shutdown_url_rewrite(). |
| include/proxy/http/remap/UrlRewrite.h | Removes bespoke refcount API from UrlRewrite. |
| include/proxy/http/HttpSM.h | Changes HttpSM::m_remap to std::shared_ptr<UrlRewrite>. |
Comment on lines
+42
to
+47
| // 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. |
|
|
||
| DbgCtl dbg_ctl_url_rewrite{"url_rewrite"}; | ||
|
|
||
| // Steers UrlRewriteDeleter to inline-delete; see shutdown_url_rewrite(). |
Comment on lines
+67
to
+69
| // Synchronously drops rewrite_table. Call from a Continuation context | ||
| // before TSSystemState::shut_down_event_system() so plugin doneInstance() | ||
| // has this_ethread() for TSMutexLock. |
Contributor
Author
|
Folded into #13452 (Release 2 bundle) — the remap-table refcount race fix belongs with the security bundle it was split from. Superseded by this branch's inclusion; closing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft — CI verification
Backports the remap-table refcount race fix to
10.2.x, adapted to this branch'sconfig-reload code. The fix is on master as
709443e870; it was deferred from theRelease 2 backport (#13452) because master's version is entangled with remap.yaml and
the
ConfigRegistry::attach()API, neither of which is on 10.2.x.The race
The global remap table's
load()andacquire()ran as two unsynchronized steps, whileconfig reload swapped the table and released the old one without a mutex. A reader
preempted between load and acquire could revive a table whose refcount reload had just
driven to zero — after the deleter was already scheduled — a use-after-free.
The fix
acquire()/release()refcount onUrlRewrite; usestd::atomic<std::shared_ptr<UrlRewrite>>(via a newAtomicSharedPtrhelper) forpublish-and-replace.
HttpSM::m_remapat session start;reload
exchange()s in a newshared_ptrand drops its ref, so the old table destructsonly after the last in-flight
HttpSMreleases its snapshot.shutdown_url_rewrite()drains and inhibits further drops so plugindoneInstance()runs while
this_ethread()is still valid.(quiet defensive remap miss), preventing a crash in
RemapProcessor::setup_for_remapwhen a transaction is created after the table is torn down.
10.2.x adaptation
Kept this branch's
register_config(…)+CfgLoad*reload instrumentation and scopedenums; grafted the
AtomicSharedPtrlifecycle onto them.Testing
Local build clean;
ctest157/158 (the one failure is the pre-existing macOStest_jsonrpcserverunix-socket flake, unrelated). Note: the unit tests do not exercisethe reload race itself — a config-reload-under-load check is the meaningful CI signal here.