Copilot commented on code in PR #13683:
URL: https://github.com/apache/trafficserver/pull/13683#discussion_r4042118960
##########
src/tsutil/Regex.cc:
##########
@@ -257,8 +310,20 @@ 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. 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);
Review Comment:
Calling `RegexContext::get_instance()` here initializes the `thread_local
RegexContext`, whose non-trivial destructor registers via
`__cxa_thread_atexit`. That is the same loader-lock path this change is trying
to avoid, and it now occurs merely by constructing a caller context (including
from plugin/static-initialization code), whereas the old constructor did not
need it. Please copy from a process/shared context without initializing this
thread-local, or otherwise defer the TLS construction, before relying on this
standalone change.
##########
src/tsutil/unit_tests/test_Regex.cc:
##########
@@ -1147,3 +1152,260 @@ TEST_CASE("Regex copies answer the same as their
original", "[libts][Regex][copy
CHECK(original.exec(ordinary, original_ordinary) == copy.exec(ordinary,
copy_ordinary));
}
}
+
+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.
+ *
+ * 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)
+{
+ int errnum = 0;
+ PCRE2_SIZE erroffset = 0;
+ pcre2_code *code = pcre2_compile(reinterpret_cast<PCRE2_SPTR>(pattern),
PCRE2_ZERO_TERMINATED, 0, &errnum, &erroffset, nullptr);
+
+ // 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;
+}
+} // 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));
+
+ // 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;
+ 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 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;
Review Comment:
Please fix `RegexMatchContext::operator=` before exercising this path:
`assigned` already owns a PCRE2 match context, but the assignment overwrites
`_match_context` without freeing the old one (src/tsutil/Regex.cc:342-351).
This added test therefore leaks one context on every run and can fail
LeakSanitizer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
