bryancall opened a new pull request, #13683:
URL: https://github.com/apache/trafficserver/pull/13683
Fixes #13660.
First of four, split out of #13671 at review request.
Two of the remaining three, #PR3 and #PR4, 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.
### 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.
- `Regex reports resource exhaustion rather than crashing`: the #5762
pattern against a
256 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 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 #13662 and present on
unmodified
master in the same run. CI's autest lane does not build with ASan.
--
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]
