Title: [202828] trunk/Source/_javascript_Core
Revision
202828
Author
sbar...@apple.com
Date
2016-07-05 13:05:02 -0700 (Tue, 05 Jul 2016)

Log 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):

Modified Paths

Added Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (202827 => 202828)


--- trunk/Source/_javascript_Core/ChangeLog	2016-07-05 19:49:19 UTC (rev 202827)
+++ trunk/Source/_javascript_Core/ChangeLog	2016-07-05 20:05:02 UTC (rev 202828)
@@ -1,5 +1,41 @@
 2016-07-05  Saam Barati  <sbar...@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  <sbar...@apple.com>
+
         reportAbandonedObjectGraph should report abandoned bytes based on capacity() so it works even if a GC has never happened
         https://bugs.webkit.org/show_bug.cgi?id=159222
         <rdar://problem/27001991>

Modified: trunk/Source/_javascript_Core/parser/Parser.cpp (202827 => 202828)


--- trunk/Source/_javascript_Core/parser/Parser.cpp	2016-07-05 19:49:19 UTC (rev 202827)
+++ trunk/Source/_javascript_Core/parser/Parser.cpp	2016-07-05 20:05:02 UTC (rev 202828)
@@ -397,19 +397,18 @@
 {
     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) {
                             if (m_vm->propertyNames->arguments == *m_parserState.lastFunctionName)
@@ -428,8 +427,16 @@
                     propagateError();
                     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);
     }
@@ -1601,7 +1608,6 @@
 {
     DepthManager statementDepth(&m_statementDepth);
     m_statementDepth++;
-    directive = 0;
     int nonTrivialExpressionCount = 0;
     failIfStackOverflow();
     TreeStatement result = 0;
@@ -1720,7 +1726,7 @@
     default:
         TreeStatement exprStatement = parseExpressionStatement(context);
         if (directive && nonTrivialExpressionCount != m_parserState.nonTrivialExpressionCount)
-            directive = 0;
+            directive = nullptr;
         result = exprStatement;
         break;
     }

Added: trunk/Source/_javascript_Core/tests/stress/ensure-proper-strict-mode-parsing.js (0 => 202828)


--- trunk/Source/_javascript_Core/tests/stress/ensure-proper-strict-mode-parsing.js	                        (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/ensure-proper-strict-mode-parsing.js	2016-07-05 20:05:02 UTC (rev 202828)
@@ -0,0 +1,90 @@
+
+function foo() {
+    "hello world i'm not use strict.";
+    function bar() {
+        return 25;
+    }
+    bar();
+    "use strict";
+    return this;
+}
+if (foo.call(undefined) !== this)
+    throw new Error("Bad parsing strict mode.");
+
+function bar() {
+    "hello world i'm not use strict.";
+    function foo() {
+        return this;
+    }
+    "use strict";
+    return foo.call(undefined);
+}
+if (bar.call(undefined) !== this)
+    throw new Error("Bad parsing strict mode.")
+
+function baz() {
+    "foo";
+    "bar";
+    "baz";
+    "foo";
+    "bar";
+    "baz";
+    "foo";
+    "bar";
+    "baz";
+    "use strict";
+    return this;
+}
+if (baz.call(undefined) !== undefined)
+    throw new Error("Bad parsing strict mode.")
+
+function jaz() {
+    "foo";
+    `bar`;
+    "use strict";
+    return this;
+}
+if (jaz.call(undefined) !== this)
+    throw new Error("Bad parsing strict mode.")
+
+function vaz() {
+    "foo";
+    "use strict";
+    `bar`;
+    return this;
+}
+if (vaz.call(undefined) !== undefined)
+    throw new Error("Bad parsing strict mode.")
+
+function hello() {
+    "foo";
+    2 + 2
+    "use strict";
+    return this;
+}
+if (hello.call(undefined) !== this)
+    throw new Error("Bad parsing strict mode.")
+
+function world() {
+    "foo";
+    let x;
+    "use strict";
+    return this;
+}
+if (world.call(undefined) !== this)
+    throw new Error("Bad parsing strict mode.")
+
+function a() {
+    "foo";
+    world;
+    "use strict";
+    return this;
+}
+if (a.call(undefined) !== this)
+    throw new Error("Bad parsing strict mode.")
+
+if (eval("'foo'; 'use strict'; 'bar';") !== "bar")
+    throw new Error("Bad parsing strict mode.");
+
+if (eval("'foo'; 'use strict'; 'bar'; this;") !== this)
+    throw new Error("Bad parsing strict mode.");
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to