Regex: fix four defects in the tsutil regex wrapper - #13671
Open
bryancall wants to merge 7 commits into
Open
Conversation
…ch context Two problems with the same root: a caller-supplied RegexMatchContext was built blank, and the JIT stack was held in a thread_local. pcre2_match_context_create(nullptr) produces a context that configures nothing, so a caller who wanted only to set a match limit silently gave up everything the shared context provides, including its 1 MiB JIT stack. PCRE2 then 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, so a caller overrides only what it means to override. 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. Take the stack from a callback backed by a pthread key instead, whose destructor is registered once at key creation and never from the matching path. pthread_key_create can fail, and jit_stack_key is zero initialized, so key 0 could belong to another subsystem and hand its value to PCRE2 as a JIT stack. Record whether the key was created and return null when it was not, which pcre2jit documents as falling back to its own stack. If pthread_setspecific fails, free the stack rather than leaking one per match. Record why the maximum is one mebibyte, measured rather than assumed: the maximum costs nothing per match at any size, and one mebibyte already resolves a longer subject than request_header_max_size lets a client send.
Moving the JIT stack into a pthread key removed half of the thread_local hazard. The other half remained: the general, compile and match contexts lived in a thread_local with a destructor, and initializing one registers that destructor through __cxa_thread_atexit, which takes the dynamic loader lock. The first compile() or exec() on a thread therefore still inverted lock order against a dlopen caller running a plugin's static initialization. Nothing in those contexts is per thread. They are built once and never modified, which pcre2api's MULTITHREADING section gives as the condition for sharing a context between threads, and the one per thread object is resolved through the callback. Allocate one instance and never destroy it: three small blocks that live as long as the process, against a destructor that can run while another thread is still matching. That also retires the "should only be null when shutting down" check, which a function-local thread_local could never satisfy. In the object file, RegexContext::~RegexContext() and the thread-local ctx are both gone; what is left is a plain static pointer behind a guard variable, which registers nothing at thread exit. The object still references __cxa_thread_atexit, but that belongs to the inline thread_local in tsutil/ts_bw_format.h and is there before and after this change. The new test runs eight threads matching on one instance, half of them through their own match context, and checks every thread reaches the same verdict. The header promised this and nothing tested it. It is clean under ThreadSanitizer.
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. Two tests cover it: a valid compile followed by a failing one must leave the first pattern matching, including its capture groups. Before this change the first of those segmentation faults.
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. 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. 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.
This was referenced Sep 12, 2026
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The concurrency regression test does not yet exercise a shared RegexMatchContext across threads.
Pull request overview
Fixes four shipped defects in the tsutil::Regex PCRE2 wrapper.
Changes:
- Adds process-wide contexts with per-thread JIT stacks.
- Makes recompilation transactional and copied regexes JIT-aware.
- Expands regression, concurrency, and integration coverage.
Review note: Moderate — the concurrency test should share one RegexMatchContext among own_context workers to exercise the production callback path. (1 vote.)
File summaries
| File | Description |
|---|---|
tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py |
Expands long-query integration coverage. |
src/tsutil/unit_tests/test_Regex.cc |
Adds regression and concurrency tests. |
src/tsutil/Regex.cc |
Implements context, JIT-stack, recompilation, and copy fixes. |
include/tsutil/Regex.h |
Documents corrected behavior. |
Review details
Suppressed comments (1)
src/tsutil/unit_tests/test_Regex.cc:1207
- These contexts are constructed inside each worker, so no
RegexMatchContextis ever passed concurrently by multiple threads. That misses the production case (plugins/regex_remap/regex_remap.cc:810) and would still pass if a copied context held one direct JIT stack; share one context among theown_contextthreads so this test exercises the callback's per-thread stack guarantee.
RegexMatchContext context;
RegexMatchContext const *const use = own_context ? &context : nullptr;
- Files reviewed: 4/4 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.
The CentOS build runs devtoolset-10 and the Ubuntu build a clang of similar vintage, and libstdc++ did not ship <latch> until 11, so both failed to compile the new test with "latch: No such file or directory". Nothing else in the change reaches past C++17 in the library. Use a mutex and condition variable for the start gate instead. It keeps the property the latch was there for, that every thread is inside the match loop before any of them gets far, so the matching actually overlaps rather than running one thread at a time.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A critical copy-failure handling issue remains unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/tsutil/unit_tests/test_Regex.cc:1215
- This test constructs each
RegexMatchContextinside the worker that uses it, so it never exercises a caller-supplied context created on one thread and shared by other threads. That is the production shape (regex_remap.cc:810/1060); an implementation that cached a direct per-thread stack in the context would pass this test even though the plugin would be unsafe. Construct one context before launching the workers and pass that same pointer to the caller-context half to verify the callback and cross-thread stack isolation.
bool const own_context = (i % 2) == 0;
RegexMatchContext context;
RegexMatchContext const *const use = own_context ? &context : nullptr;
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
…est threads pcre2_code_copy() 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 concurrency test built a RegexMatchContext inside each worker, so it never covered the shape the plugins actually use, where one context is created at configuration load and every net thread matches through it. An implementation that cached a JIT stack in the context rather than resolving one per thread through the callback would have passed the old test and corrupted the real thing. Build one context before the workers start and hand the same pointer to every thread that uses one.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The copied regex’s JIT compilation result must be checked to prevent silent interpreter fallback.
Review details
Suppressed comments (2)
src/tsutil/Regex.cc:403
pcre2_code_copy()never carries the original JIT code, so this call is required to preserve the original engine. However, its return value is ignored: if JIT compilation fails (for example because executable-memory allocation fails), the copied code is still published and silently falls back to the interpreter, recreating the behavior this change is meant to eliminate. Check the result and handle the failure based on whether the source had JIT code before assigning_code.
pcre2_jit_compile(copied_code, PCRE2_JIT_COMPLETE);
src/tsutil/unit_tests/test_Regex.cc:1219
own_contextis true when the thread uses the one sharedshared_caller_context, and false when it uses the default context, so the predicate name describes the opposite of the branch it controls. Rename it to reflect that it selects the caller-supplied context; this test is specifically meant to document the shared-context case.
bool const own_context = (i % 2) == 0;
RegexMatchContext const *const use = own_context ? &shared_caller_context : nullptr;
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
own_context was true for the threads matching through the one shared, caller-supplied context, which is the opposite of what the name says, and the shared case is the whole point of the test. Call it use_caller_context. Also record why the pcre2_jit_compile() result on a copy is 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.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The changes affect shared context lifetimes, thread concurrency, and regex memory management, warranting final human review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four defects in the
tsutilregex wrapper, all present in shipped releases. Every commitis 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 movemembers), 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
RegexMatchContextran on a 32 KiB JIT stack (#13660).pcre2_match_context_create(nullptr)builds a context that configures nothing, so acaller 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_localholding one registersits destructor through
__cxa_thread_atexit, which takes the dynamic loader lock. Doingthat from a match inverts lock order against a
dlopencaller running a plugin's staticinitialization;
Diags::tag_activateddocuments that exact deadlock. The stack now comesfrom a callback backed by a pthread key, whose destructor is registered once at key
creation.
pthread_key_createfailure is handled, becausejit_stack_keyis zeroinitialized 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_localwith a destructor, so the firstcompile()orexec()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 acrossthreads. 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 italready held before calling
pcre2_compile(). Every failure path after that returnedwith 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 nowcompiles into a local and replaces the member only on success, so a failed compile leaves
the previous pattern usable.
4. A copied
Regexsilently changed engine.pcre2_code_copy()duplicates a compiledpattern but not the machine code the JIT produced for it. The copy constructor called
nothing else, so every copied
Regexmatched 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_aclcopiesevery 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
Regexon 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:
ctest1058/1058.[Regex]clean under ThreadSanitizer and under AddressSanitizer + UBSan (411 assertions, 22 cases).regex_remapautest passes.RegexContext::~RegexContext()and the thread-localctxare both gone from the objectfile; what remains is a plain static behind a guard variable. The object still references
__cxa_thread_atexit, but that belongs to theinline thread_localintsutil/ts_bw_format.hand is present before and after.every delta inside a 5% run-to-run band). The harness is benchmark: measure Regex time and heap allocations #13670.