From f30b33310d5d61040cfeb52b886ef57fd5d007e6 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Wed, 9 Sep 2026 15:43:07 -0700 Subject: [PATCH 01/13] Inherit the shared match context configuration in RegexMatchContext RegexMatchContext built an empty PCRE2 match context, so a caller that supplied its own context silently lost everything the shared context configures. Today that is a 1 MB JIT stack, replaced by PCRE2's 32 KB fallback, which regex_remap and esi both ran on. Building the context as a copy of the shared one makes that divergence structurally impossible and leaves callers overriding only what they intend. The shared context now resolves its JIT stack through a callback rather than assigning one directly, because PCRE2 requires a distinct stack per thread and a copied context can be used on another thread. --- src/tsutil/Regex.cc | 25 +++++++++- src/tsutil/unit_tests/test_Regex.cc | 48 +++++++++++++++++++ .../regex_remap/regex_remap.test.py | 29 +++++++++-- 3 files changed, 95 insertions(+), 7 deletions(-) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index 3281622b1ec..010a2e98bb6 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -79,6 +79,12 @@ my_free(void *ptr, void * /*caller*/) free(ptr); } +//---------------------------------------------------------------------------- +// PCRE2 requires a distinct JIT stack per thread. A match context may be copied +// and used on another thread, so the stack is supplied through a callback that +// resolves to the calling thread's stack rather than assigned directly. +pcre2_jit_stack *thread_jit_stack(void *); + //---------------------------------------------------------------------------- class RegexContext { @@ -119,6 +125,11 @@ class RegexContext { return _match_context; } + pcre2_jit_stack * + get_jit_stack() + { + return _jit_stack; + } private: RegexContext() @@ -127,7 +138,7 @@ class RegexContext _compile_context = pcre2_compile_context_create(_general_context); _match_context = pcre2_match_context_create(_general_context); _jit_stack = pcre2_jit_stack_create(4096, 1024 * 1024, nullptr); // 1 page min and 1MB max - pcre2_jit_stack_assign(_match_context, nullptr, _jit_stack); + pcre2_jit_stack_assign(_match_context, &thread_jit_stack, nullptr); } pcre2_general_context *_general_context = nullptr; pcre2_compile_context *_compile_context = nullptr; @@ -135,6 +146,12 @@ class RegexContext pcre2_jit_stack *_jit_stack = nullptr; }; +pcre2_jit_stack * +thread_jit_stack(void *) +{ + return RegexContext::get_instance()->get_jit_stack(); +} + } // namespace //---------------------------------------------------------------------------- @@ -257,7 +274,11 @@ struct RegexMatchContext::_MatchContext { //---------------------------------------------------------------------------- RegexMatchContext::RegexMatchContext() { - auto ctx = pcre2_match_context_create(nullptr); + // Copy the shared context rather than building a blank one. A blank context + // silently drops everything the shared context configures, which is how this + // type came to run with PCRE2's fallback 32KiB JIT stack instead of the 1MiB + // one every other caller gets. Callers override only the fields they mean to. + auto ctx = pcre2_match_context_copy(RegexContext::get_instance()->get_match_context()); debug_assert_message(ctx, "Failed to allocate custom pcre2 match context"); _MatchContext::set(_match_context, ctx); } diff --git a/src/tsutil/unit_tests/test_Regex.cc b/src/tsutil/unit_tests/test_Regex.cc index f1bd0a7c866..f7e5eb91e5f 100644 --- a/src/tsutil/unit_tests/test_Regex.cc +++ b/src/tsutil/unit_tests/test_Regex.cc @@ -1050,3 +1050,51 @@ TEST_CASE("Regex end-anchor with alternation", "[libts][Regex]") CHECK(r.exec("cdn.example.com.evil.com", matches) == RE_ERROR_NOMATCH); CHECK(r.exec("prefix.cdn.example.com", matches) == RE_ERROR_NOMATCH); } + +// A caller-supplied RegexMatchContext must behave like the shared context that +// Regex::exec uses when none is supplied. A context built from scratch silently +// drops everything the shared one configures, which is how regex_remap came to +// run with PCRE2's fallback 32KiB JIT stack instead of the 1MiB one. +TEST_CASE("RegexMatchContext matches the shared context", "[libts][Regex][RegexMatchContext]") +{ + // Quantified alternation of capture groups: every subject character pushes a + // backtracking frame, so the JIT stack size is what bounds this. + Regex re; + REQUIRE(re.compile(R"(^(?:(a)|(b))+$)")); + + std::string const subject(1000, 'a'); + + RegexMatches shared_matches; + RegexMatchContext match_context; + RegexMatches own_matches; + + int const shared_rc = re.exec(subject, shared_matches); + int const own_rc = re.exec(subject, own_matches, 0, &match_context); + CAPTURE(shared_rc, own_rc); + + REQUIRE(shared_rc > 0); + REQUIRE(own_rc == shared_rc); +} + +// The guard from #5762: a pattern that backtracks once per character must fail +// cleanly rather than run the thread out of stack. PCRE1 recursed on the machine +// stack and a long enough subject crashed the server; PCRE2 must report an error +// instead. If this ever crashes rather than fails, that regression is back. +TEST_CASE("Regex reports resource exhaustion rather than crashing", "[libts][Regex][limits]") +{ + Regex re; + REQUIRE(re.compile(R"(^/alpha/bravo/[?]((?!action=(newsfeed|calendar|contacts|notepad)).)*$)")); + + // Well past what a 1MiB JIT stack can hold, so the bound is still exercised + // now that the plugin no longer sets its own smaller one. + std::string subject{"/alpha/bravo/?"}; + subject.append(2 * 1024 * 1024, 'x'); + + RegexMatches matches; + int const rc = re.exec(subject, matches); + CAPTURE(rc); + + // Reaching this line at all is the crash assertion. + REQUIRE(rc < 0); + REQUIRE(rc != RE_ERROR_NOMATCH); +} diff --git a/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py b/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py index c6c30830127..3f740765407 100644 --- a/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py +++ b/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py @@ -88,7 +88,9 @@ 'proxy.config.diags.debug.enabled': 1, 'proxy.config.diags.debug.tags': 'http|regex_remap', 'proxy.config.dns.nameservers': f"127.0.0.1:{nameserver.Variables.Port}", - 'proxy.config.dns.resolv_conf': 'NULL' + 'proxy.config.dns.resolv_conf': 'NULL', + # The crash-guard run below needs a request larger than the 32 KB default. + 'proxy.config.http.request_header_max_size': 131072 }) # 0 Test - Load cache (miss) (path1) @@ -123,12 +125,29 @@ tr.Processes.Default.Streams.stdout = "gold/regex_remap_simple.gold" tr.StillRunningAfter = ts -# 3 Test - Preserve the original crash guard from #5762. This request must +# 3 Test - A 3 KB query redirects. This rule backtracks once per subject +# character, so it used to exhaust the 32 KB stack PCRE2 falls back to when a +# match context carries none, and the rule was skipped. The plugin's context now +# inherits the shared 1 MB stack, so the rule matches and the redirect fires. +tr = Test.AddTestRun("long query redirects rather than exhausting the JIT stack") +creq = replay_txns[1]['client-request'] +tr.MakeCurlCommand( + curl_and_args + f"--header 'uuid: {creq['headers']['fields'][1][1]}' '{creq['url']}'" + " | grep -e '^HTTP/' -e '^Location'", + ts=ts) +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Streams.stdout = "gold/regex_remap_redirect.gold" +tr.StillRunningAfter = ts + +# 3b Test - Preserve the original crash guard from #5762. This request must # survive resource exhaustion without redirecting, regardless of which matching -# resource limit is reached (JIT stack, match work, depth, or heap). +# resource limit is reached (JIT stack, match work, depth, or heap). Against the +# shared 1 MB stack this rule needs a subject past 43 KB to exhaust it, which is +# why the request header limit is raised above. Shortening this query silently +# turns the run into a plain redirect test. +crash_guard_query = 'x' * 64000 tr = Test.AddTestRun("resource exhaustion does not crash ATS") -creq = replay_txns[1]['client-request'] -tr.MakeCurlCommand(curl_and_args + f"--header 'uuid: {creq['headers']['fields'][1][1]}' '{creq['url']}'", ts=ts) +tr.MakeCurlCommand( + curl_and_args + "--header 'uuid: 180' " + f"'http://example.one/alpha/bravo/?action=newsfed;{crash_guard_query}'", ts=ts) tr.Processes.Default.ReturnCode = 0 tr.Processes.Default.Streams.stdout = "gold/regex_remap_crash.gold" ts.Disk.diags_log.Content += Testers.ContainsExpression( From b10b6ad5476015f9d3acfccb85a40c776db58b7b Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Wed, 9 Sep 2026 16:09:47 -0700 Subject: [PATCH 02/13] Make the JIT stack callback self-contained The callback needed a forward declaration and a getter on RegexContext to reach a stack that RegexContext owned. Owning the thread local stack inside the callback removes both, and removes a member and a destructor branch from RegexContext. The callback form is required rather than stylistic: assigning a stack pointer directly binds the stack of whichever thread built the context, and a copied context can be used on another thread. --- src/tsutil/Regex.cc | 38 +++++++++++++++++--------------------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index 010a2e98bb6..90b1a883d68 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -80,10 +80,22 @@ my_free(void *ptr, void * /*caller*/) } //---------------------------------------------------------------------------- -// PCRE2 requires a distinct JIT stack per thread. A match context may be copied -// and used on another thread, so the stack is supplied through a callback that -// resolves to the calling thread's stack rather than assigned directly. -pcre2_jit_stack *thread_jit_stack(void *); +// PCRE2 needs a distinct JIT stack per thread, and a match context can be copied +// and then used on a different thread. Handing over a plain stack pointer would +// bake in the stack owned by whichever thread built the context, and two threads +// matching at once on one stack corrupts memory. PCRE2 accepts a callback for +// exactly this case and invokes it at match time, on the matching thread. +pcre2_jit_stack * +jit_stack_for_this_thread(void *) +{ + struct ThreadStack { + ThreadStack() : stack{pcre2_jit_stack_create(4096, 1024 * 1024, nullptr)} {} // 1 page min and 1MB max + ~ThreadStack() { pcre2_jit_stack_free(stack); } + pcre2_jit_stack *stack; + }; + thread_local ThreadStack owner; + return owner.stack; +} //---------------------------------------------------------------------------- class RegexContext @@ -106,9 +118,6 @@ class RegexContext if (_match_context != nullptr) { pcre2_match_context_free(_match_context); } - if (_jit_stack != nullptr) { - pcre2_jit_stack_free(_jit_stack); - } } pcre2_general_context * get_general_context() @@ -125,11 +134,6 @@ class RegexContext { return _match_context; } - pcre2_jit_stack * - get_jit_stack() - { - return _jit_stack; - } private: RegexContext() @@ -137,21 +141,13 @@ class RegexContext _general_context = pcre2_general_context_create(my_malloc, my_free, nullptr); _compile_context = pcre2_compile_context_create(_general_context); _match_context = pcre2_match_context_create(_general_context); - _jit_stack = pcre2_jit_stack_create(4096, 1024 * 1024, nullptr); // 1 page min and 1MB max - pcre2_jit_stack_assign(_match_context, &thread_jit_stack, nullptr); + pcre2_jit_stack_assign(_match_context, jit_stack_for_this_thread, nullptr); } pcre2_general_context *_general_context = nullptr; pcre2_compile_context *_compile_context = nullptr; pcre2_match_context *_match_context = nullptr; - pcre2_jit_stack *_jit_stack = nullptr; }; -pcre2_jit_stack * -thread_jit_stack(void *) -{ - return RegexContext::get_instance()->get_jit_stack(); -} - } // namespace //---------------------------------------------------------------------------- From 0e67103e041ed2d844b01588a3d86abcc96344ad Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Wed, 9 Sep 2026 16:31:09 -0700 Subject: [PATCH 03/13] Split the JIT stack thread locals to avoid a loader deadlock Holding the stack in one thread local object that also frees it means the callback touches a thread local with a destructor. That runs the TLS init function, which calls __cxa_thread_atexit and takes the loader mutex, from inside a callback PCRE2 invokes during a match. That deadlocked. The pre-PCRE2 implementation solved this by splitting the raw pointer from the cleanup object, and the arrangement was lost when the JIT stack moved into RegexContext. Restore it, with the reasoning recorded next to it. --- src/tsutil/Regex.cc | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index 90b1a883d68..9af63b6bd2a 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -85,16 +85,32 @@ my_free(void *ptr, void * /*caller*/) // bake in the stack owned by whichever thread built the context, and two threads // matching at once on one stack corrupts memory. PCRE2 accepts a callback for // exactly this case and invokes it at match time, on the matching thread. +// The stack is held in a raw thread local pointer and freed by a separate thread +// local object, rather than by one object that owns both. Touching a thread local +// with a destructor runs the TLS init function, which calls __cxa_thread_atexit and +// takes the loader mutex. Doing that from inside this callback, which PCRE2 invokes +// during a match, deadlocked. This split is how the pre-PCRE2 implementation solved +// it; the arrangement was lost when the JIT stack moved into RegexContext. +thread_local pcre2_jit_stack *jit_stack = nullptr; + +struct JitStackCleanup { + ~JitStackCleanup() + { + if (jit_stack != nullptr) { + pcre2_jit_stack_free(jit_stack); + } + } +}; + +thread_local JitStackCleanup jit_stack_cleanup; + pcre2_jit_stack * jit_stack_for_this_thread(void *) { - struct ThreadStack { - ThreadStack() : stack{pcre2_jit_stack_create(4096, 1024 * 1024, nullptr)} {} // 1 page min and 1MB max - ~ThreadStack() { pcre2_jit_stack_free(stack); } - pcre2_jit_stack *stack; - }; - thread_local ThreadStack owner; - return owner.stack; + if (jit_stack == nullptr) { + jit_stack = pcre2_jit_stack_create(4096, 1024 * 1024, nullptr); // 1 page min and 1MB max + } + return jit_stack; } //---------------------------------------------------------------------------- From ba6d204fe745e9f87f6832e7aa92e82252a6f97d Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Wed, 9 Sep 2026 16:34:11 -0700 Subject: [PATCH 04/13] Register the JIT stack cleanup outside the match callback The cleanup object is deliberately not touched inside the stack callback, because initializing a thread local with a destructor takes the loader mutex and doing that during a match deadlocks. The consequence is that a thread which only ever reaches the callback never initializes the cleanup object, so its stack is never freed. LeakSanitizer caught it. Arm the cleanup from exec instead: on the matching thread, before the match, outside the callback. --- src/tsutil/Regex.cc | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index 9af63b6bd2a..da338a4d980 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -94,6 +94,7 @@ my_free(void *ptr, void * /*caller*/) thread_local pcre2_jit_stack *jit_stack = nullptr; struct JitStackCleanup { + bool armed = true; ~JitStackCleanup() { if (jit_stack != nullptr) { @@ -104,6 +105,18 @@ struct JitStackCleanup { thread_local JitStackCleanup jit_stack_cleanup; +// Reading the member forces the thread local to be initialized, which registers its +// destructor. That has to happen here, on the matching thread but before the match, +// rather than inside the callback. A thread that only ever reached the callback +// would otherwise never initialize the cleanup object and would leak its stack. +void +arm_jit_stack_cleanup() +{ + if (!jit_stack_cleanup.armed) { + return; + } +} + pcre2_jit_stack * jit_stack_for_this_thread(void *) { @@ -532,6 +545,8 @@ Regex::exec(std::string_view subject, RegexMatches &matches, uint32_t flags, Reg bool const full_match = (flags & RE_FULL_MATCH) != 0; uint32_t const pcre2_flags = flags & ~RE_FULL_MATCH; + arm_jit_stack_cleanup(); + int rc = pcre2_match(code, reinterpret_cast(subject.data()), subject.size(), 0, pcre2_flags, RegexMatches::_MatchData::get(matches._match_data), match_context); From 65a4cdee9701b2c6ed032ef2cfaf653074419217 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Wed, 9 Sep 2026 16:37:54 -0700 Subject: [PATCH 05/13] Tighten the JIT stack callback comment Keep the two load bearing facts, why a callback and why two thread locals, and drop the history and the mechanism narration. --- src/tsutil/Regex.cc | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index da338a4d980..bd9ac02c1c2 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -80,17 +80,12 @@ my_free(void *ptr, void * /*caller*/) } //---------------------------------------------------------------------------- -// PCRE2 needs a distinct JIT stack per thread, and a match context can be copied -// and then used on a different thread. Handing over a plain stack pointer would -// bake in the stack owned by whichever thread built the context, and two threads -// matching at once on one stack corrupts memory. PCRE2 accepts a callback for -// exactly this case and invokes it at match time, on the matching thread. -// The stack is held in a raw thread local pointer and freed by a separate thread -// local object, rather than by one object that owns both. Touching a thread local -// with a destructor runs the TLS init function, which calls __cxa_thread_atexit and -// takes the loader mutex. Doing that from inside this callback, which PCRE2 invokes -// during a match, deadlocked. This split is how the pre-PCRE2 implementation solved -// it; the arrangement was lost when the JIT stack moved into RegexContext. +// A match context can be copied and used on another thread, and PCRE2 requires a +// distinct JIT stack per thread, so the stack comes from a callback invoked at +// match time rather than a pointer baked in when the context is built. +// The pointer and the cleanup object are separate thread locals on purpose: +// touching a thread local with a destructor here registers it via +// __cxa_thread_atexit, which takes the loader mutex during a match and deadlocks. thread_local pcre2_jit_stack *jit_stack = nullptr; struct JitStackCleanup { From 23c7f3a5b7cdcb87a36c183547f31f128fde5406 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Thu, 10 Sep 2026 12:40:40 -0700 Subject: [PATCH 06/13] Make RegexMatchContext non-copyable and non-movable Nothing in the tree copies or moves one; every use is a plain member or local. The four special members were dead code carrying two defects. The defaulted move copied the raw pointer and left the source holding it, so both destructors freed the same object. The copy constructor left the pointer null when the source was null, which its own destructor asserts on in a debug build and silently ignores in a release one. Deleting them makes both unrepresentable rather than fixing them. --- include/tsutil/Regex.h | 13 +++++++------ src/tsutil/Regex.cc | 26 -------------------------- 2 files changed, 7 insertions(+), 32 deletions(-) diff --git a/include/tsutil/Regex.h b/include/tsutil/Regex.h index 5b913bece06..98ff73b6e84 100644 --- a/include/tsutil/Regex.h +++ b/include/tsutil/Regex.h @@ -113,12 +113,13 @@ class RegexMatchContext RegexMatchContext(); ~RegexMatchContext(); - /// uses pcre2_match_context_copy for a deep copy. - RegexMatchContext(RegexMatchContext const &orig); - RegexMatchContext &operator=(RegexMatchContext const &orig); - - RegexMatchContext(RegexMatchContext &&) = default; - RegexMatchContext &operator=(RegexMatchContext &&) = default; + /// Not copyable or movable. Nothing copies one, the defaulted move copied the + /// raw pointer and left both objects freeing it, and the copy constructor could + /// leave the pointer null for its own destructor to assert on. + RegexMatchContext(RegexMatchContext const &) = delete; + RegexMatchContext &operator=(RegexMatchContext const &) = delete; + RegexMatchContext(RegexMatchContext &&) = delete; + RegexMatchContext &operator=(RegexMatchContext &&) = delete; /** Limits the amount of backtracking that can take place. * Any regex exec call that fails will return PCRE2_ERROR_MATCHLIMIT(-47) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index bd9ac02c1c2..96194b99541 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -303,32 +303,6 @@ RegexMatchContext::RegexMatchContext() _MatchContext::set(_match_context, ctx); } -//---------------------------------------------------------------------------- -RegexMatchContext::RegexMatchContext(RegexMatchContext const &other) -{ - auto ptr = _MatchContext::get(other._match_context); - if (nullptr != ptr) { - pcre2_match_context *const ctx = pcre2_match_context_copy(ptr); - _MatchContext::set(_match_context, ctx); - } -} - -//---------------------------------------------------------------------------- -RegexMatchContext & -RegexMatchContext::operator=(RegexMatchContext const &other) -{ - if (&other != this) { - auto ptr = _MatchContext::get(other._match_context); - if (nullptr != ptr) { - pcre2_match_context *const ctx = pcre2_match_context_copy(ptr); - _MatchContext::set(_match_context, ctx); - } else { - _MatchContext::set(_match_context, nullptr); - } - } - return *this; -} - //---------------------------------------------------------------------------- RegexMatchContext::~RegexMatchContext() { From d5dbd6015f14b1313e6e68a5fc62436f96cc739f Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Thu, 10 Sep 2026 12:50:08 -0700 Subject: [PATCH 07/13] Correct the reason the JIT stack comes from a callback The comment said a match context can be copied and used on another thread. It cannot: the copy and move members are deleted. The callback is needed because one context is shared by every thread that matches through it, which is true regardless of whether the type can be copied. --- src/tsutil/Regex.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index 96194b99541..e7a75567f43 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -80,9 +80,9 @@ my_free(void *ptr, void * /*caller*/) } //---------------------------------------------------------------------------- -// A match context can be copied and used on another thread, and PCRE2 requires a -// distinct JIT stack per thread, so the stack comes from a callback invoked at -// match time rather than a pointer baked in when the context is built. +// One match context is shared by every thread that matches through it, and PCRE2 +// requires a distinct JIT stack per thread, so the stack comes from a callback +// invoked at match time rather than a pointer baked in when the context is built. // The pointer and the cleanup object are separate thread locals on purpose: // touching a thread local with a destructor here registers it via // __cxa_thread_atexit, which takes the loader mutex during a match and deadlocks. From ec6557a3c8f3e60d4ad45d398b4133a255556049 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Thu, 10 Sep 2026 12:51:43 -0700 Subject: [PATCH 08/13] Drop the dead branch in the JIT stack cleanup arming The cleanup object carried an `armed` member that existed only to be read, and the arming function branched on it in a branch that could never be taken. Take the object's address instead, which forces the same thread local initialization without the fake member or the unreachable code. Also corrects a unit test comment that described plugin behaviour, and described it wrongly: the plugin never set a smaller JIT stack, it set none and got PCRE2's fallback. --- src/tsutil/Regex.cc | 13 +++++-------- src/tsutil/unit_tests/test_Regex.cc | 4 ++-- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index e7a75567f43..d83051f6c14 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -89,7 +89,6 @@ my_free(void *ptr, void * /*caller*/) thread_local pcre2_jit_stack *jit_stack = nullptr; struct JitStackCleanup { - bool armed = true; ~JitStackCleanup() { if (jit_stack != nullptr) { @@ -100,16 +99,14 @@ struct JitStackCleanup { thread_local JitStackCleanup jit_stack_cleanup; -// Reading the member forces the thread local to be initialized, which registers its -// destructor. That has to happen here, on the matching thread but before the match, -// rather than inside the callback. A thread that only ever reached the callback -// would otherwise never initialize the cleanup object and would leak its stack. +// Taking the address forces this thread's initialization of the cleanup object, which +// registers its destructor. That has to happen on the matching thread but outside the +// callback: a thread that only ever reached the callback would never initialize the +// object and would leak its stack. void arm_jit_stack_cleanup() { - if (!jit_stack_cleanup.armed) { - return; - } + [[maybe_unused]] auto const *cleanup = &jit_stack_cleanup; } pcre2_jit_stack * diff --git a/src/tsutil/unit_tests/test_Regex.cc b/src/tsutil/unit_tests/test_Regex.cc index f7e5eb91e5f..453dd5b8181 100644 --- a/src/tsutil/unit_tests/test_Regex.cc +++ b/src/tsutil/unit_tests/test_Regex.cc @@ -1085,8 +1085,8 @@ TEST_CASE("Regex reports resource exhaustion rather than crashing", "[libts][Reg Regex re; REQUIRE(re.compile(R"(^/alpha/bravo/[?]((?!action=(newsfeed|calendar|contacts|notepad)).)*$)")); - // Well past what a 1MiB JIT stack can hold, so the bound is still exercised - // now that the plugin no longer sets its own smaller one. + // Past what a 1MiB JIT stack holds for this pattern, which starts failing at + // roughly 43KiB of subject, so the bound is still exercised. std::string subject{"/alpha/bravo/?"}; subject.append(2 * 1024 * 1024, 'x'); From b7e1246f373eb54165edb75c7a872d30cd503da0 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Thu, 10 Sep 2026 12:52:56 -0700 Subject: [PATCH 09/13] Say copy rather than allocate in the match context assert The constructor copies the shared context now; the message still described allocating a new one, which would misdirect anyone hitting the assert. --- src/tsutil/Regex.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index d83051f6c14..0a31b5452aa 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -296,7 +296,7 @@ RegexMatchContext::RegexMatchContext() // type came to run with PCRE2's fallback 32KiB JIT stack instead of the 1MiB // one every other caller gets. Callers override only the fields they mean to. auto ctx = pcre2_match_context_copy(RegexContext::get_instance()->get_match_context()); - debug_assert_message(ctx, "Failed to allocate custom pcre2 match context"); + debug_assert_message(ctx, "Failed to copy the shared pcre2 match context"); _MatchContext::set(_match_context, ctx); } From 5071c9a99ac37d5aa1685285a46372cfcd59d94b Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Thu, 10 Sep 2026 12:58:08 -0700 Subject: [PATCH 10/13] Gate the JIT stack tests on JIT being available, and clear the freed stack Both new tests are about the JIT stack, and PCRE2 consults it only when it has JIT code for the pattern. Without JIT a blank context and the shared one both take the interpreter and return the same answer, so the parity test passed whether or not the behaviour it describes was present, and the resource exhaustion test failed outright: the interpreter keeps its backtracking frames on the heap and matches the 2 MiB subject rather than running out of anything. Both now ask PCRE2 for the pattern's JIT size and skip when there is none. Separately, the cleanup destructor freed the thread's JIT stack but left the pointer set. A regex match from a thread local destroyed after it would have been handed the freed stack; clear it so the next call allocates. --- src/tsutil/Regex.cc | 3 ++ src/tsutil/unit_tests/test_Regex.cc | 43 +++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index 0a31b5452aa..4d5d3eabf5f 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -93,6 +93,9 @@ struct JitStackCleanup { { if (jit_stack != nullptr) { pcre2_jit_stack_free(jit_stack); + // Clear it so a match from a thread local destroyed after this one gets a + // fresh stack rather than the freed pointer. + jit_stack = nullptr; } } }; diff --git a/src/tsutil/unit_tests/test_Regex.cc b/src/tsutil/unit_tests/test_Regex.cc index 453dd5b8181..769c4784a3a 100644 --- a/src/tsutil/unit_tests/test_Regex.cc +++ b/src/tsutil/unit_tests/test_Regex.cc @@ -1051,6 +1051,32 @@ TEST_CASE("Regex end-anchor with alternation", "[libts][Regex]") CHECK(r.exec("prefix.cdn.example.com", matches) == RE_ERROR_NOMATCH); } +namespace +{ +/** Does PCRE2 have JIT code for this pattern? + * + * The two tests below are about the JIT stack, and PCRE2 consults it only when it + * has JIT code to run. Without it both a blank context and the shared one take the + * interpreter and return the same answer, so the tests would pass whether or not + * the behaviour they describe is present. Ask PCRE2 rather than assume. + */ +bool +pattern_has_jit(char const *pattern) +{ + int errnum = 0; + PCRE2_SIZE erroffset = 0; + pcre2_code *code = pcre2_compile(reinterpret_cast(pattern), PCRE2_ZERO_TERMINATED, 0, &errnum, &erroffset, nullptr); + if (code == nullptr) { + return false; + } + pcre2_jit_compile(code, PCRE2_JIT_COMPLETE); + size_t jit_size = 0; + pcre2_pattern_info(code, PCRE2_INFO_JITSIZE, &jit_size); + pcre2_code_free(code); + return jit_size > 0; +} +} // namespace + // A caller-supplied RegexMatchContext must behave like the shared context that // Regex::exec uses when none is supplied. A context built from scratch silently // drops everything the shared one configures, which is how regex_remap came to @@ -1059,8 +1085,13 @@ TEST_CASE("RegexMatchContext matches the shared context", "[libts][Regex][RegexM { // Quantified alternation of capture groups: every subject character pushes a // backtracking frame, so the JIT stack size is what bounds this. + char const *const pattern = R"(^(?:(a)|(b))+$)"; + if (!pattern_has_jit(pattern)) { + SKIP("PCRE2 has no JIT for this pattern, so the JIT stack is never consulted"); + } + Regex re; - REQUIRE(re.compile(R"(^(?:(a)|(b))+$)")); + REQUIRE(re.compile(pattern)); std::string const subject(1000, 'a'); @@ -1082,8 +1113,16 @@ TEST_CASE("RegexMatchContext matches the shared context", "[libts][Regex][RegexM // instead. If this ever crashes rather than fails, that regression is back. TEST_CASE("Regex reports resource exhaustion rather than crashing", "[libts][Regex][limits]") { + // Only the JIT path has a bound to exhaust here. PCRE2's interpreter keeps its + // backtracking frames on the heap, so it matches this subject rather than running + // out of anything, and there is no resource error to assert. + char const *const pattern = R"(^/alpha/bravo/[?]((?!action=(newsfeed|calendar|contacts|notepad)).)*$)"; + if (!pattern_has_jit(pattern)) { + SKIP("PCRE2 has no JIT for this pattern, so there is no stack bound to exhaust"); + } + Regex re; - REQUIRE(re.compile(R"(^/alpha/bravo/[?]((?!action=(newsfeed|calendar|contacts|notepad)).)*$)")); + REQUIRE(re.compile(pattern)); // Past what a 1MiB JIT stack holds for this pattern, which starts failing at // roughly 43KiB of subject, so the bound is still exercised. From db72f15ace91b926cec5d00998c6a152ed4c3bf6 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Thu, 10 Sep 2026 14:05:55 -0700 Subject: [PATCH 11/13] Size the resource exhaustion subject to the bound it tests The subject was 2MiB against a bound that trips at roughly 43KiB, which is 48 times more than the test needs. 256KiB keeps a six times margin for a platform with larger JIT frames. The match cost is unchanged either way: it bails at the stack limit before traversing the subject, measured at about 0.14ms for every size from 64KiB to 2MiB. What this saves is the allocation, not time. --- src/tsutil/unit_tests/test_Regex.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/tsutil/unit_tests/test_Regex.cc b/src/tsutil/unit_tests/test_Regex.cc index 769c4784a3a..ed3d841936d 100644 --- a/src/tsutil/unit_tests/test_Regex.cc +++ b/src/tsutil/unit_tests/test_Regex.cc @@ -1124,10 +1124,12 @@ TEST_CASE("Regex reports resource exhaustion rather than crashing", "[libts][Reg Regex re; REQUIRE(re.compile(pattern)); - // Past what a 1MiB JIT stack holds for this pattern, which starts failing at - // roughly 43KiB of subject, so the bound is still exercised. + // This pattern starts failing at roughly 43KiB of subject against a 1MiB JIT + // stack, measured identically on x86_64 and arm64. 256KiB keeps a six times + // margin for a platform whose JIT frames are larger, without allocating more + // than the bound needs. Do not trim this to just above 43KiB. std::string subject{"/alpha/bravo/?"}; - subject.append(2 * 1024 * 1024, 'x'); + subject.append(256 * 1024, 'x'); RegexMatches matches; int const rc = re.exec(subject, matches); From 143e53238f4f08eb662718db6cfef687dda60f72 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Thu, 10 Sep 2026 15:10:36 -0700 Subject: [PATCH 12/13] Hold the JIT stack in a pthread key rather than a thread local A thread_local with a destructor registers it through __cxa_thread_atexit, which takes the dynamic loader lock. Registering that from Regex::exec inverts lock order against a dlopen caller running a plugin's static initialization, which is the hazard Diags::tag_activated already documents and works around. Arming the cleanup before the match avoided registering from inside PCRE2's callback but left the registration on exec, and extended it to the caller-supplied context path that never had it. A pthread key registers its destructor once, at key creation, and never from the matching path. This also removes the two thread locals, the arming function and its call, so the deadlock and leak trade goes away rather than being balanced. --- src/tsutil/Regex.cc | 51 +++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 27 deletions(-) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index 4d5d3eabf5f..7026c23c3d1 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -26,6 +26,7 @@ #define PCRE2_CODE_UNIT_WIDTH 8 #include +#include #include #include @@ -83,42 +84,40 @@ my_free(void *ptr, void * /*caller*/) // One match context is shared by every thread that matches through it, and PCRE2 // requires a distinct JIT stack per thread, so the stack comes from a callback // invoked at match time rather than a pointer baked in when the context is built. -// The pointer and the cleanup object are separate thread locals on purpose: -// touching a thread local with a destructor here registers it via -// __cxa_thread_atexit, which takes the loader mutex during a match and deadlocks. -thread_local pcre2_jit_stack *jit_stack = nullptr; +// +// The per thread stack is held in a pthread key rather than a thread_local. A +// thread_local with a destructor registers it through __cxa_thread_atexit, which +// takes the dynamic loader lock; doing that from a match would invert lock order +// against a dlopen caller running a plugin's static initialization. See the same +// hazard described at Diags::tag_activated. A pthread key registers its destructor +// once, at key creation, and never from the matching path. +pthread_key_t jit_stack_key; +pthread_once_t jit_stack_key_once = PTHREAD_ONCE_INIT; -struct JitStackCleanup { - ~JitStackCleanup() - { - if (jit_stack != nullptr) { - pcre2_jit_stack_free(jit_stack); - // Clear it so a match from a thread local destroyed after this one gets a - // fresh stack rather than the freed pointer. - jit_stack = nullptr; - } +void +destroy_jit_stack(void *stack) +{ + if (stack != nullptr) { + pcre2_jit_stack_free(static_cast(stack)); } -}; - -thread_local JitStackCleanup jit_stack_cleanup; +} -// Taking the address forces this thread's initialization of the cleanup object, which -// registers its destructor. That has to happen on the matching thread but outside the -// callback: a thread that only ever reached the callback would never initialize the -// object and would leak its stack. void -arm_jit_stack_cleanup() +make_jit_stack_key() { - [[maybe_unused]] auto const *cleanup = &jit_stack_cleanup; + pthread_key_create(&jit_stack_key, destroy_jit_stack); } pcre2_jit_stack * jit_stack_for_this_thread(void *) { - if (jit_stack == nullptr) { - jit_stack = pcre2_jit_stack_create(4096, 1024 * 1024, nullptr); // 1 page min and 1MB max + pthread_once(&jit_stack_key_once, make_jit_stack_key); + auto *stack = static_cast(pthread_getspecific(jit_stack_key)); + if (stack == nullptr) { + stack = pcre2_jit_stack_create(4096, 1024 * 1024, nullptr); // 1 page min and 1MB max + pthread_setspecific(jit_stack_key, stack); } - return jit_stack; + return stack; } //---------------------------------------------------------------------------- @@ -514,8 +513,6 @@ Regex::exec(std::string_view subject, RegexMatches &matches, uint32_t flags, Reg bool const full_match = (flags & RE_FULL_MATCH) != 0; uint32_t const pcre2_flags = flags & ~RE_FULL_MATCH; - arm_jit_stack_cleanup(); - int rc = pcre2_match(code, reinterpret_cast(subject.data()), subject.size(), 0, pcre2_flags, RegexMatches::_MatchData::get(matches._match_data), match_context); From fff1a840a28b880bf160a150efaaff17e2df6da5 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Thu, 10 Sep 2026 16:13:51 -0700 Subject: [PATCH 13/13] Handle pthread_key_create failure in the JIT stack callback The return value was ignored. pthread_once marks the initializer complete either way, so a failed create would leave jit_stack_key at its default, which may name a key belonging to something else, and the callback would hand PCRE2 whatever that key holds as a JIT stack. Record whether the key was created and return null when it was not. PCRE2 documents a null return as thread safe: the match falls back to its own default stack. --- src/tsutil/Regex.cc | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index 7026c23c3d1..5726b490ccb 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -92,7 +92,8 @@ my_free(void *ptr, void * /*caller*/) // hazard described at Diags::tag_activated. A pthread key registers its destructor // once, at key creation, and never from the matching path. pthread_key_t jit_stack_key; -pthread_once_t jit_stack_key_once = PTHREAD_ONCE_INIT; +bool jit_stack_key_valid = false; +pthread_once_t jit_stack_key_once = PTHREAD_ONCE_INIT; void destroy_jit_stack(void *stack) @@ -105,13 +106,20 @@ destroy_jit_stack(void *stack) void make_jit_stack_key() { - pthread_key_create(&jit_stack_key, destroy_jit_stack); + jit_stack_key_valid = pthread_key_create(&jit_stack_key, destroy_jit_stack) == 0; } pcre2_jit_stack * jit_stack_for_this_thread(void *) { pthread_once(&jit_stack_key_once, make_jit_stack_key); + if (!jit_stack_key_valid) { + // Without a key there is nowhere to keep a stack, and jit_stack_key holds a + // default value that may name an unrelated key. Returning null tells PCRE2 to + // use its own default stack, which pcre2jit documents as thread safe. + return nullptr; + } + auto *stack = static_cast(pthread_getspecific(jit_stack_key)); if (stack == nullptr) { stack = pcre2_jit_stack_create(4096, 1024 * 1024, nullptr); // 1 page min and 1MB max