Bug 52773 - [RegexFuzz] Crash in generated code
Summary: [RegexFuzz] Crash in generated code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-01-19 17:35 PST by Oliver Hunt
Modified: 2011-01-20 13:32 PST (History)
4 users (show)

See Also:


Attachments
Patch (5.02 KB, patch)
2011-01-20 10:10 PST, Michael Saboff
oliver: review-
Details | Formatted Diff | Diff
Patch with changes suggested by reviewer. (4.90 KB, patch)
2011-01-20 10:58 PST, Michael Saboff
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2011-01-19 17:35:02 PST
testRegexp("((?!(?:|)v{2,}|))","",["","","","","vt"])

Reduced to:
/((?!(?:|)v{2,}|))/.exec("vt")
Comment 1 Geoffrey Garen 2011-01-19 18:09:08 PST
<rdar://problem/8890203>
Comment 2 Michael Saboff 2011-01-20 10:10:39 PST
Created attachment 79611 [details]
Patch

Changed YarrGenerator::TermGenerationState::linkDataLabelToBacktrackIfExists() to resolve a DataLabelPtr is possible so that we don't overwrite an existing unresolved DataLabelPtr.
Comment 3 Oliver Hunt 2011-01-20 10:24:01 PST
Comment on attachment 79611 [details]
Patch

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

> Source/JavaScriptCore/yarr/YarrJIT.cpp:944
> +                // If we have a backtrack label, connect the datalabel to it directly.
> +                if (m_backtrack.isLabel())
> +                    generator->m_expressionState.m_backtrackRecords.append(AlternativeBacktrackRecord(dataLabel, m_backtrack.getLabel()));
> +                else
> +                    setBacktrackDataLabel(dataLabel);

Is it at all possible to add an assertion so the we catch any attempt to clobber a label in future?
Also this doesn't appear to clear m_backtrack any more so afaict every label will be pushed onto the backtrack record stack

> Source/WebCore/ChangeLog:11
> +2011-01-20  Michael Saboff  <msaboff@apple.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Need a short description and bug URL (OOPS!)
> +
> +        * WebCore.xcodeproj/project.pbxproj:
> +
>  2011-01-20  Andreas Kling  <kling@webkit.org>
>  
>          Reviewed by Ariya Hidayat.

No changes to webcore => this changelog is bogus
Comment 4 Michael Saboff 2011-01-20 10:58:21 PST
Created attachment 79618 [details]
Patch with changes suggested by reviewer.

Added requested ASSERT, built --Debug and ran tests with successful results.

The ClearDataLabel() call is no longer needed since we use the argument data label directly AND we want to preserve the existing data label.

Removed the bogus WebCore change log.
Comment 5 Oliver Hunt 2011-01-20 11:09:27 PST
Comment on attachment 79618 [details]
Patch with changes suggested by reviewer.

r=me
Comment 6 Michael Saboff 2011-01-20 13:32:01 PST
Committed r76275: <http://trac.webkit.org/changeset/76275>