2011-01-14 Oliver Hunt <oliver@apple.com>
authoroliver@apple.com <oliver@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 15 Jan 2011 01:22:58 +0000 (01:22 +0000)
committeroliver@apple.com <oliver@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 15 Jan 2011 01:22:58 +0000 (01:22 +0000)
        Reviewed by Gavin Barraclough.

        [jsfunfuzz] parser doesn't enforce continue restrictions correctly.
        https://bugs.webkit.org/show_bug.cgi?id=52493

        Add a few tests for continue to cover the cases where continue
        isn't syntactically valid.

        * fast/js/js-continue-break-restrictions-expected.txt: Added.
        * fast/js/js-continue-break-restrictions.html: Added.
        * fast/js/script-tests/js-continue-break-restrictions.js: Added.
2011-01-14  Oliver Hunt  <oliver@apple.com>

        Reviewed by Gavin Barraclough.

        [jsfunfuzz] parser doesn't enforce continue restrictions correctly.
        https://bugs.webkit.org/show_bug.cgi?id=52493

        This patch reworks handling of break, continue and label statements
        to correctly handle all the valid and invalid cases.  Previously certain
        errors would be missed by the parser in strict mode, but the bytecode
        generator needed to handle those cases for non-strict code so nothing
        failed, it simply became non-standard behaviour.

        Now that we treat break and continue errors as early faults in non-strict
        mode as well that safety net has been removed so the parser bugs result in
        crashes at codegen time.

        * parser/JSParser.cpp:
        (JSC::JSParser::ScopeLabelInfo::ScopeLabelInfo):
        (JSC::JSParser::next):
        (JSC::JSParser::nextTokenIsColon):
        (JSC::JSParser::continueIsValid):
            Continue is only valid in loops so we can't use breakIsValid()
        (JSC::JSParser::pushLabel):
            We now track whether the label is for a loop (and is therefore a
            valid target for continue.
        (JSC::JSParser::popLabel):
        (JSC::JSParser::getLabel):
            Replace hasLabel with getLabel so that we can validate the target
            when parsing continue statements.
        (JSC::JSParser::Scope::continueIsValid):
        (JSC::JSParser::Scope::pushLabel):
        (JSC::JSParser::Scope::getLabel):
        (JSC::JSParser::JSParser):
        (JSC::JSParser::parseBreakStatement):
        (JSC::JSParser::parseContinueStatement):
        (JSC::LabelInfo::LabelInfo):
        (JSC::JSParser::parseExpressionOrLabelStatement):
            Consecutive labels now get handled iteratively so that we can determine
            whether they're valid targets for continue.
        * parser/Lexer.cpp:
        (JSC::Lexer::nextTokenIsColon):
        * parser/Lexer.h:
        (JSC::Lexer::setOffset):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@75852 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/js/js-continue-break-restrictions-expected.txt [new file with mode: 0644]
LayoutTests/fast/js/js-continue-break-restrictions.html [new file with mode: 0644]
LayoutTests/fast/js/script-tests/js-continue-break-restrictions.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/parser/JSParser.cpp
Source/JavaScriptCore/parser/Lexer.cpp
Source/JavaScriptCore/parser/Lexer.h

index ccf6b28..a168380 100644 (file)
@@ -1,3 +1,17 @@
+2011-01-14  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Gavin Barraclough.
+
+        [jsfunfuzz] parser doesn't enforce continue restrictions correctly.
+        https://bugs.webkit.org/show_bug.cgi?id=52493
+
+        Add a few tests for continue to cover the cases where continue
+        isn't syntactically valid.
+
+        * fast/js/js-continue-break-restrictions-expected.txt: Added.
+        * fast/js/js-continue-break-restrictions.html: Added.
+        * fast/js/script-tests/js-continue-break-restrictions.js: Added.
+
 2011-01-14  Maciej Stachowiak  <mjs@apple.com>
 
         Reviewed by Anders Carlsson.
diff --git a/LayoutTests/fast/js/js-continue-break-restrictions-expected.txt b/LayoutTests/fast/js/js-continue-break-restrictions-expected.txt
new file mode 100644 (file)
index 0000000..75b073c
--- /dev/null
@@ -0,0 +1,37 @@
+Verify that invalid continue and break statements are handled correctly
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS L:{true;break L;false} is true
+PASS if (0) { L:{ break; } } threw exception SyntaxError: Parse error.
+PASS if (0) { L:{ continue L; } } threw exception SyntaxError: Parse error.
+PASS if (0) { L:{ continue; } } threw exception SyntaxError: Parse error.
+PASS if (0) { switch (1) { case 1: continue; } } threw exception SyntaxError: Parse error.
+PASS A:L:{true;break L;false} is true
+PASS if (0) { A:L:{ break; } } threw exception SyntaxError: Parse error.
+PASS if (0) { A:L:{ continue L; } } threw exception SyntaxError: Parse error.
+PASS if (0) { A:L:{ continue; } } threw exception SyntaxError: Parse error.
+PASS L:A:{true;break L;false} is true
+PASS if (0) { L:A:{ break; } } threw exception SyntaxError: Parse error.
+PASS if (0) { L:A:{ continue L; } } threw exception SyntaxError: Parse error.
+PASS if (0) { L:A:{ continue; } } threw exception SyntaxError: Parse error.
+PASS if(0){ L:for(;;) continue L; } is undefined.
+PASS if(0){ L:A:for(;;) continue L; } is undefined.
+PASS if(0){ A:L:for(;;) continue L; } is undefined.
+PASS if(0){ A:for(;;) L:continue L; } threw exception SyntaxError: Parse error.
+PASS if(0){ L:for(;;) A:continue L; } is undefined.
+PASS if(0){ L:do continue L; while(0); } is undefined.
+PASS if(0){ L:A:do continue L; while(0); } is undefined.
+PASS if(0){ A:L:do continue L; while(0);} is undefined.
+PASS if(0){ A:do L:continue L; while(0); } threw exception SyntaxError: Parse error.
+PASS if(0){ L:do A:continue L; while(0); } is undefined.
+PASS if(0){ L:while(0) continue L; } is undefined.
+PASS if(0){ L:A:while(0) continue L; } is undefined.
+PASS if(0){ A:L:while(0) continue L; } is undefined.
+PASS if(0){ A:while(0) L:continue L; } threw exception SyntaxError: Parse error.
+PASS if(0){ L:while(0) A:continue L; } is undefined.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/js/js-continue-break-restrictions.html b/LayoutTests/fast/js/js-continue-break-restrictions.html
new file mode 100644 (file)
index 0000000..6aa9e88
--- /dev/null
@@ -0,0 +1,13 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<link rel="stylesheet" href="resources/js-test-style.css">
+<script src="resources/js-test-pre.js"></script>
+</head>
+<body>
+<p id="description"></p>
+<div id="console"></div>
+<script src="script-tests/js-continue-break-restrictions.js"></script>
+<script src="resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/fast/js/script-tests/js-continue-break-restrictions.js b/LayoutTests/fast/js/script-tests/js-continue-break-restrictions.js
new file mode 100644 (file)
index 0000000..f50485d
--- /dev/null
@@ -0,0 +1,38 @@
+description("Verify that invalid continue and break statements are handled correctly");
+
+shouldBeTrue("L:{true;break L;false}");
+shouldThrow("if (0) { L:{ break; } }");
+shouldThrow("if (0) { L:{ continue L; } }");
+shouldThrow("if (0) { L:{ continue; } }");
+shouldThrow("if (0) { switch (1) { case 1: continue; } }");
+shouldBeTrue("A:L:{true;break L;false}");
+shouldThrow("if (0) { A:L:{ break; } }");
+shouldThrow("if (0) { A:L:{ continue L; } }");
+shouldThrow("if (0) { A:L:{ continue; } }");
+
+shouldBeTrue("L:A:{true;break L;false}");
+shouldThrow("if (0) { L:A:{ break; } }");
+shouldThrow("if (0) { L:A:{ continue L; } }");
+shouldThrow("if (0) { L:A:{ continue; } }");
+
+shouldBeUndefined("if(0){ L:for(;;) continue L; }")
+shouldBeUndefined("if(0){ L:A:for(;;) continue L; }")
+shouldBeUndefined("if(0){ A:L:for(;;) continue L; }")
+shouldThrow("if(0){ A:for(;;) L:continue L; }")
+shouldBeUndefined("if(0){ L:for(;;) A:continue L; }")
+
+shouldBeUndefined("if(0){ L:do continue L; while(0); }")
+shouldBeUndefined("if(0){ L:A:do continue L; while(0); }")
+shouldBeUndefined("if(0){ A:L:do continue L; while(0);}")
+shouldThrow("if(0){ A:do L:continue L; while(0); }")
+shouldBeUndefined("if(0){ L:do A:continue L; while(0); }")
+
+
+shouldBeUndefined("if(0){ L:while(0) continue L; }")
+shouldBeUndefined("if(0){ L:A:while(0) continue L; }")
+shouldBeUndefined("if(0){ A:L:while(0) continue L; }")
+shouldThrow("if(0){ A:while(0) L:continue L; }")
+shouldBeUndefined("if(0){ L:while(0) A:continue L; }")
+
+
+var successfullyParsed = true;
index 4f01a8b..a3043fb 100644 (file)
@@ -1,3 +1,48 @@
+2011-01-14  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Gavin Barraclough.
+
+        [jsfunfuzz] parser doesn't enforce continue restrictions correctly.
+        https://bugs.webkit.org/show_bug.cgi?id=52493
+
+        This patch reworks handling of break, continue and label statements
+        to correctly handle all the valid and invalid cases.  Previously certain
+        errors would be missed by the parser in strict mode, but the bytecode 
+        generator needed to handle those cases for non-strict code so nothing
+        failed, it simply became non-standard behaviour.
+
+        Now that we treat break and continue errors as early faults in non-strict
+        mode as well that safety net has been removed so the parser bugs result in
+        crashes at codegen time.
+
+        * parser/JSParser.cpp:
+        (JSC::JSParser::ScopeLabelInfo::ScopeLabelInfo):
+        (JSC::JSParser::next):
+        (JSC::JSParser::nextTokenIsColon):
+        (JSC::JSParser::continueIsValid):
+            Continue is only valid in loops so we can't use breakIsValid()
+        (JSC::JSParser::pushLabel):
+            We now track whether the label is for a loop (and is therefore a
+            valid target for continue.
+        (JSC::JSParser::popLabel):
+        (JSC::JSParser::getLabel):
+            Replace hasLabel with getLabel so that we can validate the target
+            when parsing continue statements.
+        (JSC::JSParser::Scope::continueIsValid):
+        (JSC::JSParser::Scope::pushLabel):
+        (JSC::JSParser::Scope::getLabel):
+        (JSC::JSParser::JSParser):
+        (JSC::JSParser::parseBreakStatement):
+        (JSC::JSParser::parseContinueStatement):
+        (JSC::LabelInfo::LabelInfo):
+        (JSC::JSParser::parseExpressionOrLabelStatement):
+            Consecutive labels now get handled iteratively so that we can determine
+            whether they're valid targets for continue.
+        * parser/Lexer.cpp:
+        (JSC::Lexer::nextTokenIsColon):
+        * parser/Lexer.h:
+        (JSC::Lexer::setOffset):
+
 2011-01-14  Patrick Gansterer  <paroga@webkit.org>
 
         Reviewed by Adam Roben.
index 142f403..792d19b 100644 (file)
@@ -84,6 +84,16 @@ private:
         JSParser* m_parser;
         bool m_oldAllowsIn;
     };
+    
+    struct ScopeLabelInfo {
+        ScopeLabelInfo(StringImpl* ident, bool isLoop)
+        : m_ident(ident)
+        , m_isLoop(isLoop)
+        {
+        }
+        StringImpl* m_ident;
+        bool m_isLoop;
+    };
 
     void next(Lexer::LexType lexType = Lexer::IdentifyReservedWords)
     {
@@ -91,7 +101,11 @@ private:
         m_lastTokenEnd = m_token.m_info.endOffset;
         m_lexer->setLastLineNumber(m_lastLine);
         m_token.m_type = m_lexer->lex(&m_token.m_data, &m_token.m_info, lexType, strictMode());
-        m_tokenCount++;
+    }
+    
+    bool nextTokenIsColon()
+    {
+        return m_lexer->nextTokenIsColon();
     }
 
     bool consume(JSTokenType expected)
@@ -140,18 +154,29 @@ private:
         }
         return true;
     }
