You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bryancall (via GitHub)" <gi...@apache.org> on 2026/09/15 04:43:34 UTC
[PR] Regex: keep the compiled pattern until a recompile succeeds [trafficserver]
bryancall opened a new pull request, #13684:
URL: https://github.com/apache/trafficserver/pull/13684
Third of four, split out of #13671 at review request. Independent of the other three;
it touches neither the shared contexts nor the JIT stack.
### A failed recompile left freed memory in the object
`Regex::compile()` freed the pattern it already held *before* calling `pcre2_compile()`.
Every failure path after that point returned with the freed pointer still stored, so
`empty()` reported the object as compiled, `exec()` passed the freed block to
`pcre2_match()`, and the destructor freed it a second time.
Compile into a local and replace the member only after the new pattern exists. A failed
compile now leaves the previous pattern in place and usable, which is what a caller
checking the return value would expect, and a fresh object that fails to compile is still
empty. The header says so on both overloads; it did not before.
### Tests
Two sections. A valid compile followed by a failing one must leave the first pattern
matching, including its capture groups, and the object must still accept a later
successful compile.
Built against the unfixed implementation the first of those segmentation faults, which is
the point: it is a use-after-free reachable from any caller that recompiles, and
`remap.config` reload is one.
### Verification
Fedora 44, gcc 16.2.1, PCRE2 10.47. `[Regex]` clean under AddressSanitizer with UBSan.
--
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] Regex: keep the compiled pattern until a recompile succeeds [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #13684:
URL: https://github.com/apache/trafficserver/pull/13684#issuecomment-5685590097
Added in b255d770d8. The Copilot re-review called this a non-blocking nit, but it was asking for the one thing that was missing, so it is in.
`test_ControlMatcher.cc` covered `UrlMatcher` only. The new case drives the exact sequence: a line that compiles its regex and then fails `Data::Init()`, followed by a line whose own pattern does not compile landing on the same slot. It asserts the second line is rejected, that `num_el` stays at zero, and that the pattern the first line left behind does not match anything.
I checked it fails against the code it covers rather than just passing. With the check reverted to `empty()`, three assertions fail:
```
CHECK( matcher.NewEntry(&second_line).failed() ) FAILED
CHECK( matcher.num_el == 0 ) FAILED
CHECK_FALSE( result.never_cache ) FAILED
```
The last one is the point: the stale pattern is not merely stored, it matches, under the second line's configuration. With the fix, `[ControlMatcher]` is 19 assertions in 4 cases, all passing.
--
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] Regex: keep the compiled pattern until a recompile succeeds [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13684:
URL: https://github.com/apache/trafficserver/pull/13684#discussion_r4018787701
##########
src/proxy/ControlMatcher.cc:
##########
@@ -424,9 +424,13 @@ RegexMatcher<Data, MatchResult>::NewEntry(matcher_line *line_info)
ink_assert(line_info->dest_entry < MATCHER_MAX_TOKENS);
ink_assert(pattern != nullptr);
- // Create the compiled regular expression
- regex_array[num_el].compile(pattern, error_msg, erroffset);
- if (regex_array[num_el].empty()) {
+ // Create the compiled regular expression. Check what compile() returned rather than
+ // asking the object whether it holds a pattern: this slot is reused when a previous
+ // line's Data::Init() failed, because that path leaves num_el where it was, so the
+ // Regex here can still hold the pattern that line compiled. empty() would then be
+ // false for a line whose own pattern never compiled, and the entry would be accepted
+ // and matched against the earlier pattern under this line's configuration.
Review Comment:
Fair, and cut down in b255d770d8. It is two lines now, keeping only the part that is not obvious from the code, which is that the slot can still hold a previous line's pattern. The rest of the reasoning is in the commit message.
--
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] Regex: keep the compiled pattern until a recompile succeeds [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #13684:
URL: https://github.com/apache/trafficserver/pull/13684#discussion_r4017786633
##########
src/proxy/ControlMatcher.cc:
##########
@@ -424,9 +424,13 @@ RegexMatcher<Data, MatchResult>::NewEntry(matcher_line *line_info)
ink_assert(line_info->dest_entry < MATCHER_MAX_TOKENS);
ink_assert(pattern != nullptr);
- // Create the compiled regular expression
- regex_array[num_el].compile(pattern, error_msg, erroffset);
- if (regex_array[num_el].empty()) {
+ // Create the compiled regular expression. Check what compile() returned rather than
+ // asking the object whether it holds a pattern: this slot is reused when a previous
+ // line's Data::Init() failed, because that path leaves num_el where it was, so the
+ // Regex here can still hold the pattern that line compiled. empty() would then be
+ // false for a line whose own pattern never compiled, and the entry would be accepted
+ // and matched against the earlier pattern under this line's configuration.
Review Comment:
I'm not convinced this level of comment depth is warranted for this self-explanatory line of 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] Regex: keep the compiled pattern until a recompile succeeds [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall merged PR #13684:
URL: https://github.com/apache/trafficserver/pull/13684
--
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] Regex: keep the compiled pattern until a recompile succeeds [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #13684:
URL: https://github.com/apache/trafficserver/pull/13684#issuecomment-5683350847
Fixed in 473b8ddc2e. The Copilot finding was filed as a suppressed comment with no thread, so this is the reply.
I verified the claim in the source rather than taking it on trust, and it holds exactly as described. `RegexMatcher::NewEntry` discarded what `compile()` returned and used `empty()` as the compile-status check (`src/proxy/ControlMatcher.cc:428`), and the slot really is reused: when a line's `Data::Init()` fails, the function resets `regex_strings[num_el]` but leaves `num_el` alone, so the next line lands on the same `Regex`, still holding the pattern the rejected line compiled.
Worth being precise about what changed, because the caller was already broken. Before this PR a failed `compile()` left a *freed* pointer in `_code`, so `empty()` answered false there too and the bad line was accepted, then matched through freed memory. This PR does not introduce the bug; it changes the symptom from a use after free to a line that is accepted and matched against the previous pattern under its own configuration. Quieter, which is a good reason to fix the caller in the same change rather than leave it.
`NewEntry` now asks `compile()` whether it compiled.
I also checked the rest of the tree for the same shape. Five call sites discard the `compile()` result: `ControlMatcher.cc:428`, `SSLSNIConfig.cc:103`, `Diags.cc:375`, `Regression.cc:97`, and `regex_revalidate.cc:215`. This change is only observable when a `Regex` that already holds a pattern is recompiled and the new pattern fails, and those other four compile into a freshly constructed object, where a failed compile leaves it empty exactly as before. `ControlMatcher` is the one with a reachable reuse path, so it is the one that needed fixing.
`ctest -R proxy` passes 86/86 with the change and 86/86 on unmodified master, so the existing `test_ControlMatcher` coverage is unaffected.
--
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