Bug 220130 – [YARR JIT] Crash on overflow when compiling /(a{1000000000}b{1000000000}|c{1000000000}|)d{1000000000}e{1000000000}/.test();

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

rdar://69884758

[Mark Lam]

Created attachment 416731 [details]
proposed patch.

[Daniel Bates]

Comment on attachment 416731 [details]
proposed patch.

Seems sane.

[Mark Lam]

Thanks.  I would still like Michael to take a look before I land, especially on whether I should be using a different error code.

[Michael Saboff]

Comment on attachment 416731 [details]
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.

[Mark Lam]

Michael is going to implement the real fix.

[Michael Saboff]

We do some checked math in the Yarr JIT compiler in the wrong order, addition before subtraction causing checked arithmetic overflows.

[Michael Saboff]

Created attachment 421381 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 421381 [details]
Patch

r=me

[EWS]

Committed r273371: <https://commits.webkit.org/r273371>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421381 [details].