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 3281622b1ec..5726b490ccb 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,54 @@ 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) { + stack = pcre2_jit_stack_create(4096, 1024 * 1024, nullptr); // 1 page min and 1MB max + pthread_setspecific(jit_stack_key, stack); + } + return stack; +} + //---------------------------------------------------------------------------- class RegexContext { @@ -100,9 +149,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 +172,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,37 +301,15 @@ 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); } -//---------------------------------------------------------------------------- -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() { 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(