Changeset 202828 in webkit

Timestamp:

Jul 5, 2016 1:05:02 PM (8 years ago)

Author:

sbarati@apple.com

Message:

our parsing for "use strict" is wrong when we first parse other directives that are not "use strict" but are located in a place where "use strict" would be valid
​https://bugs.webkit.org/show_bug.cgi?id=159376
<rdar://problem/27108773>

Reviewed by Benjamin Poulain.

Our strict mode detection algorithm used to break if we ever saw a directive
that is not "use strict" but is syntactically located in a location where our
parser looks for "use strict". It broke as follows:

If a function started with a non "use strict" string literal, we will allow
"use strict" to be in any arbitrary statement inside the top level block in
the function body. For example, this meant that if we parsed a sequence of string
literals, followed by arbitrary statements, followed by "use strict", we would parse
the function as if it's in strict mode. This is the wrong behavior with respect to
the spec. This has consequences in other ways that break invariants of the language.
For example, we used to allow functions that are lexically nested inside what we deemed
a strict function to be non-strict. This used to fire an assertion if we ever skipped over
that function using the source provider cache, but it worked just fine in release builds.

This patch fixes this bug.

• parser/Parser.cpp:

(JSC::Parser<LexerType>::parseSourceElements):
(JSC::Parser<LexerType>::parseStatement):

• tests/stress/ensure-proper-strict-mode-parsing.js: Added.

(foo.bar):
(foo):
(bar.foo):
(bar):
(bar.call.undefined.this.throw.new.Error.string_appeared_here.baz):
(baz.call.undefined.undefined.throw.new.Error.string_appeared_here.jaz):
(jaz.call.undefined.this.throw.new.Error.string_appeared_here.vaz):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• parser/Parser.cpp (modified) (4 diffs)
• tests/stress/ensure-proper-strict-mode-parsing.js (added)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2016-07-05  Saam Barati  <sbarati@apple.com>
+
+        our parsing for "use strict" is wrong when we first parse other directives that are not "use strict" but are located in a place where "use strict" would be valid
+        https://bugs.webkit.org/show_bug.cgi?id=159376
+        <rdar://problem/27108773>
+
+        Reviewed by Benjamin Poulain.
+
+        Our strict mode detection algorithm used to break if we ever saw a directive
+        that is not "use strict" but is syntactically located in a location where our
+        parser looks for "use strict". It broke as follows:
+
+        If a function started with a non "use strict" string literal, we will allow
+        "use strict" to be in any arbitrary statement inside the top level block in
+        the function body. For example, this meant that if we parsed a sequence of string
+        literals, followed by arbitrary statements, followed by "use strict", we would parse
+        the function as if it's in strict mode. This is the wrong behavior with respect to
+        the spec. This has consequences in other ways that break invariants of the language.
+        For example, we used to allow functions that are lexically nested inside what we deemed
+        a strict function to be non-strict. This used to fire an assertion if we ever skipped over
+        that function using the source provider cache, but it worked just fine in release builds.
+
+        This patch fixes this bug.
+
+        * parser/Parser.cpp:
+        (JSC::Parser<LexerType>::parseSourceElements):
+        (JSC::Parser<LexerType>::parseStatement):
+        * tests/stress/ensure-proper-strict-mode-parsing.js: Added.
+        (foo.bar):
+        (foo):
+        (bar.foo):
+        (bar):
+        (bar.call.undefined.this.throw.new.Error.string_appeared_here.baz):
+        (baz.call.undefined.undefined.throw.new.Error.string_appeared_here.jaz):
+        (jaz.call.undefined.this.throw.new.Error.string_appeared_here.vaz):
+
 2016-07-05  Saam Barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/parser/Parser.cpp

@@ -398 +398 @@
     const unsigned lengthOfUseStrictLiteral = 12; // "use strict".length
     TreeSourceElements sourceElements = context.createSourceElements();
-    bool seenNonDirective = false;
     const Identifier* directive = 0;
     unsigned directiveLiteralLength = 0;
     auto savePoint = createSavePoint();
-    bool hasSetStrict = false;
+    bool shouldCheckForUseStrict = mode == CheckForStrictMode;
     
     while (TreeStatement statement = parseStatementListItem(context, directive, &directiveLiteralLength)) {
-        if (mode == CheckForStrictMode && !seenNonDirective) {
+        if (shouldCheckForUseStrict) {
             if (directive) {
                 // "use strict" must be the exact literal without escape sequences or line continuation.
-                if (!hasSetStrict && directiveLiteralLength == lengthOfUseStrictLiteral && m_vm->propertyNames->useStrictIdentifier == *directive) {
+                if (directiveLiteralLength == lengthOfUseStrictLiteral && m_vm->propertyNames->useStrictIdentifier == *directive) {
                     setStrictMode();
-                    hasSetStrict = true;
+                    shouldCheckForUseStrict = false; // We saw "use strict", there is no need to keep checking for it.
                     if (!isValidStrictMode()) {
                         if (m_parserState.lastFunctionName) {
@@ -429 +428 @@
                     continue;
                 }
-            } else
-                seenNonDirective = true;
+
+                // We saw a directive, but it wasn't "use strict". We reset our state to
+                // see if the next statement we parse is also a directive.
+                directive = nullptr;
+            } else {
+                // We saw a statement that wasn't in the form of a directive. The spec says that "use strict"
+                // is only allowed as the first statement, or after a sequence of directives before it, but
+                // not after non-directive statements.
+                shouldCheckForUseStrict = false;
+            }
         }
         context.appendStatement(sourceElements, statement);
@@ -1602 +1609 @@
     DepthManager statementDepth(&m_statementDepth);
     m_statementDepth++;
-    directive = 0;
     int nonTrivialExpressionCount = 0;
     failIfStackOverflow();
@@ -1721 +1727 @@
         TreeStatement exprStatement = parseExpressionStatement(context);
         if (directive && nonTrivialExpressionCount != m_parserState.nonTrivialExpressionCount)
-            directive = 0;
+            directive = nullptr;
         result = exprStatement;
         break;