Copilot commented on code in PR #13748:
URL: https://github.com/apache/trafficserver/pull/13748#discussion_r4149403370
##########
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:
When `pcre2_match_context_copy` fails allocation, this branch silently uses
the shared context even though a nonzero `match_limit` was requested. The ESI
caller relies on that limit to bound attacker-influenced backtracking, so one
initialization failure leaves that thread executing those matches without the
configured cap. Return an execution error such as `PCRE2_ERROR_NOMEMORY`, or
fail context initialization, rather than dropping requested options.
##########
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:
This misstates the original failure: #5762 involved PCRE1 exhausting ATS's
native thread stack, while JIT-stack exhaustion is reported as an error.
Distinguishing the mechanisms is important because this test now verifies the
latter behavior.
--
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]
