[webkit-reviews] review denied: [Bug 220130] Add ability to handle checked offset overflow while compiling a RegExp. : [Attachment 416731] proposed patch.

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has denied  review:
Bug 220130: Add ability to handle checked offset overflow while compiling a
RegExp.
https://bugs.webkit.org/show_bug.cgi?id=220130

Attachment 416731: proposed patch.

https://bugs.webkit.org/attachment.cgi?id=416731&action=review




--- Comment #4 from Michael Saboff <msaboff at apple.com> ---
Comment on attachment 416731
  --> https://bugs.webkit.org/attachment.cgi?id=416731
proposed patch.

I spent some time trying the patch.  The test case should not cause an
overflow.  The patch found a bug and through code inspection I found another
related bug.  If you look at lines ~3107..3111, you'll notice that is "if
(!isBegin)" is true, we add the offset from the prior op before subtracting the
checked amount for the current op.  This is where we overflow with the test and
this patch.  We should be subtracting the current checked amount before adding
the prior.  When I made this fix, the offset never overflowed.	When I adjust
the test to have combined sounds >= 2^32, we overflow in the parser.

There is a similar "add before subtract" offset case in lines 2801..2805.

In a local build, I change both cases to have the subtract before the add and
ran all JSC tests with no issues.

We can land this patch, but it is more of a canary for newly introduced bugs in
YARR JIT code (after I land my changes).  If we do, we should restructure the
code to assert no overflow.