You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bneradt (via GitHub)" <gi...@apache.org> on 2026/09/29 20:19:39 UTC
[PR] Replace RegexMatchContext with Regex::Options [trafficserver]
bneradt opened a new pull request, #13748:
URL: https://github.com/apache/trafficserver/pull/13748
RegexMatchContext made every caller own a heap-allocated PCRE2 match
context just to set a match limit. Its constructor started from a
blank context rather than the shared one, so callers silently lost
the shared 1 MiB JIT stack, and its hand-rolled copy and defaulted
move members carried a latent null copy and double free. Passing a
null context meant "use the shared one," while the shared general
context wrapped malloc and free in a no-op allocator whose null would
have meant the same thing.
This patch replaces the type with Regex::Options, a plain value whose
zero fields mean the shared default. Regex now owns the only match
contexts: a default Options matches through the shared context, and a
non-default one is applied to a per-thread copy of it, so the JIT
stack and any future shared configuration carry over. The no-op
allocator is removed. The esi URL validator now passes its match
limit as Options, and regex_remap, which no longer set one, simply
uses the shared context.
Because regex_remap now gets the 1 MiB JIT stack, its 3 KB URL test
case redirects as the rule is written. The autest now expects that
redirect, and the #5762 crash property moves to a unit test that
asserts exhaustion is reported as an error. New unit tests also
cover that Options do not leak between calls and keep the shared
JIT stack.
Fixes: #13663
Co-authored-by: Claude Opus 5.5 Medium
Related: #13660, #13683. This overlaps #13683, which fixes the same JIT stack loss while keeping RegexMatchContext; whichever lands second will need a rebase.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace RegexMatchContext with Regex::Options [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace RegexMatchContext with Regex::Options [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace RegexMatchContext with Regex::Options [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13748:
URL: https://github.com/apache/trafficserver/pull/13748#issuecomment-6000473024
Thanks @bryancall. Addressed the non-blocking points in e705ed01a8:
- **Allocation failure:** the constructor guards the JIT stack assignment and the copy, and a non-default `Options` whose context could not be allocated returns `PCRE2_ERROR_NOMEMORY`, so ESI fails closed.
- **Upgrading note:** added under "Upgrading to ATS v11.x", next to the `ts::Metrics` entry.
- **JIT-stack test margin:** the subject is now 8000 characters.
- **Exhaustion assertion:** now `PCRE2_ERROR_JIT_STACKLIMIT` exactly, with a comment on why nothing is asserted without JIT.
- **Future `Options` fields:** `Options` has a defaulted `operator==`, the default check compares against `Options{}`, and the comment says each field must be applied on every call.
- **Leftovers:** the no-argument `get_match_context()` is used again by the default path, the stale `ovector` paragraph is gone from all three `exec` docs, and the test data is renamed.
I kept the separate 3-argument overload instead of `Options const &opts = {}`. A default argument of a nested type with default member initializers hits CWG 1397 and fails to compile inside the enclosing class.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org