Regex: keep the compiled pattern until a recompile succeeds

[bryancall]

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.

One caller was asking the wrong question

RegexMatcher::NewEntry() discarded what compile() returned and asked the Regex
whether it held a pattern instead. That was already wrong: with the old implementation a
failed compile left a freed pointer behind, so empty() answered false and the bad
configuration line was accepted. Making compile() transactional replaces one wrong
answer with another, because the object now legitimately holds the previous pattern.

The slot is genuinely reused. When a line's Data::Init() fails, NewEntry resets
regex_strings[num_el] but leaves num_el alone, so the next line lands on the same
Regex, still carrying the pattern the rejected line compiled. A new line whose own
pattern does not compile would then pass the empty() check, get recorded under its own
pattern string, and match against the earlier pattern with this line's configuration.

Ask compile() whether it compiled. Every other caller in the tree that recompiles a live
Regex already does.

Changes

• Regex::compile() compiles into a local and commits to the member only on success; both
  header overloads document the guarantee.
• RegexMatcher::NewEntry() checks compile()'s return value instead of empty().

Testing

[Regex] gains 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.

test_ControlMatcher gains the NewEntry reuse path, which had no coverage at all
(it exercised UrlMatcher only): a line that compiles its regex and then fails to
initialize its record, followed by a line whose own pattern does not compile landing on the
same slot. The case asserts the second line is rejected, that num_el stays at zero, and
that the pattern the first line left behind does not match. Against the old empty() check
all three fail, the last because the stale pattern really does match under the second
line's configuration.

Verification

Fedora 44, gcc 16.2.1, PCRE2 10.47. [Regex] and test_ControlMatcher clean under
AddressSanitizer with UBSan.

---

Siblings from the same split: #13683 (JIT stack and shared match context) and #13685 (JIT
on copies). The interface replacement that retires this type is #13663.

[Copilot]

🔵 Needs a closer look

Update RegexMatcher::NewEntry to handle failed compilation explicitly.

Pull request overview

This pull request makes Regex::compile() transactional, preserving a valid compiled pattern when recompilation fails.

Changes:

• Defers replacing the stored PCRE2 pattern until compilation succeeds.
• Documents failed-recompile behavior.
• Adds regression tests for matching and capture preservation.

Review finding: RegexMatcher::NewEntry must check the compile result explicitly rather than relying on empty().

File summaries

File	Summary
src/tsutil/unit_tests/test_Regex.cc	Adds failed-recompilation regression tests.
src/tsutil/Regex.cc	Preserves the old pattern until successful compilation.
include/tsutil/Regex.h	Documents transactional compile behavior.

Review details

Suppressed comments (1)

src/tsutil/Regex.cc:446

• Preserving _code on compile failure changes the meaning of the existing empty() check in RegexMatcher::NewEntry: that caller ignores the compile() result and only tests empty() (see src/proxy/ControlMatcher.cc:428-432). If a previous entry's Data::Init() fails, num_el is not advanced (ControlMatcher.cc:443-448), so the next entry reuses the same non-empty Regex; an invalid new pattern then leaves the old code in place, is accepted, and can be matched under the new entry's configuration. Update that caller to check the boolean result explicitly rather than using empty() as the compile-status check.

  // Replace the previous pattern only now that the new one exists. Freeing it before
  // pcre2_compile would leave every failure path above returning with a dangling
  // pointer in _code, which empty() reports as a compiled pattern and exec() hands to
  // pcre2_match.

• Files reviewed: 3/3 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[bryancall]

Fixed in 473b8dd. 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.

[Copilot]

🔵 Needs a closer look

Add regression coverage for failed RegexMatcher slot reuse after Data::Init() failure.

Review details

Suppressed comments (1)

src/proxy/ControlMatcher.cc:433

• This return-value check fixes a distinct regression path when a slot is reused after Data::Init() fails, but the current test_ControlMatcher.cc exercises only UrlMatcher and has no RegexMatcher coverage. Please add a regression case that first causes Init() to fail, then submits another invalid regex, and verifies the failed entry is not accepted or matched; otherwise this safety fix can regress unnoticed.

  if (!regex_array[num_el].compile(pattern, error_msg, erroffset)) {

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The safety fix is covered; the remaining requested regression test is a non-blocking nit.

Review details

Suppressed comments (1)

src/proxy/ControlMatcher.cc:433

• This new return-value check fixes a distinct RegexMatcher failure mode, but test_ControlMatcher.cc currently covers only UrlMatcher; it never exercises the sequence where a valid regex is followed by a Data::Init() failure and then another regex at the reused slot. Please add a regression test that verifies the invalid second entry is rejected and is not matched, so this stale-slot behavior cannot regress.

  if (!regex_array[num_el].compile(pattern, error_msg, erroffset)) {

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[JosiahWI]

First pass. The code looks good, but I am not sure there was a bug here, and I need a little time to think about the proposed design change and the impact on other callers.

I think auditing and reorganizing the Regex tests (probably into multiple files) would be beneficial given the amount of work going into this API, but that can go in a follow-up.

[bryancall]

Added in b255d77. 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.

[Copilot]

🟢 Approval recommended

The reviewed changes and regression coverage address the recompilation failure behavior without unresolved blocking issues.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

[JosiahWI]

Thanks!