-    void pushLabel(const Identifier* label) { currentScope()->pushLabel(label); }
-    void popLabel() { currentScope()->popLabel(); }
-    bool hasLabel(const Identifier* label)
+    bool continueIsValid()
     {
         ScopeRef current = currentScope();
-        while (!current->hasLabel(label)) {
+        while (!current->continueIsValid()) {
             if (!current.hasContainingScope())
                 return false;
             current = current.containingScope();
         }
         return true;
     }
+    void pushLabel(const Identifier* label, bool isLoop) { currentScope()->pushLabel(label, isLoop); }
+    void popLabel() { currentScope()->popLabel(); }
+    ScopeLabelInfo* getLabel(const Identifier* label)
+    {
+        ScopeRef current = currentScope();
+        ScopeLabelInfo* result = 0;
+        while (!(result = current->getLabel(label))) {
+            if (!current.hasContainingScope())
+                return 0;
+            current = current.containingScope();
+        }
+        return result;
+    }
 
     enum SourceElementsMode { CheckForStrictMode, DontCheckForStrictMode };
     template <SourceElementsMode mode, class TreeBuilder> TreeSourceElements parseSourceElements(TreeBuilder&);
@@ -224,7 +249,6 @@ private:
     JSGlobalData* m_globalData;
     JSToken m_token;
     bool m_allowsIn;
-    int m_tokenCount;
     int m_lastLine;
     int m_lastTokenEnd;
     int m_assignmentCount;
@@ -250,7 +274,7 @@ private:
         int m_originalDepth;
         int* m_depth;
     };
