Restore regex_remap's default PCRE2 match limit

[bneradt]

Valid URLs with long query strings can miss regex_remap redirects. The old PCRE matcher used recursive calls for backtracking, so its recursion limit was reduced from 2047 to 1750 after stack crashes in #6819. The PCRE2 conversion in #12575 accidentally reused 1750 as a matching-work limit, making ordinary long queries fail even though they matched before the conversion.

This patch removes that work-limit override while retaining the per-instance match context and its existing JIT stack behavior. PCRE2's normal work default is 10 million, permitting more worst-case CPU per match than 1750 but still bounding excessive backtracking. Its depth and heap limits remain intact. Since PCRE2 10.30, interpreter backtracking frames reside on the heap; JIT ignores the depth limit and uses a separately bounded stack. The old stack-derived 1750 value does not translate into a suitable matching-work budget.

The original 3 KB lookahead case predates the conversion and remains a non-redirecting crash guard from #5762. A separate shared-context/JIT-stack change is outside this PR. This patch adds ordinary long-query redirect and capture-preservation coverage, extends the excessive-backtracking input to exercise the default work limit, and retains independent rule-specific log assertions. ESI's stale cross-reference is removed without changing its limit. Resource-error fallthrough is tracked separately in #13654.

Backport: 10.2.x only. The 10.1.x and 9.2.x branches still use PCRE's correct recursion-depth limit and must retain it.

Fixes: #13651
Reported-by: Vinith Bindiganavale
Co-authored-by: Codex Astra Medium

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[bneradt]

Current scope after @bryancall's consolidated review: commit 1dee701 restores the per-instance RegexMatchContext and the original non-redirecting crash guard. The shared-context/1 MiB JIT-stack change from dccd8f0 is withdrawn from this PR and will be handled separately.

The fix here removes only the accidental 1750 matching-work override. PCRE2's default work, depth, and heap limits remain in place, as does this plugin's existing JIT-stack behavior. The original 3 KB lookahead case already failed before the PCRE2 conversion; the new ordinary long-query case is the actual conversion regression.

The PR description contains the corrected rationale and 10.2.x-only backport scope. The historical explanation remains outside the source code, and attribution is Co-authored-by: Codex Astra Medium.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

[bryancall]

Superseded by #13652 (comment)

This review asked for a shared-Regex-context change that I have since withdrawn. It is a separate defect that predates this PR, and I am opening it as its own PR rather than holding this one for it.

What still stands from this review: the Backport label needed a branch qualification, which you have already applied (10.2.x only, not 10.1.x or 9.2.x). 10.1.x and 9.2.x still run the pre-conversion PCRE1 code where match_limit_recursion = 1750 is correct semantics and is the actual stack protection added by #5762.

See the linked comment for the current state and the one change I would make.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[bneradt]

@bryancall Addressed in amended commit dccd8f0:

• Removed the per-instance match context and context arguments. The plugin now uses ATS's thread-local Regex context, including its 1 MiB JIT stack. I first changed the 3 KB test to expect its intended 301: it failed against the previous revision with a 200 and -46. After the code change, it passes with the expected 301 and Location. The existing redirect gold file is reused; the 200 gold remains for the excessive-backtracking case.
• Replaced the blanket error exclusion once with a check permitting only the deliberate /match_limit/ error, then appended the independent assertions with +=. The renamed long-query run excludes matching errors for its rule; the excessive-backtracking run requires -47 for its own rule.
• Qualified the Backport label in the commit and PR description: 10.2.x only, not 10.1.x or 9.2.x. I checked all three branch implementations.
• Documented the regression input's required 2,000-byte suffix and distinguished the token-placement/no-token controls from the backtracking case. Both wording fixes are applied.
• Removed ESI's obsolete cross-reference, retaining its independent allowlist work limit.
• Kept the historical explanation in the commit/PR rather than restoring a source-history comment, as requested by the PR author. The rationale now cites regex_remap: Adjust regex recursion limit down due to crashes in testing #6819 and regex_remap: convert from pcre to Regex #12575 and explicitly acknowledges the larger worst-case CPU budget.

The revised regex_remap and regex_remap_long_query AuTests both pass in asfats5; the deliberate excessive-backtracking case still reports -47. Build and formatting checks pass.

