From 8729df3553eb28f35ca503fff2dfc117376833ec Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Mon, 14 Sep 2026 21:14:54 -0700 Subject: [PATCH 1/2] 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. plugins/experimental/maxmind_acl copies every rule. 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. pcre2_code_copy() also 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 pcre2_jit_compile() result is deliberately 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. 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 | 21 +++++++++- src/tsutil/unit_tests/test_Regex.cc | 64 +++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 2 deletions(-) diff --git a/include/tsutil/Regex.h b/include/tsutil/Regex.h index 5b913bece06..c9c148ef181 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 3281622b1ec..91e1c71f425 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -330,7 +330,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); + } } } diff --git a/src/tsutil/unit_tests/test_Regex.cc b/src/tsutil/unit_tests/test_Regex.cc index f1bd0a7c866..5819131994e 100644 --- a/src/tsutil/unit_tests/test_Regex.cc +++ b/src/tsutil/unit_tests/test_Regex.cc @@ -1050,3 +1050,67 @@ 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); } + +// 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 8c76dfb70d002605c921f73462f7b29f50a60c80 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Tue, 15 Sep 2026 12:20:31 -0700 Subject: [PATCH 2/2] Regex: cut the copy constructor comment down Fifteen lines of comment above three lines of code, most of it repeating the paragraph already in the test that covers this. Keep what is not evident from the code, which is that a null copy means pcre2 ran out of memory, that the JIT compile is what stops the copy running on a different engine than the original, and that its result is deliberately unchecked. The rest is in the commit that made the change and in the test. --- src/tsutil/Regex.cc | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index 91e1c71f425..70b7e990d84 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -331,21 +331,12 @@ 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() 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. + // Null when pcre2 could not obtain memory. Leave the object empty rather than + // holding 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. + // The copy does not carry the machine code the JIT produced, so without this it + // matches on the interpreter, under different resource limits than the original. + // Unchecked for the same reason compile() does not check it. pcre2_jit_compile(copied_code, PCRE2_JIT_COMPLETE); _Code::set(_code, copied_code);