-
+    
     struct Scope {
         Scope(JSGlobalData* globalData, bool isFunction, bool strictMode)
             : m_globalData(globalData)
@@ -274,12 +298,13 @@ private:
         void endLoop() { ASSERT(m_loopDepth); m_loopDepth--; }
         bool inLoop() { return !!m_loopDepth; }
         bool breakIsValid() { return m_loopDepth || m_switchDepth; }
+        bool continueIsValid() { return m_loopDepth; }
 
-        void pushLabel(const Identifier* label)
+        void pushLabel(const Identifier* label, bool isLoop)
         {
             if (!m_labels)
                 m_labels = new LabelStack;
-            m_labels->append(label->impl());
+            m_labels->append(ScopeLabelInfo(label->impl(), isLoop));
         }
 
         void popLabel()
@@ -289,15 +314,15 @@ private:
             m_labels->removeLast();
         }
 
-        bool hasLabel(const Identifier* label)
+        ScopeLabelInfo* getLabel(const Identifier* label)
         {
             if (!m_labels)
-                return false;
+                return 0;
             for (int i = m_labels->size(); i > 0; i--) {
-                if (m_labels->at(i - 1) == label->impl())
-                    return true;
+                if (m_labels->at(i - 1).m_ident == label->impl())
+                    return &m_labels->at(i - 1);
             }
-            return false;
+            return 0;
         }
 
         void setIsFunction()
