bryancall opened a new issue, #13660:
URL: https://github.com/apache/trafficserver/issues/13660
## Summary
`RegexMatchContext` builds an empty PCRE2 match context, so any caller that
supplies its own context silently loses everything the shared context
configures.
Today that is a 1 MiB just-in-time (JIT) stack, replaced by PCRE2's fallback
of
32 KiB. `plugins/regex_remap` and `plugins/esi` are both affected.
Nobody chose 32 KiB. It is what PCRE2 uses when no stack is assigned, and it
arrived by omission.
## Where
`src/tsutil/Regex.cc:258` builds a blank context:
```cpp
RegexMatchContext::RegexMatchContext()
{
auto ctx = pcre2_match_context_create(nullptr);
...
}
```
`src/tsutil/Regex.cc:125-130` builds the shared, thread-local one:
```cpp
_match_context = pcre2_match_context_create(_general_context);
_jit_stack = pcre2_jit_stack_create(4096, 1024 * 1024, nullptr); // 1
page min and 1MB max
pcre2_jit_stack_assign(_match_context, nullptr, _jit_stack);
```
`Regex::exec` picks between them at `src/tsutil/Regex.cc:492-497`: a null
context
pointer gets the shared one, a supplied context gets the caller's. Every
regex in
the tree takes the first path except `regex_remap` (`regex_remap.cc:810` and
`:1060`) and `esi` (`IncludeUrlValidator.h:83`).
`Regex::compile` always JIT compiles (`src/tsutil/Regex.cc:446`), so the JIT
stack
is the live limit, not a theoretical one.
## Effect
A pattern that backtracks once per subject character exhausts 32 KiB quickly.
Measured with `pcre2test` from PCRE2 10.47 against the rule and the 3071
byte URL
already in our own test data (`regex_remap.test.py:56` and
`replay/yts-2819.replay.json`):
```
~^/alpha/bravo/[?]((?!action=(newsfeed|calendar|contacts|notepad)).)*$~
Minimum match limit = 9180 interpreter: MATCHES
~...~jitstack=32 Failed: error -46: JIT stack limit reached
~...~jitstack=1024 MATCHES
```
So a URL that should remap instead returns `PCRE2_ERROR_JIT_STACKLIMIT`, and
`regex_remap.cc:1169` logs it and skips the rule. Same pattern, same
subject, two
different answers depending on which context the caller passed.
The threshold is pattern shaped rather than length shaped. The same rule
written
with a single lookahead instead of one per character needs a depth of 4
rather
than 6119, and matches fine either way.
## How it got here
- #5762 (2019) added a recursion limit to `regex_remap` after a production
crash.
PCRE1 recursed on the machine stack, and the estimate at the time was
roughly
500 bytes per character, so a 2000 character URL exhausted a 1 MiB thread
stack.
The limit was 2047, lowered to 1750 in #6819 after it still crashed in
testing.
The surviving comment said `POOMA` and "also dependent on actual stack
size".
- #12575 (2025) converted the plugin to the shared `Regex` class and
introduced
`RegexMatchContext` so it could keep setting a limit. The new constructor
did
not pick up the JIT stack that `RegexContext` had been assigning since
#11014.
- #13652 removed the limit that conversion had mistranslated. The empty
context
remains, so its only present effect is to reduce the JIT stack.
The intent in 2019 was to use most of a 1 MiB thread stack without falling
off
it. 32 KiB is not a descendant of that decision.
## Proposed fix
Three parts, each required by the next.
1. Build `RegexMatchContext` by copying the shared context rather than
creating a
blank one, so a caller-supplied context cannot silently diverge, and
anything
added to the shared context later applies automatically. Callers override
only
what they intend.
2. Have the shared context resolve its JIT stack through a callback rather
than
assigning it directly. PCRE2 requires a distinct stack per thread and a
copied
context can be used on another thread, so the callback returns the calling
thread's stack. This is the pattern `pcre2jit` recommends. Measured cost
is
below noise: 19.9 to 25.4 ns per match assigned directly, 20.4 to 21.7 ns
through the callback, on a short ordinary match.
3. Assert the behavior in `src/tsutil/unit_tests/test_Regex.cc` rather than
through an AuTest. The existing AuTest cannot distinguish what it is
testing:
a recursion limit error, a work limit error, a JIT stack error and no
limit at
all all render as `HTTP/1.1 200 OK` to curl. That is why its assertion
has been
retargeted twice, from -21 in 2019 to -47 in #12575, while the
transaction it
names now emits -46.
This needs no change to `regex_remap.cc`. The plugin keeps its context and
the
context starts correct. It fixes `esi` in the same change.
## Behavior change this causes
`regex_remap`'s effective JIT stack goes from 32 KiB to 1 MiB, and the 3071
byte
URL in `replay/yts-2819.replay.json` starts redirecting rather than falling
through to origin, so that AuTest expectation changes. The crash property
from
#5762 moves to a unit test that asserts it directly.
The 2019 crash cannot recur in this form. PCRE2 has stored backtracking
frames on
the heap since 10.30, and an assigned JIT stack is heap allocated, whereas
the
current 32 KiB default sits on the machine stack. This reduces thread stack
pressure rather than increasing it.
## Out of scope
Whether `regex_remap` should carry a CPU bound at all, and what it should
be, is
a separate question. With PCRE2's default work limit of 10,000,000 I measured
12.4 ms for a single match against the pathological rule already in our
tests,
compared to 0.002 ms under the old 1750. That deserves its own discussion
rather
than being settled inside this change.
The related handling of resource errors as non matches is #13654.
## Open question
`AuTest 1of4` failed on a revision of #13652 that made this same change, and
passed on the seven other open pull requests at the time, so it was not
infrastructure. The author reported both regex_remap AuTests passing
locally. I
have not been able to read that job's output. If the expectation holds on
one host
and not another, the JIT stack threshold varies by platform and this change
needs
to account for that. I will chase it before opening a pull request.
--
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]
