You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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 RegexMatchContext ran on a 32 KiB JIT stack
RegexMatchContext's constructor called pcre2_match_context_create(nullptr), which
builds 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_remap rule hit the bound at 1,377 bytes of query string. regex_remap and the esi URL 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 a thread_local with a
destructor registers it 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.
A pthread key registers its destructor once, at key creation, and never from the matching
path. pthread_key_create failure is handled: jit_stack_key is zero initialized and key
0 can belong to another subsystem, so a failed create returns null and PCRE2 falls back to
its own stack, which pcre2jit documents as thread safe. A failed pthread_setspecific
frees 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_size lets 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 stack
produced. 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 info
now reports TS_HAS_PCRE2_JIT from pcre2_config(PCRE2_CONFIG_JIT, ...), and the run gates
on it through Condition.HasATSFeature, the same way the QUIC and Brotli runs gate on
theirs. traffic_layout already includes pcre2.h and links PCRE2 through tsutil, so
this needed no build change.
Nothing is lost without a JIT: the match-limit run still reaches -47 through the
interpreter, and the unit test asserts the crash property directly.
Tests
RegexMatchContext matches the shared context: a quantified alternation of capture
groups 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 without
JIT code PCRE2 never consults the stack and the test would pass whether or not the
behaviour is present.
test_tsutil "[Regex]": 386 assertions in 20 cases, clean under AddressSanitizer with
UBSan.
Negative control, this branch's tests against the unfixed Regex.cc: shared_rc := 2, own_rc := -46. The caller context hits JIT_STACKLIMIT exactly
where the shared context matches.
regex_remap autest: every behavioural assertion passes, including the 3 KB URL
returning 301 with its Location and both deliberate resource-limit errors appearing
in diags.log. The run is marked failed only by traffic_server exiting 1 on a
LeakSanitizer 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 unmodified
master in the same run. CI's autest lane does not build with ASan.
…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.
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 RegexMatchContext can 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 uses std::thread in test_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))+$)";
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.
The reason will be displayed to describe this comment to others. Learn more.
🔵 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 one RegexMatchContext across several concurrent matches; otherwise a shared-stack regression could pass the current tests while corrupting simultaneous JIT matches.
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 RegexMatchContext per 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);
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 Regex, half of them through a single RegexMatchContext built before the workers start. That is deliberate and it is the shape your comment asked for: an implementation that cached one stack in the context, or handed the same stack to every thread, passes a test that gives each thread its own context and corrupts this one. 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 lane runs devtoolset-10.
Verified on Fedora 44, gcc 16.2.1, PCRE2 10.47:
[Regex] under ThreadSanitizer: 388 assertions in 21 cases, no warnings. [threads] alone is also clean.
[Regex] under AddressSanitizer with UBSan: same counts, clean.
regex_remap autest executes 14 runs with TS_HAS_PCRE2_JIT 1; diags.log carries both -46 and -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 thread_local, which is the change someone would otherwise make back into a loader-lock inversion. Happy to cut it too if you disagree.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The legacy PCRE2 feature path can report JIT support when executable-memory allocation is unavailable, causing the gated AuTest to fail.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/traffic_layout/info.cc:188
On PCRE2 versions before 10.45, this fallback only reports compile-time JIT support. pcre2_config(PCRE2_CONFIG_JIT) remains nonzero when executable-memory allocation is denied, while Regex::compile() falls back to the interpreter; the gated 64 KiB AuTest will then redirect and fail despite correct behavior. Since CMake accepts any libpcre2-8 version (CMakeLists.txt:284), please make the legacy branch probe a real pattern's JIT result or otherwise mark the feature false when allocation is unavailable.
Assignment overwrote the held pcre2 match context without releasing it,
so every assignment leaked one. Nothing in the tree assigned a
RegexMatchContext until the new unit test did, which is why this only
surfaced now, and only on the Rocky lane: its ci-rocky preset inherits
asan, so LeakSanitizer fails the ctest process while every other lane
passes.
The copy is taken before the old context is released so a failing copy
leaves the object holding its old context rather than a freed one.
Verified in the CI platform's own container on eris: Rocky Linux 8.10,
gcc-toolset-11 (11.2.1), pcre2 10.32-3.el8_6, g++ -fsanitize=address.
Before: 418 assertions pass but LeakSanitizer reports 80 bytes leaked
from pcre2_match_context_copy and the process exits 1. After: same 418
assertions, no leak, exit 0.
… concurrent round
Two review points from apache#13683.
RegexMatchContext::operator= carried a comment promising that a failing
copy leaves the object holding its old context. The code did not do
that. pcre2_match_context_copy() returns nullptr when it cannot
allocate, and the operator assigned that nullptr and freed the old
context anyway, so an OOM emptied the object rather than leaving it
alone. The ordering was already right; the missing piece was declining
to swap when the copy came back null. OOM only, but a comment asserting
a safety property the code does not have is worse than no comment.
The concurrency case had three inlined assertions repeated inside the
thread body. They move to concurrent_match_round(), which returns false
on the first disagreement. The case asserts on zero failures either way,
so counting rounds rather than assertions does not weaken it.
The copy constructor has the same null return from
pcre2_match_context_copy() but no old context to lose, so it is left
alone deliberately.
The new RegexMatchContext constructor copied the shared context, which
meant calling RegexContext::get_instance(). That constructs a
thread_local whose destructor registers through __cxa_thread_atexit and
takes the dynamic loader lock, which is the inversion this change
documents above jit_stack_key and exists to avoid. The old constructor
reached none of it, so the copy introduced the hazard on any thread that
builds a context without matching, including a plugin doing so during
its static initialization while dlopen already holds that lock.
Nothing the shared context carries is needed here. The JIT stack
callback is thread independent because it resolves its stack through the
pthread key, so assigning it directly gives the 1MiB stack without the
thread_local, and a null general context is what this constructor used
before this branch.
This also drops the copy and its null-shared fallback, so the
constructor is shorter than the version it replaces.
TS_HAS_PCRE2_JIT only says that the library has a JIT; it does not guarantee that this 64,000-byte subject exhausts the 1 MiB stack before the parser's 65,535-byte ceiling. The comments above explicitly acknowledge architectures where the exhaustion floor is above that ceiling, but this branch still creates the run and requires a resource-limit diagnostic, so those otherwise supported JIT builds will fail. Please gate this case on a measured safe range or rely on the unit test for this property.
The move constructor and move assignment were defaulted over a bare
void* that the destructor frees, so a move left both objects owning the
same pcre2 match context and the second destruction was a double free.
Nothing in the tree moved one, so it never fired, but this PR exists to
repair that type's ownership and it added copy and copy-assign tests
without a move test. Steal the pointer and null the source.
The destructor no longer asserts the pointer is set. Null is now a
legitimate state, because a moved-from object holds nothing, and the
assert would abort a debug build on the first destruction of one.
Also correct a comment that claimed a mebibyte JIT stack resolves a
longer subject than proxy.config.http.request_header_max_size permits.
Measured with the pattern the unit tests use, a mebibyte carries about
26,213 characters and that record defaults to 32768, so a long enough
subject still falls back and can hit a resource-exhaustion code.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Runtime JIT allocation detection and the inaccurate JIT-stack comment must be corrected.
Review effort: Lite Findings: None
Previously missed (1)
In code that hasn't changed since last review
Verify runtime PCRE2 JIT allocation on pre-10.45 fallback
src/traffic_layout/info.cc:189
On PCRE2 versions before 10.45 this fallback only reports that JIT support was compiled in; it does not verify that executable memory can actually be allocated. A JIT-enabled library running under a hardened/W^X policy can therefore publish TS_HAS_PCRE2_JIT=1, causing the gated 64 KB AuTest to run through the interpreter and expect a resource-limit diagnostic even though the rule redirects. Please probe a compiled pattern's JIT size (or otherwise verify runtime JIT allocation) on this fallback path.
std::exchange on the raw _ptr member yields void*, and _MatchContext::set
takes pcre2_match_context*, which void* does not implicitly convert to.
Go through the typed accessors instead, which also stops the move
operations reaching into the raw member at all.
Verified by building and by removing the fix again: with the move
operations defaulted the new move case segfaults on the second
destruction, which is the double free it exists to catch.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
JIT capability detection must handle runtime allocation failures, with the noted documentation and comment updates also outstanding.
Review effort: Lite Findings: None
Previously missed (1)
In code that hasn't changed since last review
Detect runtime JIT allocation failure in fallback feature probe
src/traffic_layout/info.cc:189
On PCRE2 versions before 10.45 (which this tree still supports), this fallback only reports the library's compile-time JIT capability. It does not detect a hardened runtime where executable-memory allocation makes pcre2_jit_compile() fail; Regex::compile() ignores that failure, so TS_HAS_PCRE2_JIT remains true while the gated 64 KiB AuTest runs in the interpreter and expects the wrong result. Probe an actual JIT compilation, or otherwise make this feature false when JIT allocation is unavailable, on the fallback path.
Drop the destructor comment that described the code before moves
existed, and correct the JIT stack comment: a subject past the stack
limit returns PCRE2_ERROR_JIT_STACKLIMIT, with no fallback. The move
tests no longer skip when JIT is unavailable, and the traffic_layout
JIT probe compiles a pattern instead of trusting PCRE2_CONFIG_JIT.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
The low-level threading and regex-stack changes warrant final human review.
Review effort: Lite Findings: None
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
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.
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.