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.
There was a problem hiding this comment.
🟡 Changes recommended
Multithreaded coverage and JIT-capability gating remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes regex matching by preserving the shared PCRE2 context and providing per-thread JIT stacks.
Changes:
- Adds pthread-key-backed JIT stack handling.
- Copies shared settings into caller-supplied contexts.
- Updates unit and
regex_remapregression tests.
File summaries
| File | Description |
|---|---|
tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py |
Updates long-query expectations and crash-guard coverage. |
src/tsutil/unit_tests/test_Regex.cc |
Adds JIT inheritance and resource-exhaustion tests. |
src/tsutil/Regex.cc |
Implements shared-context copying and per-thread JIT stack handling. |
Review details
Suppressed comments (1)
src/tsutil/unit_tests/test_Regex.cc:1088
- The key correctness property of this change is that one copied
RegexMatchContextcan be used concurrently without sharing a JIT stack, but both new tests execute matches on a single thread. A regression that returned one stack for every thread would therefore still pass; please add a multithreaded test that shares a context and drives JIT matching (the repository already usesstd::threadintest_thread_safety.cc).
TEST_CASE("RegexMatchContext matches the shared context", "[libts][Regex][RegexMatchContext]")
{
// Quantified alternation of capture groups: every subject character pushes a
// backtracking frame, so the JIT stack size is what bounds this.
char const *const pattern = R"(^(?:(a)|(b))+$)";
- Files reviewed: 3/3 changed files
- Comments generated: 1
- 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 crash-guard run sends a 64 KB query and asserts two things: that the rule does not redirect, and that regex_remap logs a resource-limit error for it. Both hold only when PCRE2 can run the pattern on the just-in-time engine, because that is the only engine with a stack to exhaust here. The interpreter keeps its backtracking frames on the heap, so on a build without JIT the subject simply matches, the rule redirects, and the run fails for a reason that has nothing to do with what it tests. The unit tests added alongside it already skip themselves on the same condition; this run did not, so a PCRE2 built without JIT would fail the suite. Report whether PCRE2 has a JIT as TS_HAS_PCRE2_JIT from traffic_layout, which already includes pcre2.h and links it through tsutil, and gate the run on it the way the QUIC and Brotli runs gate on their features. Nothing else in the file depends on the JIT: the 3 KB redirect run gets the same answer from either engine, and the crash property itself is still covered without a JIT by the match-limit run and by the unit test that asserts it directly.
The two tests added with the pthread key both match on a single thread, so an implementation that handed the same JIT stack to every thread would pass them. That is the one regression this change exists to prevent, and nothing covered it. Eight threads match on one Regex, half of them through a single caller-supplied context built before the workers start. That is the production shape: regex_remap builds a context when it loads a rule and every net thread then matches through it, so a context that cached a stack rather than resolving one per thread through the callback would pass a test that gave each thread its own context and corrupt this one. Every thread must reach the same verdict, and under ThreadSanitizer the run must also be clean. The start gate is a mutex and condition variable rather than std::latch, which says it more directly but is not in libstdc++ before 11, and the CentOS build runs devtoolset-10. 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 overlaps instead of running one thread at a time.
There was a problem hiding this comment.
🔵 Needs a closer look
Add concurrent-match regression coverage for a shared RegexMatchContext before approval.
Review details
Suppressed comments (2)
src/tsutil/Regex.cc:188
- This callback is the thread-safety guarantee for a single copied
RegexMatchContext, but the new test only runs the shared and caller-supplied paths serially on one thread. Please add a regression test that shares oneRegexMatchContextacross several concurrent matches; otherwise a shared-stack regression could pass the current tests while corrupting simultaneous JIT matches.
pcre2_jit_stack_assign(_match_context, jit_stack_for_this_thread, nullptr);
src/tsutil/unit_tests/test_Regex.cc:1103
- This test exercises the caller-supplied context only on the thread that constructed it. The new callback/key design is specifically required because regex_remap keeps one
RegexMatchContextper remap instance and uses it from multiple ET_NET threads; an implementation that accidentally shared one thread's stack pointer would still pass this test but corrupt concurrent matches. Please add a multithreaded regression that constructs one context and runs matches concurrently from other threads.
RegexMatchContext match_context;
RegexMatches own_matches;
int const shared_rc = re.exec(subject, shared_matches);
int const own_rc = re.exec(subject, own_matches, 0, &match_context);
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Both findings from the Copilot review are addressed. The JIT gating one had a thread and is answered and resolved there; this covers the suppressed one, which had no thread to reply in. Multithreaded coverage. You were right, and the gap was mine: I had written exactly this test and then filed it with the wrong change. It sat on the follow-up branch that makes the pcre2 contexts process-wide, because that is where it first landed. The property it guards is introduced here, by resolving the JIT stack per thread through a callback, so here is where it belongs. Moved in c4cdac1. Eight threads match on one Verified on Fedora 44, gcc 16.2.1, PCRE2 10.47:
|
Eight lines of measurements above one call. Keep the two facts a reader needs to judge the number, that the maximum is reserved rather than committed and that a mebibyte already outruns what a client can send, and leave the measured table in the pull request. The rationale for holding the stack in a pthread key stays where it is, because that is the part someone would otherwise simplify back into a deadlock.
|
@JosiahWI you flagged comment verbosity on #13684 and #13685, so I applied the same pass here before you had to say it a third time: the eight lines of stack-size measurements above one call are down to the two facts needed to judge the number, and the table is in the description instead. I deliberately left the comment on the pthread key at full length. It is the one that explains why the stack is not in a |
|
@bryancall I want to clarify one thing just to make sure we are on the same page. The AuTest that was expecting a 200, and has been modified to expect a redirect - was that test enforcing a bug, or are we changing the documented behavior in this patch? |
JosiahWI
left a comment
There was a problem hiding this comment.
First of at least three review passes, focusing on the new test code. The second review will focus on the design and correctness of the concurrent unit test. The motivation for the chance is good and the tests are appropriate enough.
| if (code == nullptr) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This error handling is a risk for future bugs. Returning false from pattern_has_jit, due to an error while compiling the pattern, communicates the wrong idea to the programmer calling the function. I think it might be best to throw an exception here, but returning a stronger error-carrying type or doing a release assert are alternatives.
| if (code == nullptr) { | ||
| return false; | ||
| } | ||
| pcre2_jit_compile(code, PCRE2_JIT_COMPLETE); |
There was a problem hiding this comment.
The PCRE2 library provides a mechanism specifically for a similar kind of test, which simplifies this function implementation considerably. One caveat: the API I'm going to mention is used to test general JIT support, but the wording of your comment suggests JIT can be supported for some patterns and not others. In that case, this API would not be applicable. Do you have a reference for that?
The availability of JIT support can be tested by calling pcre2_compile_jit() with a single option PCRE2_JIT_TEST_ALLOC (the code argument is ignored, so a NULL value is accepted). Such a call returns zero if JIT is available and has a working allocator. Otherwise it returns PCRE2_ERROR_NOMEMORY if JIT is available but cannot allocate executable memory, or PCRE2_ERROR_JIT_UNSUPPORTED if JIT support is not compiled.
| // The guard from #5762: a pattern that backtracks once per character must fail | ||
| // cleanly rather than run the thread out of stack. PCRE1 recursed on the machine | ||
| // stack and a long enough subject crashed the server; PCRE2 must report an error | ||
| // instead. If this ever crashes rather than fails, that regression is back. |
There was a problem hiding this comment.
Remove the last sentence, please. I think the programmer can discern easily enough that a failing test could mean a regression after reading the first part of the comment.
| // stack from the callback rather than share one. Run this under ThreadSanitizer to get the | ||
| // second half of the guarantee. | ||
| TEST_CASE("Regex matches concurrently on one instance", "[libts][Regex][threads]") | ||
| { |
There was a problem hiding this comment.
I think some setup or other helpers could usefully be extracted from the concurrent test.
Fixes #13660.
First of four, split out of #13671 at review request.
Two of the remaining three, #13684 and #13685, are independent of this one and of each
other. The fourth, making the pcre2 contexts process-wide, builds directly on this
change and cannot stand without it, so it follows once this merges rather than
carrying this commit along for the ride.
A caller-supplied
RegexMatchContextran on a 32 KiB JIT stackRegexMatchContext's constructor calledpcre2_match_context_create(nullptr), whichbuilds a context that configures nothing. A caller who wanted only to set a match limit
therefore silently gave up everything the shared context provides, including its 1 MiB
JIT stack, and PCRE2 fell back to its own 32 KiB machine-stack block. That block resolves
about 1,362 bytes of a subject that backtracks once per character; a production
regex_remaprule hit the bound at 1,377 bytes of query string.regex_remapand theesiURL validator are the only two callers, and both were affected.Copy the shared context instead, so a caller overrides only what it means to override
and anything added to the shared context later propagates on its own.
The JIT stack moves to a pthread key
A shared match context needs a per thread JIT stack, and PCRE2 supplies one through a
callback invoked at match time rather than a pointer baked in when the context is built.
The obvious place to keep that stack is a
thread_local, but athread_localwith adestructor registers it through
__cxa_thread_atexit, which takes the dynamic loaderlock. Doing that from a match inverts lock order against a
dlopencaller running aplugin's static initialization;
Diags::tag_activateddocuments that exact deadlock.A pthread key registers its destructor once, at key creation, and never from the matching
path.
pthread_key_createfailure is handled:jit_stack_keyis zero initialized and key0 can belong to another subsystem, so a failed create returns null and PCRE2 falls back to
its own stack, which
pcre2jitdocuments as thread safe. A failedpthread_setspecificfrees the stack rather than leaking one per match.
The 1 MiB maximum is now recorded as measured rather than assumed: the maximum costs
nothing per match at any size, and 1 MiB already resolves a longer subject than
proxy.config.http.request_header_max_sizelets a client send.The autest expectation changes
regex_remap's 3 KB URL case asserted a 200 fall-through, which is what a 32 KiB stackproduced. With the rule matching as written it is a 301, so the run now expects the
redirect and its
Location. The crash property that case was really guarding, from#5762, moves to a unit test that asserts it directly rather than inferring it from a
status code.
The crash-guard run is gated on a JIT
Only the JIT engine has a stack to exhaust for this pattern; PCRE2's interpreter keeps its
backtracking frames on the heap and simply matches. A PCRE2 built without JIT would
therefore have failed that run on behaviour that is entirely correct.
traffic_layout infonow reports
TS_HAS_PCRE2_JITfrompcre2_config(PCRE2_CONFIG_JIT, ...), and the run gateson it through
Condition.HasATSFeature, the same way the QUIC and Brotli runs gate ontheirs.
traffic_layoutalready includespcre2.hand links PCRE2 throughtsutil, sothis needed no build change.
Nothing is lost without a JIT: the match-limit run still reaches
-47through theinterpreter, and the unit test asserts the crash property directly.
Tests
RegexMatchContext matches the shared context: a quantified alternation of capturegroups over 1,000 characters, run through the shared context and through a caller
context, must return the same result. Gated on
PCRE2_INFO_JITSIZE, because withoutJIT code PCRE2 never consults the stack and the test would pass whether or not the
behaviour is present.
Regex reports resource exhaustion rather than crashing: the Limit resources used by regex_remap to prevent crashes on stack overflow #5762 pattern against a256 KiB subject must return an error, not take down the thread.
Verification
Fedora 44, gcc 16.2.1, PCRE2 10.47, dev-asan.
test_tsutil "[Regex]": 386 assertions in 20 cases, clean under AddressSanitizer withUBSan.
Regex.cc:shared_rc := 2,own_rc := -46. The caller context hitsJIT_STACKLIMITexactlywhere the shared context matches.
regex_remapautest: every behavioural assertion passes, including the 3 KB URLreturning
301with itsLocationand both deliberate resource-limit errors appearingin
diags.log. The run is marked failed only bytraffic_serverexiting 1 on aLeakSanitizer report, and that leak is 104 bytes in
ConfigReloadTask::start_progress_checker, which is regex_remap AuTest is flaky: LeakSanitizer leak in ConfigReloadTask::start_progress_checker #13662 and present on unmodifiedmaster in the same run. CI's autest lane does not build with ASan.