From 02f0b281b527b7be7fa7a8276161af8afd2ee793 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Sat, 12 Sep 2026 12:22:11 -0700 Subject: [PATCH 1/7] Regex: hold the JIT stack in a pthread key and inherit the shared match context Two problems with the same root: a caller-supplied RegexMatchContext was built blank, and the JIT stack was held in a thread_local. pcre2_match_context_create(nullptr) produces a context that configures nothing, so a caller who wanted only to set a match limit silently gave up everything the shared context provides, including its 1 MiB JIT stack. PCRE2 then fell back to its own 32 KiB machine-stack block, which resolves about 1,362 bytes of a subject that backtracks once per character. A production regex_remap rule hit that bound at 1,377 bytes of query string. Copy the shared context instead, so a caller overrides only what it means to override. A shared context needs a per thread JIT stack, and a thread_local holding one registers its destructor through __cxa_thread_atexit, which takes the dynamic loader lock. Doing that from a match inverts lock order against a dlopen caller running a plugin's static initialization; Diags::tag_activated documents that exact deadlock. Take the stack from a callback backed by a pthread key instead, whose destructor is registered once at key creation and never from the matching path. pthread_key_create can fail, and jit_stack_key is zero initialized, so key 0 could belong to another subsystem and hand its value to PCRE2 as a JIT stack. Record whether the key was created and return null when it was not, which pcre2jit documents as falling back to its own stack. If pthread_setspecific fails, free the stack rather than leaking one per match. Record why the maximum is one mebibyte, measured rather than assumed: the maximum costs nothing per match at any size, and one mebibyte already resolves a longer subject than request_header_max_size lets a client send. --- src/tsutil/Regex.cc | 77 ++++++++++++++-- src/tsutil/unit_tests/test_Regex.cc | 89 +++++++++++++++++++ .../regex_remap/regex_remap.test.py | 29 ++++-- 3 files changed, 182 insertions(+), 13 deletions(-) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index 3281622b1ec..30a126da6fe 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 @@ -79,6 +80,67 @@ my_free(void *ptr, void * /*caller*/) free(ptr); } +//---------------------------------------------------------------------------- +// 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 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; +bool jit_stack_key_valid = false; +pthread_once_t jit_stack_key_once = PTHREAD_ONCE_INIT; + +void +destroy_jit_stack(void *stack) +{ + if (stack != nullptr) { + pcre2_jit_stack_free(static_cast(stack)); + } +} + +void +make_jit_stack_key() +{ + 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) { + // One page to start, one mebibyte at most. Measured on PCRE2 10.47 against a pattern + // that backtracks once per character, which turns the maximum directly into a subject + // length: 32 KiB of stack resolves a 1,362 byte subject, 1 MiB resolves 43,687, 8 MiB + // resolves 349,522, and match time is flat across all of them. The maximum is address + // space reserved at creation, made resident only as deep as a match actually goes, and + // pcre2 does not hand it back, so a thread that once saw a deep subject keeps the + // pages. One mebibyte already covers a longer subject than a client can deliver, since + // proxy.config.http.request_header_max_size defaults to 32,768 bytes. + stack = pcre2_jit_stack_create(4096, 1024 * 1024, nullptr); + if (pthread_setspecific(jit_stack_key, stack) != 0) { + // Nothing holds the stack now, so it would leak once per match. Give it back and + // let PCRE2 use its own default stack for this call. + pcre2_jit_stack_free(stack); + return nullptr; + } + } + return stack; +} + //---------------------------------------------------------------------------- class RegexContext { @@ -100,9 +162,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() @@ -126,13 +185,11 @@ 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, nullptr, _jit_stack); + 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; }; } // namespace @@ -257,8 +314,12 @@ struct RegexMatchContext::_MatchContext { //---------------------------------------------------------------------------- RegexMatchContext::RegexMatchContext() { - auto ctx = pcre2_match_context_create(nullptr); - debug_assert_message(ctx, "Failed to allocate custom pcre2 match context"); + // 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 copy the shared 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..ed3d841936d 100644 --- a/src/tsutil/unit_tests/test_Regex.cc +++ b/src/tsutil/unit_tests/test_Regex.cc @@ -1050,3 +1050,92 @@ 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); } + +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 +// 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. + 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(pattern)); + + 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]") +{ + // 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(pattern)); + + // 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(256 * 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 3cd632e9ea9252d7b2c5fccb4efb2d49980fdb02 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Sat, 12 Sep 2026 12:25:59 -0700 Subject: [PATCH 2/7] Regex: share one process-wide set of pcre2 contexts Moving the JIT stack into a pthread key removed half of the thread_local hazard. The other half remained: the general, compile and match contexts lived in a thread_local with a destructor, and initializing one registers that destructor through __cxa_thread_atexit, which takes the dynamic loader lock. The first compile() or exec() on a thread therefore still inverted lock order against a dlopen caller running a plugin's static initialization. Nothing in those contexts is per thread. They are built once and never modified, which pcre2api's MULTITHREADING section gives as the condition for sharing a context between threads, and the one per thread object is resolved through the callback. Allocate one instance and never destroy it: three small blocks that live as long as the process, against a destructor that can run while another thread is still matching. That also retires the "should only be null when shutting down" check, which a function-local thread_local could never satisfy. In the object file, RegexContext::~RegexContext() and the thread-local ctx are both gone; what is left is a plain static pointer behind a guard variable, which registers nothing at thread exit. The object still references __cxa_thread_atexit, but that belongs to the inline thread_local in tsutil/ts_bw_format.h and is there before and after this change. The new test runs eight threads matching on one instance, half of them through their own match context, and checks every thread reaches the same verdict. The header promised this and nothing tested it. It is clean under ThreadSanitizer. --- src/tsutil/Regex.cc | 31 +++++++--------- src/tsutil/unit_tests/test_Regex.cc | 56 +++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 18 deletions(-) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index 30a126da6fe..8956d0a9903 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -142,26 +142,25 @@ jit_stack_for_this_thread(void *) } //---------------------------------------------------------------------------- +// These three contexts are built once and never modified, which pcre2api's MULTITHREADING +// section gives as the condition for sharing a context across threads. The one genuinely +// per thread object, the JIT stack, is reached through the callback above. +// +// The instance is allocated once and deliberately never destroyed. A thread_local with a +// destructor registers it through __cxa_thread_atexit on first use, which takes the +// dynamic loader lock, so the first compile() or exec() on a thread inverts lock order +// against a dlopen caller running a plugin's static initialization; that is the same +// hazard the JIT stack moved to a pthread key to avoid, and the one Diags and DbgCtl work +// around. A context destroyed at thread or process exit can also still be in use by +// another thread that is matching. class RegexContext { public: static RegexContext * get_instance() { - thread_local RegexContext ctx; - return &ctx; - } - ~RegexContext() - { - if (_general_context != nullptr) { - pcre2_general_context_free(_general_context); - } - if (_compile_context != nullptr) { - pcre2_compile_context_free(_compile_context); - } - if (_match_context != nullptr) { - pcre2_match_context_free(_match_context); - } + static RegexContext *const ctx = new RegexContext(); + return ctx; } pcre2_general_context * get_general_context() @@ -460,11 +459,7 @@ Regex::compile(std::string_view pattern, std::string &error, int &erroroffset, u pcre2_code_free(ptr); } - // get the RegexContext instance - should only be null when shutting down RegexContext *regex_context = RegexContext::get_instance(); - if (regex_context == nullptr) { - return false; - } // On PCRE2 < 10.30 the ENDANCHORED bit is not a valid pcre2_compile option. Rewrite // the pattern to "(?:pattern)\z" and strip the bit so pcre2 enforces end-of-subject diff --git a/src/tsutil/unit_tests/test_Regex.cc b/src/tsutil/unit_tests/test_Regex.cc index ed3d841936d..d24cbfc78f5 100644 --- a/src/tsutil/unit_tests/test_Regex.cc +++ b/src/tsutil/unit_tests/test_Regex.cc @@ -20,7 +20,11 @@ limitations under the License. */ +#include +#include +#include #include +#include #include #define PCRE2_CODE_UNIT_WIDTH 8 @@ -1139,3 +1143,55 @@ TEST_CASE("Regex reports resource exhaustion rather than crashing", "[libts][Reg REQUIRE(rc < 0); REQUIRE(rc != RE_ERROR_NOMATCH); } + +// The header promises that exec() may be called concurrently on one instance, and nothing +// tested that. Every thread must reach the same verdict, whether it matches through the +// shared context or through one it built itself, and each thread must get its own JIT +// stack from the callback rather than share one. Run this under ThreadSanitizer to get the +// second half of the guarantee. +TEST_CASE("Regex matches concurrently on one instance", "[libts][Regex][threads]") +{ + Regex re; + REQUIRE(re.compile(R"(^/([a-z]+)/([0-9]+)/(.*)$)")); + + constexpr int THREADS = 8; + constexpr int ITERATIONS = 2000; + + std::string const hit{"/alpha/42/tail"}; + std::string const miss{"/Alpha/xx/tail"}; + + std::atomic failures{0}; + std::latch start{THREADS}; + + std::vector threads; + threads.reserve(THREADS); + for (int i = 0; i < THREADS; ++i) { + threads.emplace_back([&, i]() { + // Half the threads bring their own match context, which is a copy of the shared one + // and so carries the same JIT stack callback. + bool const own_context = (i % 2) == 0; + RegexMatchContext context; + RegexMatchContext const *const use = own_context ? &context : nullptr; + + start.arrive_and_wait(); + + for (int n = 0; n < ITERATIONS; ++n) { + RegexMatches matches; + if (re.exec(hit, matches, 0, use) != 4 || matches[1] != "alpha" || matches[2] != "42" || matches[3] != "tail") { + ++failures; + } + + RegexMatches no_matches; + if (re.exec(miss, no_matches, 0, use) != RE_ERROR_NOMATCH) { + ++failures; + } + } + }); + } + + for (auto &t : threads) { + t.join(); + } + + CHECK(failures.load() == 0); +} From e0f416cc1cd2d78f79a269494ba60b1c5f9005ff Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Sat, 12 Sep 2026 12:23:01 -0700 Subject: [PATCH 3/7] Regex: keep the compiled pattern until a recompile succeeds Regex::compile() freed the pattern it already held before calling pcre2_compile(). Every failure path after that point returned with the freed pointer still stored, so empty() reported the object as compiled, exec() passed the freed block to pcre2_match(), and the destructor freed it a second time. Compile into a local and replace the member only after the new pattern exists. A failed compile now leaves the previous pattern in place and usable, which is what a caller checking the return value would expect, and a fresh object that fails to compile is still empty. Two tests cover it: a valid compile followed by a failing one must leave the first pattern matching, including its capture groups. Before this change the first of those segmentation faults. --- include/tsutil/Regex.h | 8 +++++-- src/tsutil/Regex.cc | 13 +++++++----- src/tsutil/unit_tests/test_Regex.cc | 33 +++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 7 deletions(-) diff --git a/include/tsutil/Regex.h b/include/tsutil/Regex.h index 5b913bece06..1b4291509d2 100644 --- a/include/tsutil/Regex.h +++ b/include/tsutil/Regex.h @@ -163,23 +163,27 @@ class Regex /** Compile the @a pattern into a regular expression. * - * @param pattern Source pattern for regular expression (null terminated). + * @param pattern Source pattern for regular expression. * @param flags Compilation flags. * @return @a true if compiled successfully, @a false otherwise. * * @a flags should be the bitwise @c or of @c REFlags values. + * + * On failure any previously compiled pattern is left in place and remains usable. */ bool compile(std::string_view pattern, uint32_t flags = 0); /** Compile the @a pattern into a regular expression. * - * @param pattern Source pattern for regular expression (null terminated). + * @param pattern Source pattern for regular expression. * @param error String to receive error message. * @param erroffset Pointer to integer to receive error offset. * @param flags Compilation flags. * @return @a true if compiled successfully, @a false otherwise. * * @a flags should be the bitwise @c or of @c REFlags values. + * + * On failure any previously compiled pattern is left in place and remains usable. */ bool compile(std::string_view pattern, std::string &error, int &erroffset, unsigned flags = 0); diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index 8956d0a9903..a75e0c917b6 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -454,11 +454,6 @@ Regex::compile(std::string_view pattern, uint32_t flags) bool Regex::compile(std::string_view pattern, std::string &error, int &erroroffset, uint32_t flags) { - // free the existing compiled regex if there is one - if (auto ptr = _Code::get(_code); ptr != nullptr) { - pcre2_code_free(ptr); - } - RegexContext *regex_context = RegexContext::get_instance(); // On PCRE2 < 10.30 the ENDANCHORED bit is not a valid pcre2_compile option. Rewrite @@ -501,6 +496,14 @@ Regex::compile(std::string_view pattern, std::string &error, int &erroroffset, u // support for JIT pcre2_jit_compile(code, PCRE2_JIT_COMPLETE); + // Replace the previous pattern only now that the new one exists. Freeing it before + // pcre2_compile would leave every failure path above returning with a dangling + // pointer in _code, which empty() reports as a compiled pattern and exec() hands to + // pcre2_match. + if (auto ptr = _Code::get(_code); ptr != nullptr) { + pcre2_code_free(ptr); + } + _Code::set(_code, code); return true; diff --git a/src/tsutil/unit_tests/test_Regex.cc b/src/tsutil/unit_tests/test_Regex.cc index d24cbfc78f5..b1b375a881b 100644 --- a/src/tsutil/unit_tests/test_Regex.cc +++ b/src/tsutil/unit_tests/test_Regex.cc @@ -650,6 +650,39 @@ TEST_CASE("Regex recompilation behavior", "[libts][Regex][recompile]") CHECK(r.exec("valid") == true); } + SECTION("a failed recompile leaves the working pattern in place") + { + // compile() is a transaction. A pattern that fails to compile must not disturb the + // pattern already held, because the alternative is worse than either outcome: freeing + // the old pattern before knowing the new one compiles leaves a dangling pointer that + // empty() reports as compiled and exec() hands to pcre2_match. + Regex r; + REQUIRE(r.compile("foo") == true); + + REQUIRE(r.compile("(invalid") == false); + + CHECK(r.empty() == false); + CHECK(r.exec("foo") == true); + CHECK(r.exec("bar") == false); + + // And the object is still usable for a later successful compile. + REQUIRE(r.compile("bar") == true); + CHECK(r.exec("bar") == true); + } + + SECTION("a failed recompile leaves captures working") + { + Regex r; + REQUIRE(r.compile("^(a+)(b+)$") == true); + + REQUIRE(r.compile("(unterminated") == false); + + RegexMatches matches; + REQUIRE(r.exec("aaabb", matches) == 3); + CHECK(matches[1] == "aaa"); + CHECK(matches[2] == "bb"); + } + SECTION("recompile with different flags") { Regex r; From 974406d2a7050c0c8c549e073b7e6759c31d6856 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Sat, 12 Sep 2026 12:23:25 -0700 Subject: [PATCH 4/7] Regex: compile the copy for the JIT engine pcre2_code_copy() copies a compiled pattern but not the machine code the JIT produced for it, because that code is position dependent. The copy constructor called nothing else, so every copied Regex matched on the interpreter: the same answers, far slower, and under a different set of resource limits. A pattern that reports a JIT stack limit through the original quietly matched through a copy, which is how the two disagree about whether a subject is acceptable at all. Compile the copy for the JIT after copying it, exactly as compile() does for a new pattern, and describe that in the header, which called it a deep copy. The test asserts the property that matters: a copy answers the same as its original on a subject sized past the JIT stack bound. Before this change the original returned the stack limit error and the copy returned a match. It needs no knowledge of whether the build has a JIT, because without one both sides simply agree. --- include/tsutil/Regex.h | 3 +- src/tsutil/Regex.cc | 8 ++++ src/tsutil/unit_tests/test_Regex.cc | 64 +++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 1 deletion(-) diff --git a/include/tsutil/Regex.h b/include/tsutil/Regex.h index 1b4291509d2..5b299b8c15a 100644 --- a/include/tsutil/Regex.h +++ b/include/tsutil/Regex.h @@ -144,7 +144,8 @@ class Regex * * Creates a new Regex object with a deep copy of the compiled pattern. * Uses pcre2_code_copy() to duplicate the compiled pattern without - * requiring the original pattern string. + * requiring the original pattern string, then compiles the copy for the + * just-in-time engine, which pcre2_code_copy() cannot carry over. * * @param other The Regex object to copy from. */ diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index a75e0c917b6..542f63c3568 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -390,6 +390,14 @@ Regex::Regex(Regex const &other) if (other_code != nullptr) { // Use PCRE2's built-in function to deep copy the compiled pattern auto *copied_code = pcre2_code_copy(other_code); + + // pcre2_code_copy() does not carry the machine code the JIT produced, because that + // code is position dependent. Without this the copy would match on the interpreter: + // same answers, much slower, and a different set of resource limits, so a pattern + // that reports a JIT stack limit through the original would quietly match through + // the copy. Compile it again, exactly as Regex::compile() does for a new pattern. + pcre2_jit_compile(copied_code, PCRE2_JIT_COMPLETE); + _Code::set(_code, copied_code); } } diff --git a/src/tsutil/unit_tests/test_Regex.cc b/src/tsutil/unit_tests/test_Regex.cc index b1b375a881b..5af2f9ed6ff 100644 --- a/src/tsutil/unit_tests/test_Regex.cc +++ b/src/tsutil/unit_tests/test_Regex.cc @@ -1228,3 +1228,67 @@ TEST_CASE("Regex matches concurrently on one instance", "[libts][Regex][threads] CHECK(failures.load() == 0); } + +// pcre2_code_copy() copies the compiled pattern but not the machine code the JIT produced +// for it, because that code is position dependent. A copy that is not passed back through +// pcre2_jit_compile() therefore matches on the interpreter: the same answers, far more +// slowly, and under a different set of resource limits, so a subject one of them reports +// as too expensive the other quietly matches. +// +// The subject below is sized past the JIT engine's stack bound for this pattern, which is +// what makes the two engines disagree. The assertion is that a copy answers the same as +// its original, whatever that answer is, so the test needs no knowledge of whether this +// build has a JIT. +TEST_CASE("Regex copies answer the same as their original", "[libts][Regex][copy]") +{ + Regex original; + REQUIRE(original.compile(R"(^/alpha/bravo/[?]((?!action=(newsfeed|calendar|contacts|notepad)).)*$)")); + + std::string subject{"/alpha/bravo/?"}; + subject.append(256 * 1024, 'x'); + + RegexMatches original_matches; + int const original_rc = original.exec(subject, original_matches); + CAPTURE(original_rc); + + SECTION("copy constructor") + { + Regex copy(original); + RegexMatches matches; + int const rc = copy.exec(subject, matches); + CAPTURE(rc); + CHECK(rc == original_rc); + } + + SECTION("copy assignment") + { + Regex copy; + REQUIRE(copy.compile("unrelated")); + copy = original; + + RegexMatches matches; + int const rc = copy.exec(subject, matches); + CAPTURE(rc); + CHECK(rc == original_rc); + } + + SECTION("a copy of a copy") + { + Regex first(original); + Regex second(first); + RegexMatches matches; + int const rc = second.exec(subject, matches); + CAPTURE(rc); + CHECK(rc == original_rc); + } + + SECTION("a copy still matches what the original matches") + { + Regex copy(original); + std::string const ordinary{"/alpha/bravo/?action=weather"}; + + RegexMatches original_ordinary; + RegexMatches copy_ordinary; + CHECK(original.exec(ordinary, original_ordinary) == copy.exec(ordinary, copy_ordinary)); + } +} From 4c01e34661adbd4129a8e19f7262243016a0f5ac Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Sat, 12 Sep 2026 13:16:30 -0700 Subject: [PATCH 5/7] Regex: gate the concurrency test without std::latch The CentOS build runs devtoolset-10 and the Ubuntu build a clang of similar vintage, and libstdc++ did not ship until 11, so both failed to compile the new test with "latch: No such file or directory". Nothing else in the change reaches past C++17 in the library. Use a mutex and condition variable for the start gate instead. It keeps the property the latch was there for, that every thread is inside the match loop before any of them gets far, so the matching actually overlaps rather than running one thread at a time. --- src/tsutil/unit_tests/test_Regex.cc | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/tsutil/unit_tests/test_Regex.cc b/src/tsutil/unit_tests/test_Regex.cc index 5af2f9ed6ff..317aa85708a 100644 --- a/src/tsutil/unit_tests/test_Regex.cc +++ b/src/tsutil/unit_tests/test_Regex.cc @@ -21,7 +21,8 @@ */ #include -#include +#include +#include #include #include #include @@ -1194,7 +1195,14 @@ TEST_CASE("Regex matches concurrently on one instance", "[libts][Regex][threads] std::string const miss{"/Alpha/xx/tail"}; std::atomic failures{0}; - std::latch start{THREADS}; + + // A start gate, so every thread is inside the match loop before any of them gets far and + // the matching actually overlaps. std::latch would say this directly, but the oldest + // toolchain this project builds with does not carry . + std::mutex gate_mutex; + std::condition_variable gate; + int arrived = 0; + bool go = false; std::vector threads; threads.reserve(THREADS); @@ -1206,7 +1214,15 @@ TEST_CASE("Regex matches concurrently on one instance", "[libts][Regex][threads] RegexMatchContext context; RegexMatchContext const *const use = own_context ? &context : nullptr; - start.arrive_and_wait(); + { + std::unique_lock lock{gate_mutex}; + if (++arrived == THREADS) { + go = true; + gate.notify_all(); + } else { + gate.wait(lock, [&]() { return go; }); + } + } for (int n = 0; n < ITERATIONS; ++n) { RegexMatches matches; From 4355035f6d2e1002ba45cf438c6d87f996615c4d Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Sat, 12 Sep 2026 13:34:25 -0700 Subject: [PATCH 6/7] Regex: guard a failed copy, and share one caller context across the test threads pcre2_code_copy() returns null when it cannot obtain memory, and the copy constructor passed that straight to pcre2_jit_compile(). Check it, and leave the object empty when the copy fails: that is the state a default constructed Regex is in and the state empty() reports, rather than a Regex holding a null pattern. The concurrency test built a RegexMatchContext inside each worker, so it never covered the shape the plugins actually use, where one context is created at configuration load and every net thread matches through it. An implementation that cached a JIT stack in the context rather than resolving one per thread through the callback would have passed the old test and corrupted the real thing. Build one context before the workers start and hand the same pointer to every thread that uses one. --- src/tsutil/Regex.cc | 21 +++++++++++++-------- src/tsutil/unit_tests/test_Regex.cc | 12 ++++++++---- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index 542f63c3568..583f4f4ed44 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -391,14 +391,19 @@ Regex::Regex(Regex const &other) // Use PCRE2's built-in function to deep copy the compiled pattern auto *copied_code = pcre2_code_copy(other_code); - // pcre2_code_copy() does not carry the machine code the JIT produced, because that - // code is position dependent. Without this the copy would match on the interpreter: - // same answers, much slower, and a different set of resource limits, so a pattern - // that reports a JIT stack limit through the original would quietly match through - // the copy. Compile it again, exactly as Regex::compile() does for a new pattern. - pcre2_jit_compile(copied_code, PCRE2_JIT_COMPLETE); - - _Code::set(_code, copied_code); + // pcre2_code_copy() returns null when it cannot obtain memory. Leave the object empty + // in that case, which is the state a default constructed Regex is in and which + // empty() reports truthfully, rather than compiling a null pattern. + if (copied_code != nullptr) { + // pcre2_code_copy() does not carry the machine code the JIT produced, because that + // code is position dependent. Without this the copy would match on the interpreter: + // same answers, much slower, and a different set of resource limits, so a pattern + // that reports a JIT stack limit through the original would quietly match through + // the copy. Compile it again, exactly as Regex::compile() does for a new pattern. + pcre2_jit_compile(copied_code, PCRE2_JIT_COMPLETE); + + _Code::set(_code, copied_code); + } } } diff --git a/src/tsutil/unit_tests/test_Regex.cc b/src/tsutil/unit_tests/test_Regex.cc index 317aa85708a..f97dc8091ab 100644 --- a/src/tsutil/unit_tests/test_Regex.cc +++ b/src/tsutil/unit_tests/test_Regex.cc @@ -1204,15 +1204,19 @@ TEST_CASE("Regex matches concurrently on one instance", "[libts][Regex][threads] int arrived = 0; bool go = false; + // One caller-supplied context, built here and shared by half the threads. That is the + // production shape: regex_remap builds a context when it loads a rule and every net + // thread then matches through it. A context that cached a JIT stack directly rather than + // resolving one per thread through the callback would pass a test that gave each thread + // its own context, and would corrupt this one. + RegexMatchContext shared_caller_context; + std::vector threads; threads.reserve(THREADS); for (int i = 0; i < THREADS; ++i) { threads.emplace_back([&, i]() { - // Half the threads bring their own match context, which is a copy of the shared one - // and so carries the same JIT stack callback. bool const own_context = (i % 2) == 0; - RegexMatchContext context; - RegexMatchContext const *const use = own_context ? &context : nullptr; + RegexMatchContext const *const use = own_context ? &shared_caller_context : nullptr; { std::unique_lock lock{gate_mutex}; From 08d8be9be4ccbcfef147a5c1ef48c074a23ba8c3 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Sat, 12 Sep 2026 13:51:24 -0700 Subject: [PATCH 7/7] Regex: name the concurrency test's predicate for the branch it selects own_context was true for the threads matching through the one shared, caller-supplied context, which is the opposite of what the name says, and the shared case is the whole point of the test. Call it use_caller_context. Also record why the pcre2_jit_compile() result on a copy is not checked. A pattern the JIT declines still matches correctly on the interpreter, this class has no way to tell a caller which engine it got, and "no JIT" is not a single error code across PCRE2 versions and build options. compile() has the same property. Reporting the engine belongs to the replacement API rather than here. --- src/tsutil/Regex.cc | 6 ++++++ src/tsutil/unit_tests/test_Regex.cc | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index 583f4f4ed44..2c3fa7379ea 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -400,6 +400,12 @@ Regex::Regex(Regex const &other) // same answers, much slower, and a different set of resource limits, so a pattern // that reports a JIT stack limit through the original would quietly match through // the copy. Compile it again, exactly as Regex::compile() does for a new pattern. + // + // The result is not checked, for the same reason compile() does not check it: a + // pattern the JIT will not take still matches correctly on the interpreter, and this + // class has no way to tell a caller which engine it ended up with. Whether a build + // even has a JIT is not one error code either, so a check here would have to know + // three of them. Reporting the engine is what the replacement API adds. pcre2_jit_compile(copied_code, PCRE2_JIT_COMPLETE); _Code::set(_code, copied_code); diff --git a/src/tsutil/unit_tests/test_Regex.cc b/src/tsutil/unit_tests/test_Regex.cc index f97dc8091ab..1fd208b3d3c 100644 --- a/src/tsutil/unit_tests/test_Regex.cc +++ b/src/tsutil/unit_tests/test_Regex.cc @@ -1215,8 +1215,8 @@ TEST_CASE("Regex matches concurrently on one instance", "[libts][Regex][threads] threads.reserve(THREADS); for (int i = 0; i < THREADS; ++i) { threads.emplace_back([&, i]() { - bool const own_context = (i % 2) == 0; - RegexMatchContext const *const use = own_context ? &shared_caller_context : nullptr; + bool const use_caller_context = (i % 2) == 0; + RegexMatchContext const *const use = use_caller_context ? &shared_caller_context : nullptr; { std::unique_lock lock{gate_mutex};