I filed #13654 for resource-error fallthrough and left that behavioral decision outside this patch. I also updated my earlier PR comment so it no longer describes the obsolete 32 KiB context as the final design.

[bryancall]

Consolidating, because I made a mess of this thread. This comment supersedes my
earlier one. My review asked for something I should not have asked for here, you
did it, and then I posted a correction that was written against your previous
revision and so was already stale when it landed.

Two facts I got wrong in the review

#12575 did not break the 3 KB lookahead URL. I let my review read that way. I
built the pre-conversion code against PCRE1 8.45, the engine regex_remap used
before #12575, which never enabled just-in-time compilation:

[A] yts-2819 lookahead rule, 3071 byte URL
  with recursion limit 1750 (as shipped)   rc= -21  RECURSIONLIMIT
  with no limit at all                     rc=   2  MATCH

[B] the rule from this PR's new test, 2021 byte URL
  with recursion limit 1750 (as shipped)   rc=   3  MATCH
  with no limit at all                     rc=   3  MATCH

So your bug is a genuine regression from #12575: that URL matched before the
conversion and stopped after. The lookahead URL has been failing since 2019 and
is not something the conversion broke. Only the mechanism changed.

I called the gold file update bookkeeping. It is a crash regression test. It
comes from #5762, where Alan wrote "without it, the second request will crash
ATS". The failure is the pass condition. His assertion was -21, #12575
retargeted it to -47, and the transaction actually emits -46 today. It has
drifted twice because it asserts an error number rather than the property, in a
scope that cannot attribute it.

What I would do with this PR

AuTest 1of4 is red only here. I checked the seven other open PRs and it passes
on all of them, so it is not infrastructure.

Add a new commit, please without amending and force pushing, so the diff stays
reviewable. Revert just these two things to how 3f5cfa1 had them:

• plugins/regex_remap/regex_remap.cc: restore the per-instance
  RegexMatchContext match_context member and passing &(ri->match_context) to
  re->match(). Keep the removal of REGEX_MATCH_LIMIT and its
  set_match_limit() call, which is the actual fix.
• The 3071 byte /alpha/bravo/ transaction: put back the non-redirecting
  expectation against gold/regex_remap_crash.gold. Keep the 19 to 32 change on
  the /match_limit/ subject, which is necessary and correct.

Keep everything else you added. All of it is independent of the context change
and all of it is an improvement: the Backport qualification to 10.2.x only, +=
instead of = on the two diags_log assertions, the ESI cross-reference removal,
the documented 2000 byte minimum, the wording fixes, the long_query.conf
comments, and #13654.

What I will do

I will open the shared-context change as its own PR, with the resource-limit
behavior asserted in test_Regex.cc instead of through curl and gold files, and
with a replacement for the #5762 crash guard. It belongs in a unit test because
the AuTest cannot see what it is testing: a recursion-limit error, a work-limit
error, a just-in-time stack error, and no limit at all all render as
HTTP/1.1 200 OK to curl. That is how the assertion drifted twice unnoticed.

That part is mine to carry, not yours. Your change fixes a ten month old
regression and adds the first coverage this plugin has ever had for the ordinary
long URL case, which is why it caught what two engine conversions did not.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[bneradt]

@bryancall Applied the consolidated correction in new commit 1dee701, pushed normally without amending or force-pushing.

• regex_remap.cc now matches 3f5cfa1 exactly: the per-instance match context and explicit context arguments are restored, while the 1750 work-limit override remains removed.
• The original /alpha/bravo/ request again uses gold/regex_remap_crash.gold and verifies that ATS survives resource exhaustion without redirecting. Its appended diagnostic assertion is tied to that rule and accepts the resource-limit category rather than incorrectly requiring -47.
• The independent /match_limit/ assertion still requires -47, and the 32-character subject remains. The other review improvements are retained.

Both regex_remap and regex_remap_long_query pass in asfats5, as do the build and formatting checks. The restored crash guard reports -46 with a 200 response; the separate work-limit case reports -47; the ordinary long-query redirects pass.

I updated the PR title, description, and earlier scope comment to distinguish the conversion regression from the pre-existing crash-guard behavior. The shared-context change is left for your separate PR, and the backport scope remains 10.2.x only.

[cmcfarlen]

Cherry-picked to the 10.2.x branch as 72c0d24 for the 10.2.1 release.