Limit resources used by regex_remap to prevent crashes on stack overflow - #5762
Merged
SolidWallOfCode merged 2 commits intoAug 15, 2019
Merged
Conversation
Contributor
|
[approve ci autest] |
Member
Author
|
This is stuck on a bug in AuTest, @macisasandwich is working on it. |
x-jesse-zhang
suggested changes
Aug 5, 2019
| tr.Processes.Default.Streams.stdout = "gold/regex_remap_smoke.gold" | ||
| tr.StillRunningAfter = ts | ||
|
|
||
| # Crash test. |
Contributor
There was a problem hiding this comment.
To address the TS failure in Autest, you need to this to test your diags.log
ts.Disk.diags_log.Content = Testers.ContainsExpression(<expression you want>, <description>)
| Test.Summary = ''' | ||
| Test regex_remap | ||
| ''' | ||
|
|
Contributor
There was a problem hiding this comment.
You can add Test.Variables.Autest.NormalizeKill = 0 to work around the -9 sigkill return code on microserver while I try to work up a fix.
SolidWallOfCode
force-pushed
the
regex_remap-pcre-fix
branch
from
August 5, 2019 16:46
c3a7353 to
af4b4b8
Compare
SolidWallOfCode
force-pushed
the
regex_remap-pcre-fix
branch
from
August 6, 2019 21:45
af4b4b8 to
926c550
Compare
shinrich
reviewed
Aug 14, 2019
| _extra = pcre_study(_rex, 0, &error); | ||
| if ((_extra == nullptr) && (error != nullptr)) { | ||
| _extra = pcre_study(_rex, PCRE_STUDY_EXTRA_NEEDED, &error); | ||
| if (error != nullptr) { |
Member
There was a problem hiding this comment.
Should you still have a test somewhere to return if _extra is nullptr before you dereference it?
Member
Author
There was a problem hiding this comment.
No. Because of the PCRE_STUDY_EXTRA_NEEDED, the returned value will only be nullptr if memory is exhausted, in which case we're already over the cliff.
shinrich
approved these changes
Aug 14, 2019
shinrich
left a comment
shinrich
left a comment
Member
There was a problem hiding this comment.
Looks good save for the nullptr check comment.
bneradt
pushed a commit
to bneradt/trafficserver
that referenced
this pull request
Sep 9, 2026
Moving regex_remap to the shared Regex context changes JIT stack behavior beyond the matching-work regression. The original 3 KB lookahead request already failed before the PCRE2 conversion and is part of the crash guard from apache#5762, not a new redirect regression. This patch restores the per-instance context and non-redirecting crash expectation while retaining the removal of the 1750 work limit. The independent log assertions identify resource exhaustion for the crash guard and match-work exhaustion for the separate nested-quantifier rule. The ordinary long-query coverage and other review improvements remain in place; the shared-context change belongs in a separate PR. Co-authored-by: Codex Astra Medium
bneradt
added a commit
that referenced
this pull request
Sep 9, 2026
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, causing ordinary long queries that previously matched to fail. This patch removes the work-limit override while retaining the per-instance match context and existing JIT stack behavior. PCRE2's normal work default is 10 million, allowing more worst-case CPU time per match while 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 value therefore does not translate into a suitable matching-work budget. This patch adds long-query redirect and capture-preservation coverage and extends the excessive-backtracking input to exercise the default work limit. The original 3 KB lookahead case remains a non-redirecting crash guard from #5762; its failure predates the PCRE2 conversion. Independent rule-specific log assertions preserve both checks. Fixes: #13651 Reported-by: Vinith Bindiganavale Co-authored-by: Codex Astra Medium
cmcfarlen
pushed a commit
to cmcfarlen/trafficserver
that referenced
this pull request
Sep 9, 2026
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 apache#6819. The PCRE2 conversion in apache#12575 accidentally reused 1750 as a matching-work limit, causing ordinary long queries that previously matched to fail. This patch removes the work-limit override while retaining the per-instance match context and existing JIT stack behavior. PCRE2's normal work default is 10 million, allowing more worst-case CPU time per match while 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 value therefore does not translate into a suitable matching-work budget. This patch adds long-query redirect and capture-preservation coverage and extends the excessive-backtracking input to exercise the default work limit. The original 3 KB lookahead case remains a non-redirecting crash guard from apache#5762; its failure predates the PCRE2 conversion. Independent rule-specific log assertions preserve both checks. Fixes: apache#13651 Reported-by: Vinith Bindiganavale Co-authored-by: Codex Astra Medium (cherry picked from commit 7ed34a3)
This was referenced Sep 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This was the cause of a production crash. The root using a negative matching regular expression in
regex_remap. Because of the nature of such things, this will recurse roughly once for every character in the URL. This consumes roughly 500 bytes (according to the PCRE documentation) so 2000 or so is enough to blow the ATS stack.With this change, the recursion is limited to protect the stack and an error logged when the regular expression fails to match due to hitting this limit.
This also adds an AuTest to validate the fix (without it, the second request will crash ATS).