bryancall commented on PR #13652:
URL: https://github.com/apache/trafficserver/pull/13652#issuecomment-5593375834
Two corrections to my review, one of which changes what I am asking you for.
## I implied the conversion broke the 3 KB case. It did not.
I said the long lookahead transaction in `replay/yts-2819.replay.json`
"fails the
same way before and after this change". That is true of this pull request,
but I
let it read as though #12575 introduced the failure. It did not, and I should
have checked before writing it that way.
I built the pre-conversion code path against PCRE1 8.45, which is the engine
regex_remap used before #12575 and 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 real regression from #12575: that URL matched before the
conversion and stopped matching after it. The lookahead URL has been failing
continuously since 2019 and is not something the conversion broke. Only the
reason changed, from a deliberate recursion cap to an incidental 32 KB
just-in-time stack.
## I called the gold file update bookkeeping. It is a crash regression test.
This is the one that matters. In my review I wrote that giving the plugin the
larger just-in-time stack means the 3 KB URL starts matching, "so
`gold/regex_remap_crash.gold` and the 'match limit 0' run need updating in
the
same change". That undersold it badly.
That test came from #5762, where Alan Carroll wrote:
> This was the cause of a production crash. [...] This also adds an AuTest to
> validate the fix (without it, the second request will crash ATS).
The failure is the pass condition. His original assertion was:
```python
tr = Test.AddTestRun("crash test")
ts.Disk.diags_log.Content = Testers.ContainsExpression(
'ERROR: [regex_remap] Bad regular expression result -21', "Resource
limit exceeded")
```
`-21` is the PCRE1 recursion limit error. #12575 retargeted it:
```
-tr = Test.AddTestRun("crash test")
+tr = Test.AddTestRun("match limit 0")
- 'Bad regular expression result -21', "Resource limit exceeded")
+ 'Bad regular expression result -47', "Match limit exceeded")
```
So the assertion now tracks whatever the implementation emits rather than the
property being guarded, and the word "crash" is gone from the name. That is
how
it got to today's state, where the transaction actually produces -46 and the
run
still passes because a different run writes -47 into the same log.
The consequence for my own suggestion: if the plugin gets the 1 MB stack,
that
pattern matches, and a guard that has stood over a real production crash
since
2019 quietly stops guarding. Whoever makes that change owes a replacement
case,
a subject long enough to still exhaust 1 MB, asserting graceful failure
rather
than a redirect. My suggestion did not have one, so as written it deleted a
test
and called it a rename.
## Where that leaves the review
**I am no longer asking you to fix the just-in-time stack here.** It is a
separate defect in `RegexMatchContext`, it predates the symptom you are
fixing,
and it needs its own test work. I will take it to its own pull request.
Sorry for
putting it in your path.
Two things from the review still stand.
The `Backport` label needs a branch qualification, and this is the one I
would
still hold for. 10.1.x and 9.2.x both still run the pre-conversion code with
`match_limit_recursion = 1750`, which is correct semantics and is the actual
stack protection #5762 added. Given Alan's "the second request will crash
ATS",
removing it there is a crash, not a missed redirect.
The "match limit 0" run is not exercising a match limit, for the reasons
above.
Worth either giving it a subject that does, or renaming it back to what it
guards. Not a blocker on this change.
Everything I said about your new test stands, including that it fails without
the fix and that the 19 to 32 change was necessary rather than cosmetic.
--
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]