@@ -405,7 +430,8 @@ private:
         bool m_isValidStrictMode : 1;
         int m_loopDepth;
         int m_switchDepth;
-        typedef Vector<StringImpl*, 2> LabelStack;
+
+        typedef Vector<ScopeLabelInfo, 2> LabelStack;
         LabelStack* m_labels;
         IdentifierSet m_declaredVariables;
         IdentifierSet m_usedVariables;
@@ -532,7 +558,6 @@ JSParser::JSParser(Lexer* lexer, JSGlobalData* globalData, FunctionParameters* p
     , m_errorMessage("Parse error")
     , m_globalData(globalData)
     , m_allowsIn(true)
-    , m_tokenCount(0)
     , m_lastLine(0)
     , m_lastTokenEnd(0)
     , m_assignmentCount(0)
@@ -862,7 +887,7 @@ template <class TreeBuilder> TreeStatement JSParser::parseBreakStatement(TreeBui
     }
     matchOrFail(IDENT);
     const Identifier* ident = m_token.m_data.ident;
-    failIfFalse(hasLabel(ident));
+    failIfFalse(getLabel(ident));
     endCol = tokenEnd();
     endLine = tokenLine();
     next();
@@ -880,12 +905,14 @@ template <class TreeBuilder> TreeStatement JSParser::parseContinueStatement(Tree
     next();
 
     if (autoSemiColon()) {
-        failIfFalse(breakIsValid());
+        failIfFalse(continueIsValid());
         return context.createContinueStatement(startCol, endCol, startLine, endLine);
     }
     matchOrFail(IDENT);
     const Identifier* ident = m_token.m_data.ident;
-    failIfFalse(hasLabel(ident));
+    ScopeLabelInfo* label = getLabel(ident);
+    failIfFalse(label);
+    failIfFalse(label->m_isLoop);
     endCol = tokenEnd();
     endLine = tokenLine();
     next();
@@ -1240,35 +1267,79 @@ template <class TreeBuilder> TreeStatement JSParser::parseFunctionDeclaration(Tr
     return context.createFuncDeclStatement(name, body, parameters, openBracePos, closeBracePos, bodyStartLine, m_lastLine);
 }
 
+struct LabelInfo {
+    LabelInfo(const Identifier* ident, int start, int end)
+        : m_ident(ident)
+        , m_start(start)
+        , m_end(end)
+    {
+    }
+
+    const Identifier* m_ident;
+    int m_start;
+    int m_end;
+};
+
 template <class TreeBuilder> TreeStatement JSParser::parseExpressionOrLabelStatement(TreeBuilder& context)
 {
 
-    /* Expression and Label statements are ambiguous at LL(1), to avoid
-     * the cost of having a token buffer to support LL(2) we simply assume
-     * we have an expression statement, and then only look for a label if that
-     * parse fails.
+    /* Expression and Label statements are ambiguous at LL(1), so we have a
+     * special case that looks for a colon as the next character in the input.
      */
-    int start = tokenStart();
-    int startLine = tokenLine();
-    const Identifier* ident = m_token.m_data.ident;
-    int currentToken = m_tokenCount;
-    TreeExpression expression = parseExpression(context);
-    failIfFalse(expression);
-    if (autoSemiColon())
-        return context.createExprStatement(expression, startLine, m_lastLine);
-    failIfFalse(currentToken + 1 == m_tokenCount);
-    int end = tokenEnd();
-    consumeOrFail(COLON);
+    Vector<LabelInfo> labels;
+
+    do {
+        int start = tokenStart();
+        int startLine = tokenLine();
+        if (!nextTokenIsColon()) {
+            // If we hit this path we're making a expression statement, which
+            // by definition can't make use of continue/break so we can just
+            // ignore any labels we might have accumulated.
+            TreeExpression expression = parseExpression(context);
+            failIfFalse(expression);
+            failIfFalse(autoSemiColon());
+            return context.createExprStatement(expression, startLine, m_lastLine);
+        }
+        const Identifier* ident = m_token.m_data.ident;
+        int end = tokenEnd();
+        next();
+        consumeOrFail(COLON);
+        if (!m_syntaxAlreadyValidated) {
+            // This is O(N^2) over the current list of consecutive labels, but I
+            // have never seen more than one label in a row in the real world.
+            for (size_t i = 0; i < labels.size(); i++)
+                failIfTrue(ident == labels[i].m_ident);
+            failIfTrue(getLabel(ident));
+            labels.append(LabelInfo(ident, start, end));
+        }
+    } while (match(IDENT));
+    bool isLoop = false;
+    switch (m_token.m_type) {
+    case FOR:
+    case WHILE:
+    case DO:
+        isLoop = true;
+        break;
+
+    default:
+        break;
+    }
     const Identifier* unused = 0;
     if (!m_syntaxAlreadyValidated) {
-        failIfTrue(hasLabel(ident));
-        pushLabel(ident);
+        for (size_t i = 0; i < labels.size(); i++)
+            pushLabel(labels[i].m_ident, isLoop);
     }
     TreeStatement statement = parseStatement(context, unused);
-    if (!m_syntaxAlreadyValidated)
-        popLabel();
+    if (!m_syntaxAlreadyValidated) {
+        for (size_t i = 0; i < labels.size(); i++)
+            popLabel();
+    }
     failIfFalse(statement);
-    return context.createLabelStatement(ident, statement, start, end);
+    for (size_t i = 0; i < labels.size(); i++) {
+        const LabelInfo& info = labels[labels.size() - i - 1];
+        statement = context.createLabelStatement(info.m_ident, statement, info.m_start, info.m_end);
+    }
+    return statement;
 }
 
 template <class TreeBuilder> TreeStatement JSParser::parseExpressionStatement(TreeBuilder& context)
index ff7079f..35d0906 100644 (file)
@@ -703,6 +703,15 @@ ALWAYS_INLINE bool Lexer::parseMultilineComment()
     }
 }
 
