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/12 19:27:23 UTC
[PR] Regex: fix four defects in the tsutil regex wrapper [trafficserver]
bryancall opened a new pull request, #13671:
URL: https://github.com/apache/trafficserver/pull/13671
Four defects in the `tsutil` regex wrapper, all present in shipped releases. Every commit
is a behaviour fix with no change to the public API surface, so the series backports as a
unit.
This supersedes #13661, whose two commits are the first of the four here. That PR bundled
these fixes with a source-breaking change (deleting `RegexMatchContext`'s copy and move
members), which would have kept the whole thing off the release branches. The member
deletion is deferred to #13663, where the type is replaced outright.
### The fixes
**1. A caller-supplied `RegexMatchContext` ran on a 32 KiB JIT stack** (#13660).
`pcre2_match_context_create(nullptr)` builds a context that configures nothing, so a
caller who wanted only a match limit silently gave up everything the shared context
provides, including its 1 MiB JIT stack. PCRE2 fell back to its own 32 KiB machine-stack
block, which resolves about 1,362 bytes of a subject that backtracks once per character; a
production regex_remap rule hit that bound at 1,377 bytes of query string. Copy the shared
context instead.
A shared context needs a per thread JIT stack, and a `thread_local` holding one registers
its destructor through `__cxa_thread_atexit`, which takes the dynamic loader lock. Doing
that from a match inverts lock order against a `dlopen` caller running a plugin's static
initialization; `Diags::tag_activated` documents that exact deadlock. The stack now comes
from a callback backed by a pthread key, whose destructor is registered once at key
creation. `pthread_key_create` failure is handled, because `jit_stack_key` is zero
initialized and key 0 can belong to another subsystem.
**2. The pcre2 contexts are now process-wide.** The general, compile and match contexts
were themselves in a `thread_local` with a destructor, so the first `compile()` or `exec()`
on a thread still took the loader lock. Nothing in them is per thread: they are built once
and never modified, which is `pcre2api`'s stated condition for sharing a context across
threads. One instance, never destroyed, against a destructor that can run while another
thread is still matching.
**3. A failed recompile left freed memory in the object.** `compile()` freed the pattern it
already held *before* calling `pcre2_compile()`. Every failure path after that 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 again. It now
compiles into a local and replaces the member only on success, so a failed compile leaves
the previous pattern usable.
**4. A copied `Regex` silently changed engine.** `pcre2_code_copy()` duplicates a compiled
pattern but not the machine code the JIT produced for it. The copy constructor called
nothing else, so every copied `Regex` matched on the interpreter: same answers, far slower,
and a different set of resource limits, so a pattern that reports a JIT stack limit through
the original quietly matched through a copy. `plugins/experimental/maxmind_acl` copies
every rule. The copy is now compiled for the JIT, which costs what a compile costs
(measured 7.5 ns to 2549 ns, 1 allocation to 6); nothing copies a `Regex` on a request path.
### Tests
Each test fails against the code it fixes. Built against the unfixed implementation, the
recompile test segmentation faults and the copy test returns a match where the original
returns the stack-limit error.
New: two recompile sections, four copy sections, and an eight-thread concurrency test on a
single instance, half the threads through their own match context. The header has always
promised `exec()` is safe to call concurrently and nothing tested it.
### Verification
Fedora 44, gcc 16.2.1, PCRE2 10.47:
- Full build with experimental plugins, `ctest` **1058/1058**.
- `[Regex]` clean under **ThreadSanitizer** and under **AddressSanitizer + UBSan** (411 assertions, 22 cases).
- `regex_remap` autest passes.
- `RegexContext::~RegexContext()` and the thread-local `ctx` are both gone from the object
file; what remains is a plain static behind a guard variable. The object still references
`__cxa_thread_atexit`, but that belongs to the `inline thread_local` in
`tsutil/ts_bw_format.h` and is present before and after.
- No measurable change on any match or compile path (medians of three interleaved rounds,
every delta inside a 5% run-to-run band). The harness is #13670.
--
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: fix four defects in the tsutil regex wrapper [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall closed pull request #13671: Regex: fix four defects in the tsutil regex wrapper
URL: https://github.com/apache/trafficserver/pull/13671
--
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: fix four defects in the tsutil regex wrapper [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #13671:
URL: https://github.com/apache/trafficserver/pull/13671#issuecomment-5674857797
Split into one PR per defect, as requested in review. This PR is superseded by:
- #13683: a caller-supplied `RegexMatchContext` ran on a 32 KiB JIT stack, and the JIT stack moves to a pthread key. Fixes #13660.
- #13684: a failed recompile left freed memory in the object.
- #13685: a copied `Regex` silently changed engine.
The fourth defect, making the pcre2 contexts process-wide, is held rather than opened
now. It is only safe once the JIT stack is resolved per thread through a callback, which
is #13683, so opening it today would mean a PR carrying #13683's commit along for the
ride, which is the bundling this split is meant to undo. It goes up as its own PR once
#13683 merges. The branch is ready on my fork as `regex-process-wide-contexts`.
Applying all four to master reproduces this PR's tree byte for byte in every file it
touched, so nothing was dropped in the split.
--
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: fix four defects in the tsutil regex wrapper [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13671:
URL: https://github.com/apache/trafficserver/pull/13671#discussion_r3997452248
##########
src/tsutil/Regex.cc:
##########
@@ -330,6 +390,14 @@ Regex::Regex(Regex const &other)
if (other_code != nullptr) {
// Use PCRE2's built-in function to deep copy the compiled pattern
auto *copied_code = pcre2_code_copy(other_code);
+
+ // pcre2_code_copy() does not carry the machine code the JIT produced, because that
+ // code is position dependent. Without this the copy would match on the interpreter:
+ // same answers, much slower, and a different set of resource limits, so a pattern
+ // that reports a JIT stack limit through the original would quietly match through
+ // the copy. Compile it again, exactly as Regex::compile() does for a new pattern.
+ pcre2_jit_compile(copied_code, PCRE2_JIT_COMPLETE);
+
_Code::set(_code, copied_code);
Review Comment:
`pcre2_code_copy()` can return `nullptr` on allocation failure. This unconditional call passes that null pointer to `pcre2_jit_compile`, turning a recoverable copy failure into a crash; guard the JIT step and only publish the copied code when the copy succeeded.
--
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: fix four defects in the tsutil regex wrapper [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall closed pull request #13671: Regex: fix four defects in the tsutil regex wrapper
URL: https://github.com/apache/trafficserver/pull/13671
--
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: fix four defects in the tsutil regex wrapper [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13671:
URL: https://github.com/apache/trafficserver/pull/13671#discussion_r3997489352
##########
src/tsutil/Regex.cc:
##########
@@ -330,6 +390,14 @@ Regex::Regex(Regex const &other)
if (other_code != nullptr) {
// Use PCRE2's built-in function to deep copy the compiled pattern
auto *copied_code = pcre2_code_copy(other_code);
+
+ // pcre2_code_copy() does not carry the machine code the JIT produced, because that
+ // code is position dependent. Without this the copy would match on the interpreter:
+ // same answers, much slower, and a different set of resource limits, so a pattern
+ // that reports a JIT stack limit through the original would quietly match through
+ // the copy. Compile it again, exactly as Regex::compile() does for a new pattern.
+ pcre2_jit_compile(copied_code, PCRE2_JIT_COMPLETE);
+
_Code::set(_code, copied_code);
Review Comment:
Fixed in 4355035f6d. The man page agrees: pcre2_code_copy() returns NULL if it cannot obtain memory. The copy constructor now checks it and leaves the object empty when the copy fails, which is the state a default constructed Regex is in and the one empty() reports, rather than holding a null pattern or handing it to pcre2_jit_compile().
_🤖 Addressed by [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