bneradt opened a new issue, #13651:
URL: https://github.com/apache/trafficserver/issues/13651

   The PCRE2 conversion in #12575 (`891d348dff`) changed regex_remap's existing 
recursion-depth limit into a matching-work limit. As a result, a simple 
matching URL of about 2 KB can fail with `PCRE2_ERROR_MATCHLIMIT` (-47), 
causing the plugin to skip a rule that should match.
   
   This remains present in master at `390bafd7683d14de7faee45129a122557456c092`.
   
   ### Cause
   
   Before #12575, the plugin used:
   
   ```cpp
   _extra->match_limit_recursion = 1750;
   _extra->flags |= PCRE_EXTRA_MATCH_LIMIT_RECURSION;
   ```
   
   After the conversion, it uses:
   
   ```cpp
   static const int32_t REGEX_MATCH_LIMIT = 1750;
   // ...
   ri->match_context.set_match_limit(REGEX_MATCH_LIMIT);
   ```
   
   `RegexMatchContext::set_match_limit()` calls `pcre2_set_match_limit()`. This 
limits matching work/backtracking, not recursion depth. The value of 1,750 was 
originally chosen to protect the old matcher from stack exhaustion; it is not 
an equivalent work budget.
   
   Current source:
   
   - 
[Constant](https://github.com/apache/trafficserver/blob/390bafd7683d14de7faee45129a122557456c092/plugins/regex_remap/regex_remap.cc#L60)
   - [Setting the 
limit](https://github.com/apache/trafficserver/blob/390bafd7683d14de7faee45129a122557456c092/plugins/regex_remap/regex_remap.cc#L920)
   - [PCRE2 
wrapper](https://github.com/apache/trafficserver/blob/390bafd7683d14de7faee45129a122557456c092/src/tsutil/Regex.cc#L303)
   
   ### Standalone reproducer
   
   Tested with PCRE2 10.44 on Linux x86_64. This exercises the underlying 
library with the pattern/options and work limit used by the plugin; it is not 
an end-to-end ATS test.
   
   With `pcre2test` on PATH:
   
   ```sh
   python3 - <<'PY' | pcre2test
   subject = '/cms?partner=TOKEN&x=' + 'a' * 2000
   for mode in ['', 'jit']:
       for budget in [1750, 10000000]:
           print(r'~^/cms(\?.*)TOKEN(.*)$~' + mode)
           print(subject + r'\=match_limit=' + str(budget))
           print()
   PY
   ```
   
   The subject is 2,021 bytes. Observed results:
   
   | Engine | Match limit | Result |
   | --- | --- | --- |
   | PCRE2 10.44, interpreted | 1,750 | `Failed: error -47: match limit 
exceeded` |
   | PCRE2 10.44, interpreted | 10,000,000 | Matches, with both capture groups |
   | PCRE2 10.44, JIT | 1,750 | `Failed: error -47: match limit exceeded` |
   | PCRE2 10.44, JIT | 10,000,000 | Matches, with both capture groups |
   
   A separate direct-library check using PCRE 8.45, `pcre_study(..., 
PCRE_STUDY_EXTRA_NEEDED, ...)`, and the old plugin's recursion limit of 1,750 
successfully matched the equivalent pattern and subject. A short subject also 
succeeds under the new limit. Moving the token to the end of the long subject 
succeeds: the position of the token and amount of backtracking matter, not just 
the total URL length.
   
   The first greedy `.*` consumes the query and then backs up to find the 
token. This ordinary matching work is enough to exhaust the new budget; a 
nested or exponentially backtracking pattern is not necessary.
   
   ### Impact
   
   For a rule such as:
   
   ```text
   ^/cms(\?.*)TOKEN(.*)$ https://example.com/redirect$1TOKEN$2 @status=302
   ```
   
   the plugin's [error 
branch](https://github.com/apache/trafficserver/blob/390bafd7683d14de7faee45129a122557456c092/plugins/regex_remap/regex_remap.cc#L1171)
 increments its failure counter, logs the error, and continues to the next 
rule. The failed rule's substitution and 302 are not applied. If no later rule 
matches, it returns `TSREMAP_NO_REMAP`. Thus this can affect routing, not 
merely produce noisy logs. This control-flow consequence is established by 
source inspection.
   
   ### Expected behavior / fix direction
   
   Preserve sensible resource limits without treating the old recursion-depth 
value as a total matching-work budget. PCRE2's default match limit is 
10,000,000 in the tested build. Work, depth, and JIT stack limits need to be 
considered separately; simply replacing the call with a depth limit does not 
protect JIT execution because JIT ignores the depth limit.
   
   Please add regression coverage for long but simple matching URLs and for the 
plugin's expected redirect behavior. The [PCRE2 API 
documentation](https://pcre.org/current/doc/html/pcre2api.html) describes the 
different match/depth limits and JIT behavior.
   


-- 
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]

Reply via email to