+bool Lexer::nextTokenIsColon()
+{
+    const UChar* code = m_code;
+    while (code < m_codeEnd && (isWhiteSpace(*code) || isLineTerminator(*code)))
+        code++;
+        
+    return code < m_codeEnd && *code == ':';
+}
+
 JSTokenType Lexer::lex(JSTokenData* lvalp, JSTokenInfo* llocp, LexType lexType, bool strictMode)
 {
     ASSERT(!m_error);
index 4d2513d..e72888f 100644 (file)
@@ -53,6 +53,7 @@ namespace JSC {
         // Functions for the parser itself.
         enum LexType { IdentifyReservedWords, IgnoreReservedWords };
         JSTokenType lex(JSTokenData* lvalp, JSTokenInfo* llocp, LexType, bool strictMode);
+        bool nextTokenIsColon();
         int lineNumber() const { return m_lineNumber; }
         void setLastLineNumber(int lastLineNumber) { m_lastLineNumber = lastLineNumber; }
         int lastLineNumber() const { return m_lastLineNumber; }
@@ -67,6 +68,7 @@ namespace JSC {
         int currentOffset() { return m_code - m_codeStart; }
         void setOffset(int offset)
         {
+            m_error = 0;
             m_code = m_codeStart + offset;
             m_current = *m_code;
         }