From 1d105a59131a54ae289d86acadf8330380b99bc6 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Sat, 12 Sep 2026 12:22:11 -0700 Subject: [PATCH 1/5] 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 6efbf77f3e58f42f6fd6d334d9a0c419fdac2856 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Tue, 15 Sep 2026 08:34:58 -0700 Subject: [PATCH 2/5] Regex: gate the crash-guard autest run on a PCRE2 that has a JIT The crash-guard run sends a 64 KB query and asserts two things: that the rule does not redirect, and that regex_remap logs a resource-limit error for it. Both hold only when PCRE2 can run the pattern on the just-in-time engine, because that is the only engine with a stack to exhaust here. The interpreter keeps its backtracking frames on the heap, so on a build without JIT the subject simply matches, the rule redirects, and the run fails for a reason that has nothing to do with what it tests. The unit tests added alongside it already skip themselves on the same condition; this run did not, so a PCRE2 built without JIT would fail the suite. Report whether PCRE2 has a JIT as TS_HAS_PCRE2_JIT from traffic_layout, which already includes pcre2.h and links it through tsutil, and gate the run on it the way the QUIC and Brotli runs gate on their features. Nothing else in the file depends on the JIT: the 3 KB redirect run gets the same answer from either engine, and the crash property itself is still covered without a JIT by the match-limit run and by the unit test that asserts it directly. --- src/traffic_layout/info.cc | 9 ++++++ .../regex_remap/regex_remap.test.py | 32 +++++++++++++------ 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/src/traffic_layout/info.cc b/src/traffic_layout/info.cc index 91b0677e042..6405a172440 100644 --- a/src/traffic_layout/info.cc +++ b/src/traffic_layout/info.cc @@ -170,6 +170,15 @@ produce_features(bool json) print_feature("TS_IP_TRANSPARENT", TS_IP_TRANSPARENT, json); print_feature("TS_HAS_128BIT_CAS", TS_HAS_128BIT_CAS, json); print_feature("TS_HAS_TESTS", TS_HAS_TESTS, json); + // Whether PCRE2 can run a pattern on the just-in-time engine. This is a property of how + // PCRE2 itself was built, not of ATS, and it decides which resource limit a pathological + // pattern reaches: the JIT stack, or nothing at all, because the interpreter keeps its + // backtracking frames on the heap. Tests that assert on one of those limits need to know. + { + uint32_t has_jit = 0; + pcre2_config(PCRE2_CONFIG_JIT, &has_jit); + print_feature("TS_HAS_PCRE2_JIT", has_jit != 0, json); + } print_feature("TS_MAX_THREADS_IN_EACH_THREAD_TYPE", TS_MAX_THREADS_IN_EACH_THREAD_TYPE, json); print_feature("TS_MAX_NUMBER_EVENT_THREADS", TS_MAX_NUMBER_EVENT_THREADS, json); print_feature("TS_MAX_HOST_NAME_LEN", TS_MAX_HOST_NAME_LEN, json); 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 3f740765407..639f5c5d543 100644 --- a/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py +++ b/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py @@ -129,6 +129,10 @@ # 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. +# +# This run needs no JIT gate. With JIT the 1 MB stack resolves the subject, and +# without JIT the interpreter resolves it on the heap, so the redirect is the +# answer either way. It only demonstrates the fix on a build that has a JIT. tr = Test.AddTestRun("long query redirects rather than exhausting the JIT stack") creq = replay_txns[1]['client-request'] tr.MakeCurlCommand( @@ -144,16 +148,24 @@ # 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") -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( - r'ERROR: \[regex_remap\] Bad regular expression result -(?:46|47|53|63).*"\^/alpha/bravo/', - "The crash-guard rule must report resource exhaustion") -tr.StillRunningAfter = ts +# +# Only the JIT engine has a stack to exhaust here. PCRE2's interpreter keeps its +# backtracking frames on the heap, so on a build without JIT this subject simply +# matches, the rule redirects, and both assertions below fail for a reason that has +# nothing to do with the behaviour under test. The crash property itself is not lost +# on such a build: run 4 reaches the match limit through the interpreter, and the +# unit test in test_Regex.cc asserts it directly. +if Condition.HasATSFeature('TS_HAS_PCRE2_JIT'): + crash_guard_query = 'x' * 64000 + tr = Test.AddTestRun("resource exhaustion does not crash ATS") + 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( + r'ERROR: \[regex_remap\] Bad regular expression result -(?:46|47|53|63).*"\^/alpha/bravo/', + "The crash-guard rule must report resource exhaustion") + tr.StillRunningAfter = ts # 4 Test - The nested quantifiers must exceed PCRE2's default matching-work limit. tr = Test.AddTestRun("excessive backtracking reaches the match limit") From c4cdac1110e9a708c3982c5a6a2852a672fbd272 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Tue, 15 Sep 2026 08:40:35 -0700 Subject: [PATCH 3/5] Regex: exercise one shared match context from eight threads The two tests added with the pthread key both match on a single thread, so an implementation that handed the same JIT stack to every thread would pass them. That is the one regression this change exists to prevent, and nothing covered it. Eight threads match on one Regex, half of them through a single caller-supplied context built before the workers start. That is the production shape: regex_remap builds a context when it loads a rule and every net thread then matches through it, so a context that cached a stack rather than resolving one per thread through the callback would pass a test that gave each thread its own context and corrupt this one. Every thread must reach the same verdict, and under ThreadSanitizer the run must also be clean. The start gate is a mutex and condition variable rather than std::latch, which says it more directly but is not in libstdc++ before 11, and the CentOS build runs devtoolset-10. 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 overlaps instead of running one thread at a time. --- src/tsutil/unit_tests/test_Regex.cc | 76 +++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/src/tsutil/unit_tests/test_Regex.cc b/src/tsutil/unit_tests/test_Regex.cc index ed3d841936d..5d848af28f6 100644 --- a/src/tsutil/unit_tests/test_Regex.cc +++ b/src/tsutil/unit_tests/test_Regex.cc @@ -20,7 +20,12 @@ limitations under the License. */ +#include +#include +#include +#include #include +#include #include #define PCRE2_CODE_UNIT_WIDTH 8 @@ -1139,3 +1144,74 @@ 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}; + + // 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; + + // 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]() { + 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}; + if (++arrived == THREADS) { + go = true; + gate.notify_all(); + } else { + gate.wait(lock, [&]() { return go; }); + } + } + + 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 c3b4f4d444534c0834b309550b88bfba877d2cb4 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Tue, 15 Sep 2026 12:21:51 -0700 Subject: [PATCH 4/5] Regex: shorten the JIT stack size comment Eight lines of measurements above one call. Keep the two facts a reader needs to judge the number, that the maximum is reserved rather than committed and that a mebibyte already outruns what a client can send, and leave the measured table in the pull request. The rationale for holding the stack in a pthread key stays where it is, because that is the part someone would otherwise simplify back into a deadlock. --- src/tsutil/Regex.cc | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index 30a126da6fe..86ff9931994 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -122,14 +122,10 @@ jit_stack_for_this_thread(void *) 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. + // One page to start, one mebibyte at most. The maximum is address space reserved at + // creation and made resident only as deep as a match actually goes, so a larger one + // costs nothing per match, and a mebibyte already resolves a longer subject than + // proxy.config.http.request_header_max_size lets a client send. 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 From 062b74668282395fa7b6ef72cb77d14b733a95a2 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Thu, 17 Sep 2026 15:19:42 -0700 Subject: [PATCH 5/5] Regex: make the JIT stack tests able to fail, and stop the null copy The concurrent test asserted nothing about the JIT stack: a shared stack passed it 320,000 times and ThreadSanitizer cannot see sljit-generated writes. Its subjects now need the 1 MiB stack, so losing the callback fails it. The crash-guard case gains the positive control that tells a 1 MiB stack from the 32 KiB fallback, since 256 KiB overran both, and the copy paths gain the coverage that let a blank copy go unnoticed. pcre2_match_context_copy dereferences its argument, so an unchecked shared context turned an allocation failure into a crash where the old blank context degraded. Gate the autest on the JIT allocator rather than on PCRE2_CONFIG_JIT, which reports success on a hardened runtime. The crash-guard query stays at 64,000. Raising request_line_max_size to buy margin instead trips the parser's assert that the request line fits in a uint16, which aborts traffic_server on an assert-enabled build. --- src/traffic_layout/info.cc | 18 +- src/tsutil/Regex.cc | 14 +- src/tsutil/unit_tests/test_Regex.cc | 171 ++++++++++++++---- .../regex_remap/regex_remap.test.py | 24 ++- 4 files changed, 179 insertions(+), 48 deletions(-) diff --git a/src/traffic_layout/info.cc b/src/traffic_layout/info.cc index 6405a172440..d023a755d01 100644 --- a/src/traffic_layout/info.cc +++ b/src/traffic_layout/info.cc @@ -170,13 +170,23 @@ produce_features(bool json) print_feature("TS_IP_TRANSPARENT", TS_IP_TRANSPARENT, json); print_feature("TS_HAS_128BIT_CAS", TS_HAS_128BIT_CAS, json); print_feature("TS_HAS_TESTS", TS_HAS_TESTS, json); - // Whether PCRE2 can run a pattern on the just-in-time engine. This is a property of how - // PCRE2 itself was built, not of ATS, and it decides which resource limit a pathological - // pattern reaches: the JIT stack, or nothing at all, because the interpreter keeps its - // backtracking frames on the heap. Tests that assert on one of those limits need to know. + // Whether PCRE2 can run a pattern on the just-in-time engine. This is a property of the + // PCRE2 that ATS is linked against, not of ATS, and it decides which resource limit a + // pathological pattern reaches: the JIT stack, or the interpreter's far larger match, + // depth and heap limits, because the interpreter keeps its backtracking frames on the + // heap. Tests that assert on one of those limits need to know. + // + // PCRE2_JIT_TEST_ALLOC (PCRE2 10.45) also confirms the JIT can allocate executable + // memory. PCRE2_CONFIG_JIT does not, and reports success on a hardened runtime where + // every JIT compile then fails, which is the direction that misleads a test gate. { uint32_t has_jit = 0; + +#ifdef PCRE2_JIT_TEST_ALLOC + has_jit = pcre2_jit_compile(nullptr, PCRE2_JIT_TEST_ALLOC) == 0; +#else pcre2_config(PCRE2_CONFIG_JIT, &has_jit); +#endif print_feature("TS_HAS_PCRE2_JIT", has_jit != 0, json); } print_feature("TS_MAX_THREADS_IN_EACH_THREAD_TYPE", TS_MAX_THREADS_IN_EACH_THREAD_TYPE, json); diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index 86ff9931994..b68eb223373 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -313,9 +313,17 @@ RegexMatchContext::RegexMatchContext() // 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"); + // one. Callers override only the fields they mean to. + // + // pcre2_match_context_copy dereferences its argument rather than returning null + // for one, and RegexContext's constructor does not check its allocations, so a + // shared context that failed to allocate would crash here. Fall back to a blank + // context, which is what this constructor built before and which no reader of + // _match_context dereferences unchecked. + auto *shared = RegexContext::get_instance()->get_match_context(); + auto *ctx = shared != nullptr ? pcre2_match_context_copy(shared) : pcre2_match_context_create(nullptr); + + debug_assert_message(ctx, "Failed to obtain a 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 5d848af28f6..2d7555e5937 100644 --- a/src/tsutil/unit_tests/test_Regex.cc +++ b/src/tsutil/unit_tests/test_Regex.cc @@ -1064,6 +1064,11 @@ namespace * 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. + * + * This asks about one pattern, deliberately: PCRE2 declines some pattern items and + * leaves PCRE2_INFO_JITSIZE at zero on a build whose JIT is otherwise fine, so the + * library-wide probes (pcre2_config, PCRE2_JIT_TEST_ALLOC) answer a different + * question. Call it from the test thread only; Catch2 assertions are not thread safe. */ bool pattern_has_jit(char const *pattern) @@ -1071,11 +1076,14 @@ 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; - } + + // A pattern that will not compile is a broken test, not a build without a JIT. + // Reporting it as "no JIT" would turn a typo into a silent skip. + REQUIRE(code != nullptr); + 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; @@ -1098,7 +1106,11 @@ TEST_CASE("RegexMatchContext matches the shared context", "[libts][Regex][RegexM Regex re; REQUIRE(re.compile(pattern)); - std::string const subject(1000, 'a'); + // 40 bytes of JIT stack per subject character, so PCRE2's 32KiB fallback stops at + // 818 characters and the shared context's 1MiB stack at 26,213. 5,000 sits six + // times above the first and five times below the second, so a blank context and + // the shared one give different answers with margin either way. + std::string const subject(5000, 'a'); RegexMatches shared_matches; RegexMatchContext match_context; @@ -1110,12 +1122,25 @@ TEST_CASE("RegexMatchContext matches the shared context", "[libts][Regex][RegexM REQUIRE(shared_rc > 0); REQUIRE(own_rc == shared_rc); + + // The copy paths must carry the inherited configuration too. Building a blank + // context in either of them is the same defect, and nothing else here notices. + RegexMatchContext const copied{match_context}; + RegexMatchContext assigned; + + assigned = match_context; + + RegexMatches copied_matches; + RegexMatches assigned_matches; + + REQUIRE(re.exec(subject, copied_matches, 0, &copied) == shared_rc); + REQUIRE(re.exec(subject, assigned_matches, 0, &assigned) == 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. +// instead. 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 @@ -1130,9 +1155,9 @@ TEST_CASE("Regex reports resource exhaustion rather than crashing", "[libts][Reg 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. + // stack, measured identically on x86_64 and arm64. Smaller JIT frames get more + // subject out of the same stack and so push that threshold up; 256KiB keeps a + // six times margin against that. Do not trim this to just above 43KiB. std::string subject{"/alpha/bravo/?"}; subject.append(256 * 1024, 'x'); @@ -1143,17 +1168,99 @@ TEST_CASE("Regex reports resource exhaustion rather than crashing", "[libts][Reg // Reaching this line at all is the crash assertion. REQUIRE(rc < 0); REQUIRE(rc != RE_ERROR_NOMATCH); + + // The paired positive control, and the only part of this case that can tell a + // 1MiB stack from PCRE2's 32KiB fallback: 256KiB overruns both bounds, so every + // assertion above also holds with the JIT stack callback deleted. 10,000 is seven + // times over the fallback's bound and four times under the 1MiB stack's. Neither + // bound pins 1MiB itself; the suite only requires a maximum in roughly + // [240KB, 6MB]. + std::string smaller{"/alpha/bravo/?"}; + smaller.append(10000, 'x'); + + RegexMatches small_matches; + int const small_rc = re.exec(smaller, small_matches); + CAPTURE(small_rc); + + REQUIRE(small_rc > 0); +} + +namespace +{ +/** 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; it is + * hand-rolled here to match notstd::barrier in benchmark_LogObject.cc. + * + * Single-use, like the std::latch it stands in for: once the gate opens it stays open + * and a second round returns immediately. Exactly `expected` threads must call + * arrive_and_wait() or the rest block forever, and the gate must outlive all of them, + * since destroying a condition_variable with waiters on it is undefined. + */ +class ThreadGate +{ +public: + explicit ThreadGate(int expected) : _expected{expected} { REQUIRE(expected > 0); } + + /// Count this thread in, then block until every expected thread has done the same. + void + arrive_and_wait() + { + std::unique_lock lock{_mutex}; + + if (++_arrived == _expected) { + _open = true; + _cv.notify_all(); + return; + } + _cv.wait(lock, [this]() { return _open; }); + } + +private: + std::mutex _mutex; + std::condition_variable _cv; + int const _expected; + int _arrived = 0; + bool _open = false; +}; + +/** A subject the deep pattern below backtracks through once per character. + * + * At 40 bytes of JIT stack per character the longest of these needs about 223KiB: six + * times what PCRE2's 32KiB fallback resolves, and under a quarter of the shared + * context's 1MiB stack. Each thread gets its own length and its own content, so no two + * threads are matching identical bytes. + */ +std::string +stack_hungry_subject(int thread_index) +{ + std::string subject(5000 + thread_index * 100, 'a'); + + subject[11 + thread_index] = 'b'; + return subject; } +} // namespace -// 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. +// The header promises that exec() may be called concurrently on one instance. Every thread +// must reach the same verdict, whether it matches through a caller-supplied context shared +// by several threads or through the thread-global one. +// +// The deep subjects are longer than PCRE2's 32KiB fallback stack can resolve, so on a JIT +// build this also fails if the callback stops handing every thread a 1MiB stack: deleting +// the callback, or returning null from it, turns every deep match here into +// PCRE2_ERROR_JIT_STACKLIMIT. Without a JIT the interpreter resolves them either way, so +// only the concurrency half of this case carries over to such a build. +// +// It does not establish that the stacks are distinct. One stack shared by all eight threads +// corrupts a result only when two matches are deep at the same instant, so it fails +// intermittently rather than reliably and is not an oracle for that. ThreadSanitizer does +// not close the gap either: the racing writes come from sljit-generated code it never sees. TEST_CASE("Regex matches concurrently on one instance", "[libts][Regex][threads]") { - Regex re; - REQUIRE(re.compile(R"(^/([a-z]+)/([0-9]+)/(.*)$)")); + Regex re_captures; + REQUIRE(re_captures.compile(R"(^/([a-z]+)/([0-9]+)/(.*)$)")); + + Regex re_deep; + REQUIRE(re_deep.compile(R"(^(?:(a)|(b))+$)")); constexpr int THREADS = 8; constexpr int ITERATIONS = 2000; @@ -1162,20 +1269,11 @@ TEST_CASE("Regex matches concurrently on one instance", "[libts][Regex][threads] std::string const miss{"/Alpha/xx/tail"}; std::atomic failures{0}; - - // 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; + ThreadGate gate{THREADS}; // 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. + // thread then matches through it. RegexMatchContext shared_caller_context; std::vector threads; @@ -1184,25 +1282,24 @@ TEST_CASE("Regex matches concurrently on one instance", "[libts][Regex][threads] threads.emplace_back([&, i]() { bool const use_caller_context = (i % 2) == 0; RegexMatchContext const *const use = use_caller_context ? &shared_caller_context : nullptr; + std::string const deep = stack_hungry_subject(i); - { - std::unique_lock lock{gate_mutex}; - if (++arrived == THREADS) { - go = true; - gate.notify_all(); - } else { - gate.wait(lock, [&]() { return go; }); - } - } + gate.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") { + if (re_captures.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) { + if (re_captures.exec(miss, no_matches, 0, use) != RE_ERROR_NOMATCH) { + ++failures; + } + + // Needs the 1MiB stack, and must consume the whole subject to have used it. + RegexMatches deep_matches; + if (re_deep.exec(deep, deep_matches, 0, use) <= 0 || deep_matches[0].size() != deep.size()) { ++failures; } } 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 639f5c5d543..41415778f61 100644 --- a/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py +++ b/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py @@ -89,7 +89,11 @@ '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', - # The crash-guard run below needs a request larger than the 32 KB default. + # Run 3b sends a query past the 1 MB JIT stack's ~44 KB bound, which is over the + # 32 KB default. Do not raise request_line_max_size to match: http_parser_parse_req + # asserts `parsed.size() < UINT16_MAX` before it rejects an over-long line, so on + # any assert-enabled build a request line past 65,535 aborts traffic_server instead + # of getting a 414. 65,535 is the real ceiling here, whatever the record says. 'proxy.config.http.request_header_max_size': 131072 }) @@ -145,9 +149,21 @@ # 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). 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. +# shared 1 MB stack this rule needs a subject past ~44 KB to exhaust it (measured: +# 43,514 matches, 44,021 does not), which is why the header limit is raised above. +# Shortening this query silently turns the run into a plain redirect test. +# +# This run is boxed in, and the box cannot be widened by configuration. The floor is +# that ~44 KB bound; the ceiling is the 65,535 request line the parser asserts on. So +# the usable window is roughly 44,100 to 65,475 bytes of query, about 1.5x wide, and +# 64,000 is near the top of it. A platform whose JIT frames are enough smaller to push +# the floor over the ceiling cannot run this case at any query length: prefer the +# test_Regex.cc unit test, which bounds the stack directly, over stretching this one. +# +# Note this run is a crash guard, not a check on the stack size: its ContainsExpression +# accepts any of -46/-47/-53/-63, and the 32 KB fallback reaches -46 too. Run 3 above is +# what actually fails if the fix is reverted, because its 3 KB subject redirects on the +# 1 MB stack and errors on the fallback. # # Only the JIT engine has a stack to exhaust here. PCRE2's interpreter keeps its # backtracking frames on the heap, so on a build without JIT this subject simply