Conversation
pcre2_code_copy() copies a compiled pattern but not the machine code the JIT produced for it, because that code is position dependent. The copy constructor called nothing else, so every copied Regex matched on the interpreter: the same answers, far slower, and under a different set of resource limits. A pattern that reports a JIT stack limit through the original quietly matched through a copy, which is how the two disagree about whether a subject is acceptable at all. plugins/experimental/maxmind_acl copies every rule. Compile the copy for the JIT after copying it, exactly as compile() does for a new pattern, and describe that in the header, which called it a deep copy. pcre2_code_copy() also returns null when it cannot obtain memory, and the copy constructor passed that straight to pcre2_jit_compile(). Check it, and leave the object empty when the copy fails: that is the state a default constructed Regex is in and the state empty() reports, rather than a Regex holding a null pattern. The pcre2_jit_compile() result is deliberately not checked. A pattern the JIT declines still matches correctly on the interpreter, this class has no way to tell a caller which engine it got, and "no JIT" is not a single error code across PCRE2 versions and build options. compile() has the same property. Reporting the engine belongs to the replacement API rather than here. The test asserts the property that matters: a copy answers the same as its original on a subject sized past the JIT stack bound. Before this change the original returned the stack limit error and the copy returned a match. It needs no knowledge of whether the build has a JIT, because without one both sides simply agree.
There was a problem hiding this comment.
🟢 Approval recommended
The changes preserve JIT behavior, handle allocation failure safely, and include regression coverage.
Pull request overview
Updates Regex deep copies to preserve PCRE2 JIT behavior and safely handle failed pattern copies.
Changes:
- Recompiles copied patterns for JIT.
- Leaves failed copies empty.
- Adds coverage for copy construction, assignment, chained copies, and matching parity.
File summaries
| File | Description |
|---|---|
src/tsutil/unit_tests/test_Regex.cc |
Tests equivalent behavior across copy paths. |
src/tsutil/Regex.cc |
Re-JITs copied patterns and handles null copies. |
include/tsutil/Regex.h |
Documents JIT compilation during copying. |
Review details
- 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.
|
Copilot reviewed this one as approval recommended with no findings, so there is nothing to fix here. Noting for the record that the earlier red CI on this PR was not a build failure. Every failing check went from pending to failure in 60 to 75 seconds, which is not long enough for any ATS lane to compile, and the one check that was retried (Fedora, at 05:01Z) took 16 minutes and passed. Three PRs opened within three minutes of each other, and the jobs were aborted rather than run. Retriggering below. |
|
[approve ci centos clang-analyzer debian osx rocky ubuntu autest 0 autest 1 autest 2 autest 3] |
|
[approve ci autest 1 autest 2 autest 3] |
|
[approve ci autest 2] |
|
[approve ci autest 3] |
JosiahWI
left a comment
There was a problem hiding this comment.
I think the comment verbosity in the source code is a little over the top, but the change looks good overall. Please clean up the comments or give a justification for the duplicated information.
The Regex unit tests look like they need to be refactored, and most likely split into multiple files, but that can be a follow-up.
|
[approve ci centos clang-analyzer debian osx rocky ubuntu autest] |
Fifteen lines of comment above three lines of code, most of it repeating the paragraph already in the test that covers this. Keep what is not evident from the code, which is that a null copy means pcre2 ran out of memory, that the JIT compile is what stops the copy running on a different engine than the original, and that its result is deliberately unchecked. The rest is in the commit that made the change and in the test.
|
Both points addressed, and thanks for the read. Comment verbosity. Cut in 8c76dfb, replied in the thread. Same treatment I gave the comment you flagged on #13684. You were right both times: I had put the reasoning in the source where it duplicated the test, instead of in the commit message where it belongs. Unit test structure. Agreed, and taking it as the follow-up you suggested rather than growing this PR. CI was 14/14 on the previous head. The only change since is comments, and |
Fourth 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 copied
Regexsilently changed enginepcre2_code_copy()duplicates a compiled pattern but not the machine code the JITproduced for it, because that code is position dependent. The copy constructor called
nothing else, so every copied
Regexmatched on the interpreter: the same answers, farslower, and under a different set of resource limits. A pattern that reports a JIT stack
limit through the original quietly matched through a copy, which is how the two can
disagree about whether a subject is acceptable at all.
plugins/experimental/maxmind_aclcopies every rule.Compile the copy for the JIT after copying it, exactly as
compile()does for a newpattern, and describe that in the header, which called it a deep copy. Nothing copies a
Regexon a request path; the cost is what a compile costs.pcre2_code_copy()also returns null when it cannot obtain memory, and the copyconstructor passed that straight to
pcre2_jit_compile(). It is checked now, and a failedcopy leaves the object empty: the state a default constructed
Regexis in and the oneempty()reports, rather than aRegexholding a null pattern.The
pcre2_jit_compile()result is deliberately not checked, for the same reasoncompile()does not check it. A pattern the JIT declines still matches correctly on theinterpreter, this class has no way to tell a caller which engine it got, and "no JIT" is
not a single error code across PCRE2 versions and build options. Reporting the engine
belongs to the replacement API in #13663.
Tests
Four sections asserting the property that matters: a copy answers the same as its
original on a subject sized past the JIT stack bound, through the copy constructor, copy
assignment, a copy of a copy, and on an ordinary subject that simply matches.
Built against the unfixed implementation the original returns the stack limit error and
the copy returns a match. The test needs no knowledge of whether the build has a JIT,
because without one both sides agree.
Verification
Fedora 44, gcc 16.2.1, PCRE2 10.47.
[Regex]clean under AddressSanitizer with UBSan.Siblings from the same split: #13683 (JIT stack and shared match context) and #13684 (failed recompile). The interface replacement that retires this type is #13663.