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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
