diff --git a/include/tsutil/Regex.h b/include/tsutil/Regex.h index 5b913bece06..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. */ @@ -163,23 +164,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 3281622b1ec..2c3fa7379ea 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 @@ -80,29 +81,86 @@ my_free(void *ptr, void * /*caller*/) } //---------------------------------------------------------------------------- +// One match context is shared by every thread that matches through it, and PCRE2 +// requires a distinct JIT stack per thread, so the stack comes from a callback +// invoked at match time rather than a pointer baked in when the context is built. +// +// The 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; +} + +//---------------------------------------------------------------------------- +// 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); - } - if (_jit_stack != nullptr) { - pcre2_jit_stack_free(_jit_stack); - } + static RegexContext *const ctx = new RegexContext(); + return ctx; } pcre2_general_context * get_general_context() @@ -126,13 +184,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 +313,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); } @@ -330,7 +390,26 @@ 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); - _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. + // + // 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); + } } } @@ -394,16 +473,7 @@ 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); - } - - // 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 @@ -445,6 +515,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 f1bd0a7c866..1fd208b3d3c 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 @@ -646,6 +651,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; @@ -1050,3 +1088,227 @@ 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); +} + +// 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); +} + +// 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)); + } +} 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(