bryancall commented on code in PR #13683:
URL: https://github.com/apache/trafficserver/pull/13683#discussion_r4066776157
##########
src/tsutil/unit_tests/test_Regex.cc:
##########
@@ -1050,3 +1055,163 @@ 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<PCRE2_SPTR>(pattern),
PCRE2_ZERO_TERMINATED, 0, &errnum, &erroffset, nullptr);
+ if (code == nullptr) {
+ return false;
+ }
Review Comment:
Agreed, and taken: it is a `REQUIRE(code != nullptr)` now, which is the
release-assert alternative you offered. A pattern that will not compile is a
broken test rather than a build without a JIT, and reporting it as "no JIT"
would turn a typo into a silent skip, so the comment says that too.
##########
src/tsutil/unit_tests/test_Regex.cc:
##########
@@ -1050,3 +1055,163 @@ 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<PCRE2_SPTR>(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.
Review Comment:
Removed.
##########
src/tsutil/unit_tests/test_Regex.cc:
##########
@@ -1050,3 +1055,163 @@ 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<PCRE2_SPTR>(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]")
+{
Review Comment:
Done in `9e73cb6`. The three inlined assertions in the thread body move to
`concurrent_match_round()`, so the loop is now just the call and the failure
count.
One behavioural note rather than bury it: the helper returns false on the
first disagreement, so the counter tallies rounds that went wrong rather than
individual assertions. The case asserts on zero either way, so the oracle is
unchanged, but the denominator is different if you ever read the number.
##########
src/tsutil/unit_tests/test_Regex.cc:
##########
@@ -1050,3 +1055,163 @@ 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<PCRE2_SPTR>(pattern),
PCRE2_ZERO_TERMINATED, 0, &errnum, &erroffset, nullptr);
+ if (code == nullptr) {
+ return false;
+ }
+ pcre2_jit_compile(code, PCRE2_JIT_COMPLETE);
Review Comment:
You asked for a reference, and your own caveat turns out to be the reason I
cannot use it. From `pcre2jit(3)`:
> having the JIT code available does not guarantee that it will be used for
any particular match. One reason for this is that there are a number of options
and pattern items that are not supported by JIT (see below).
The UNSUPPORTED OPTIONS AND PATTERN ITEMS section names `\C` in a UTF mode
and a callout immediately before an assertion condition in a conditional group.
So the two calls answer different questions: `PCRE2_JIT_TEST_ALLOC` answers "is
JIT compiled in and can it allocate executable memory", and what the gate needs
is "did *this* pattern get a JIT block", because the JIT stack is only
consulted for a pattern that was actually JIT compiled. Querying
`PCRE2_INFO_JITSIZE` after `pcre2_jit_compile()` answers that directly.
There is a second, independent reason. `PCRE2_JIT_TEST_ALLOC` arrived in
PCRE2 10.45. ATS pins no minimum version (`pkg_check_modules(PCRE2 REQUIRED
IMPORTED_TARGET libpcre2-8)`) and the `rocky` CI lane is Rocky Linux 8.10,
whose pcre2 predates 10.45, so it would not compile there.
Leaving this thread open rather than resolving it, since it is your call
whether that answer satisfies the question.
##########
src/tsutil/Regex.cc:
##########
@@ -278,11 +343,16 @@ 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);
+
+ // Take the copy before releasing what this object already holds, so a
failing
+ // copy leaves it holding its old context rather than a freed one.
Releasing it
+ // is what this operator used to omit, and every assignment leaked one
context.
+ pcre2_match_context *const ctx = nullptr != ptr ?
pcre2_match_context_copy(ptr) : nullptr;
+ pcre2_match_context *const old = _MatchContext::get(_match_context);
+
+ _MatchContext::set(_match_context, ctx);
+ if (old != nullptr) {
+ pcre2_match_context_free(old);
}
Review Comment:
Correct, and fixed in `9e73cb6`.
Worse than the report: the comment directly above claimed that taking the
copy first leaves the object holding its old context on failure, and the code
then assigned the null and freed the old one anyway. The ordering was already
right; what was missing was declining to swap when the copy comes back null,
which it now does.
The copy constructor has the same null return but no old context to lose, so
it is left alone deliberately.
--
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]
