JosiahWI commented on code in PR #13683:
URL: https://github.com/apache/trafficserver/pull/13683#discussion_r4020795421
##########
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:
Remove the last sentence, please. I think the programmer can discern easily
enough that a failing test could mean a regression after reading the first part
of the comment.
##########
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:
This error handling is a risk for future bugs. Returning `false` from
`pattern_has_jit`, due to an error while compiling the pattern, communicates
the wrong idea to the programmer calling the function. I think it might be best
to throw an exception here, but returning a stronger error-carrying type or
doing a release assert are alternatives.
##########
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:
The PCRE2 library provides a mechanism specifically for a similar kind of
test, which simplifies this function implementation considerably. One caveat:
the API I'm going to mention is used to test general JIT support, but the
wording of your comment suggests JIT can be supported for some patterns and not
others. In that case, this API would not be applicable. Do you have a reference
for that?
> The availability of JIT support can be tested by calling
pcre2_compile_jit() with a single option PCRE2_JIT_TEST_ALLOC (the code
argument is ignored, so a NULL value is accepted). Such a call returns zero if
JIT is available and has a working allocator. Otherwise it returns
PCRE2_ERROR_NOMEMORY if JIT is available but cannot allocate executable memory,
or PCRE2_ERROR_JIT_UNSUPPORTED if JIT support is not compiled.
\- [PCRE2 API
reference](https://www.pcre.org/current/doc/html/pcre2_jit_compile.html)
##########
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:
I think some setup or other helpers could usefully be extracted from the
concurrent test.
--
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]
