Bug 159954 - CrashOnOverflow in JSC::Yarr::YarrPatternConstructor::setupAlternativeOffsets
Summary: CrashOnOverflow in JSC::Yarr::YarrPatternConstructor::setupAlternativeOffsets
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-07-19 15:24 PDT by Michael Saboff
Modified: 2016-07-20 07:51 PDT (History)
4 users (show)

See Also:


Attachments
Patch (19.90 KB, patch)
2016-07-19 15:37 PDT, Michael Saboff
benjamin: review+
Details | Formatted Diff | Diff
Patch for landing with suggested fixes (21.22 KB, patch)
2016-07-19 15:49 PDT, Michael Saboff
msaboff: review-
Details | Formatted Diff | Diff
Patch for landing - exported YarrPattern::errorMessage (21.97 KB, patch)
2016-07-19 23:28 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2016-07-19 15:24:15 PDT
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)
    ...
Comment 1 Michael Saboff 2016-07-19 15:24:43 PDT
<rdar://problem/27009529>
Comment 2 Michael Saboff 2016-07-19 15:37:29 PDT
Created attachment 284063 [details]
Patch
Comment 3 Benjamin Poulain 2016-07-19 15:40:34 PDT
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.
Comment 4 Saam Barati 2016-07-19 15:43:12 PDT
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
Comment 5 Keith Miller 2016-07-19 15:45:21 PDT
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.
Comment 6 Michael Saboff 2016-07-19 15:46:32 PDT
(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.
Comment 7 Michael Saboff 2016-07-19 15:49:28 PDT
Created attachment 284064 [details]
Patch for landing with suggested fixes
Comment 8 Michael Saboff 2016-07-19 16:15:40 PDT
Comment on attachment 284064 [details]
Patch for landing with suggested fixes

Need to export YarrPattern::error() to the rest of webkit.
Comment 9 Michael Saboff 2016-07-19 23:28:57 PDT
Created attachment 284088 [details]
Patch for landing - exported YarrPattern::errorMessage
Comment 10 Michael Saboff 2016-07-20 07:51:31 PDT
Committed r203452: <http://trac.webkit.org/changeset/203452>