Bug 159954 – CrashOnOverflow in JSC::Yarr::YarrPatternConstructor::setupAlternativeOffsets

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

[Michael Saboff]

We are failing on regular expressions that exceed 2^32 characters.  For example:
  /a{2147483649,2147483650}a{2147483649,2147483650}/

Backtraces look something like:
    #0 0x10a0d2cb8 in WTFCrash (JavaScriptCore+0x2a87cb8)
    #1 0x10766291d in WTF::CrashOnOverflow::crash() (JavaScriptCore+0x1791d)
    #2 0x1076628bd in WTF::CrashOnOverflow::overflowed() (JavaScriptCore+0x178bd)
    #3 0x10a0ab684 in WTF::Checked<unsigned int, WTF::CrashOnOverflow> const WTF::Checked<unsigned int, WTF::CrashOnOverflow>::operator+=<unsigned int>(unsigned int) (JavaScriptCore+0x2a60684)
    #4 0x10a0ab0b0 in WTF::Checked<unsigned int, WTF::CrashOnOverflow> const WTF::Checked<unsigned int, WTF::CrashOnOverflow>::operator+=<unsigned int, WTF::CrashOnOverflow>(WTF::Checked<unsigned int, WTF::CrashOnOverflow>) (JavaScriptCore+0x2a600b0)
    #5 0x10a0a9f6a in JSC::Yarr::YarrPatternConstructor::setupAlternativeOffsets(JSC::Yarr::PatternAlternative*, unsigned int, unsigned int, unsigned int&) (JavaScriptCore+0x2a5ef6a)
    #6 0x10a0a8035 in JSC::Yarr::YarrPatternConstructor::setupDisjunctionOffsets(JSC::Yarr::PatternDisjunction*, unsigned int, unsigned int, unsigned int&) (JavaScriptCore+0x2a5d035)
    #7 0x10a0aa835 in JSC::Yarr::YarrPatternConstructor::setupAlternativeOffsets(JSC::Yarr::PatternAlternative*, unsigned int, unsigned int, unsigned int&) (JavaScriptCore+0x2a5f835)
    #8 0x10a0a8035 in JSC::Yarr::YarrPatternConstructor::setupDisjunctionOffsets(JSC::Yarr::PatternDisjunction*, unsigned int, unsigned int, unsigned int&) (JavaScriptCore+0x2a5d035)
    #9 0x10a09d9f2 in JSC::Yarr::YarrPatternConstructor::setupOffsets() (JavaScriptCore+0x2a529f2)
    #10 0x10a09ba89 in JSC::Yarr::YarrPattern::compile(WTF::String const&, void*) (JavaScriptCore+0x2a50a89)
    ...

[Michael Saboff]

<rdar://problem/27009529>

[Michael Saboff]

Created attachment 284063 [details]
Patch

[Benjamin Poulain]

Comment on attachment 284063 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284063&action=review

No test? :(

> Source/JavaScriptCore/ChangeLog:8
> +        YarrPatternConstructor::setupAlternativeOffsets() is using theecked arithmetic class

typo: "theecked arithmetic"

> Source/JavaScriptCore/yarr/YarrPattern.cpp:671
> +                    error = setupDisjunctionOffsets(term.parentheses.disjunction, currentCallFrameSize + YarrStackSpaceForBackTrackInfoParentheticalAssertion, currentInputPosition.unsafeGet(), currentCallFrameSize);

The indentation looks off.

[Saam Barati]

Comment on attachment 284063 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284063&action=review

> Source/JavaScriptCore/ChangeLog:8
> +        YarrPatternConstructor::setupAlternativeOffsets() is using theecked arithmetic class

typo: "theecked"

> Source/JavaScriptCore/yarr/YarrPattern.cpp:643
> +                    error = setupDisjunctionOffsets(term.parentheses.disjunction, currentCallFrameSize, currentInputPosition.unsafeGet(), currentCallFrameSize);

Nit: I'd make these error checks UNLIKELY

> Source/JavaScriptCore/yarr/YarrPattern.cpp:896
> +        0, // NoError

nullptr

[Keith Miller]

Comment on attachment 284063 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284063&action=review

Do you have a test case?

>> Source/JavaScriptCore/yarr/YarrPattern.cpp:671
>> +                    error = setupDisjunctionOffsets(term.parentheses.disjunction, currentCallFrameSize + YarrStackSpaceForBackTrackInfoParentheticalAssertion, currentInputPosition.unsafeGet(), currentCallFrameSize);
> 
> The indentation looks off.

Whitespace? idk if this is just bugzilla.

[Michael Saboff]

(In reply to comment #3)
> Comment on attachment 284063 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284063&action=review
> 
> No test? :(

Didn't "svn add" the test file when creating the patch.

> > Source/JavaScriptCore/ChangeLog:8
> > +        YarrPatternConstructor::setupAlternativeOffsets() is using theecked arithmetic class
> 
> typo: "theecked arithmetic"

Fixed.
 
> > Source/JavaScriptCore/yarr/YarrPattern.cpp:671
> > +                    error = setupDisjunctionOffsets(term.parentheses.disjunction, currentCallFrameSize + YarrStackSpaceForBackTrackInfoParentheticalAssertion, currentInputPosition.unsafeGet(), currentCallFrameSize);
> 
> The indentation looks off.

Fixed.

[Michael Saboff]

Created attachment 284064 [details]
Patch for landing with suggested fixes

[Michael Saboff]

Comment on attachment 284064 [details]
Patch for landing with suggested fixes

Need to export YarrPattern::error() to the rest of webkit.

[Michael Saboff]

Created attachment 284088 [details]
Patch for landing - exported YarrPattern::errorMessage

[Michael Saboff]

Committed r203452: <http://trac.webkit.org/changeset/203452>