bneradt commented on code in PR #13748:
URL: https://github.com/apache/trafficserver/pull/13748#discussion_r4186237937
##########
src/tsutil/Regex.cc:
##########
@@ -120,19 +101,35 @@ class RegexContext
return _match_context;
}
+ /** The shared match context with @a opts applied.
+ *
+ * Default @a opts get the shared context itself. Otherwise @a opts is
applied to a copy of it, so everything the shared
+ * context configures, the JIT stack included, carries over and only what @a
opts sets differs. Every field is reapplied
+ * on each call, so nothing from a previous caller's @a opts leaks into this
one.
+ */
+ pcre2_match_context *
+ get_match_context(Regex::Options const &opts)
+ {
+ if (opts.match_limit == 0 || _options_match_context == nullptr) {
+ return _match_context;
Review Comment:
Agreed, fixed in 96d14df42c. A non-default `Options` whose context could not
be allocated now makes `exec` return `PCRE2_ERROR_NOMEMORY` instead of matching
without the limit, so the ESI allowlist fails closed. The constructor also
guards the JIT stack assignment and the copy against a failed
`pcre2_match_context_create`.
##########
src/tsutil/unit_tests/test_Regex.cc:
##########
@@ -959,17 +960,72 @@ std::vector<match_context_test_t> match_context_test_data{
{{"(."}, {"a"}, false, -51},
};
-TEST_CASE("RegexMatchContext", "[libts][Regex][RegexMatchContext]")
+TEST_CASE("Regex::Options match limit", "[libts][Regex][Options]")
{
- RegexMatchContext match_context;
- match_context.set_match_limit(2);
+ Regex::Options opts;
+ opts.match_limit = 2;
RegexMatches matches;
auto item = GENERATE(from_range(match_context_test_data));
CAPTURE(item.regex, item.str, item.valid, item.rcode);
Regex r;
REQUIRE(r.compile(item.regex) == item.valid);
- REQUIRE(r.exec(item.str, matches, 0, &match_context) == item.rcode);
+ REQUIRE(r.exec(item.str, matches, 0, opts) == item.rcode);
+}
+
+TEST_CASE("Regex::Options does not leak between calls",
"[libts][Regex][Options]")
+{
+ Regex r;
+ REQUIRE(r.compile(R"(^(\d{3})-(\d{3})-(\d{4})$)"));
+
+ RegexMatches matches;
+ Regex::Options limited;
+ limited.match_limit = 2;
+
+ REQUIRE(r.exec("123-456-7890", matches, 0, limited) == -47);
+ REQUIRE(r.exec("123-456-7890", matches, 0) == 4);
+ REQUIRE(r.exec("123-456-7890", matches, 0, Regex::Options{}) == 4);
+
+ Regex::Options generous;
+ generous.match_limit = 1000;
+ REQUIRE(r.exec("123-456-7890", matches, 0, generous) == 4);
+}
+
+TEST_CASE("Regex::Options keeps the shared JIT stack",
"[libts][Regex][Options]")
+{
+ Regex r;
+ REQUIRE(r.compile(R"(^(?:(a)|b)*$)"));
+
+ std::string const subject(1000, 'a');
+ RegexMatches matches;
+ Regex::Options opts;
+ opts.match_limit = 100000000;
+
+ int const shared_rc = r.exec(subject, matches, 0);
+ int const opts_rc = r.exec(subject, matches, 0, opts);
+ CAPTURE(shared_rc, opts_rc);
+ REQUIRE(opts_rc > 0);
+ REQUIRE(shared_rc == opts_rc);
+}
+
+TEST_CASE("Regex reports resource exhaustion rather than crashing",
"[libts][Regex]")
+{
+ // The regex_remap rule from #5762, whose per character backtracking once
exhausted the JIT stack and crashed.
Review Comment:
Right, #5762 was PCRE1 recursion overflowing the native thread stack. Fixed
in 96d14df42c: the unit test and autest comments now say that, and that the
test checks PCRE2 JIT stack exhaustion is reported as an error.
--
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]
