Function constructor needs to follow the spec and validate parameters and body indepe...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 27 Jun 2017 21:37:46 +0000 (21:37 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 27 Jun 2017 21:37:46 +0000 (21:37 +0000)
https://bugs.webkit.org/show_bug.cgi?id=173303
<rdar://problem/32732526>

Reviewed by Keith Miller.

JSTests:

* ChakraCore/test/Function/FuncBodyES5.baseline-jsc:
* stress/function-constructor-semantics.js: Added.
(assert):
(hasSyntaxError):
(foo):
(async.foo):
(testError):
(testOK.toString):
(toString):

LayoutTests/imported/w3c:

* web-platform-tests/html/webappapis/scripting/events/inline-event-handler-ordering-expected.txt:
* web-platform-tests/html/webappapis/scripting/events/invalid-uncompiled-raw-handler-compiled-late-expected.txt:
* web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-attribute-expected.txt:
* web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-body-onerror-expected.txt:

Source/JavaScriptCore:

The Function constructor must check the arguments and body strings
independently for syntax errors. People rely on this specified behavior
to verify that a particular string is a valid function body. We used
to check these things strings concatenated together, instead of
independently. For example, this used to be valid: `Function("/*", "*/){")`.
However, we should throw a syntax error here since "(/*)" is not a valid
parameter list, and "*/){" is not a valid body.

To implement the specified behavior, we check the syntax independently of
both the body and the parameter list. To check that the parameter list has
valid syntax, we check that it is valid if in a function with an empty body.
To check that the body has valid syntax, we check it is valid in a function
with an empty parameter list.

* runtime/FunctionConstructor.cpp:
(JSC::constructFunctionSkippingEvalEnabledCheck):

LayoutTests:

* fast/dom/attribute-event-listener-errors-expected.txt:
* fast/events/attribute-listener-deletion-crash-expected.txt:
* fast/events/window-onerror-syntax-error-in-attr-expected.txt:
* js/dom/invalid-syntax-for-function-expected.txt:

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

15 files changed:
JSTests/ChakraCore/test/Function/FuncBodyES5.baseline-jsc
JSTests/ChangeLog
JSTests/stress/function-constructor-semantics.js [new file with mode: 0644]
LayoutTests/ChangeLog
LayoutTests/fast/dom/attribute-event-listener-errors-expected.txt
LayoutTests/fast/events/attribute-listener-deletion-crash-expected.txt
LayoutTests/fast/events/window-onerror-syntax-error-in-attr-expected.txt
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/html/webappapis/scripting/events/inline-event-handler-ordering-expected.txt
LayoutTests/imported/w3c/web-platform-tests/html/webappapis/scripting/events/invalid-uncompiled-raw-handler-compiled-late-expected.txt
LayoutTests/imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-attribute-expected.txt
LayoutTests/imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-body-onerror-expected.txt
LayoutTests/js/dom/invalid-syntax-for-function-expected.txt
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/FunctionConstructor.cpp

index c7ca6b6..36f2a20 100644 (file)
@@ -9,13 +9,13 @@ PASS: 7: new Function succeeded as expected
 PASS: 8: new Function succeeded as expected
 PASS: 9: new Function succeeded as expected
 PASS: 10: new Function succeeded as expected
-PASS: 100: new Function failed as expected. SyntaxError: Parser error
-PASS: 100: new Function failed as expected. SyntaxError: Parser error
-PASS: 101: new Function failed as expected. SyntaxError: Parser error
-PASS: 102: new Function failed as expected. SyntaxError: Parser error
-PASS: 103: new Function failed as expected. SyntaxError: Parser error
-PASS: 104: new Function failed as expected. SyntaxError: Invalid character: '\0'
-PASS: 105: new Function failed as expected. SyntaxError: Parser error
+PASS: 100: new Function failed as expected. SyntaxError: Unexpected token '{'. Expected ')' to end a compound expression.
+PASS: 100: new Function failed as expected. SyntaxError: Unexpected token '{'. Expected ')' to end a compound expression.
+PASS: 101: new Function failed as expected. SyntaxError: Unexpected keyword 'function'. Expected ')' to end a compound expression.
+PASS: 102: new Function failed as expected. SyntaxError: Unexpected keyword 'function'. Expected ')' to end a compound expression.
+PASS: 103: new Function failed as expected. SyntaxError: Unexpected keyword 'function'. Expected ')' to end a compound expression.
+PASS: 104: new Function failed as expected. SyntaxError: Unexpected token ';'. Expected ')' to end a compound expression.
+PASS: 105: new Function failed as expected. SyntaxError: Unexpected token ';'. Expected ')' to end a compound expression.
 PASS: 200: new Function failed as expected. SyntaxError: Unexpected token ','. Expected a parameter pattern or a ')' in parameter list.
 PASS: 200: new Function failed as expected. SyntaxError: Unexpected token ','. Expected a parameter pattern or a ')' in parameter list.
 PASS: 201: new Function failed as expected. SyntaxError: Unexpected token ','. Expected a parameter pattern or a ')' in parameter list.
index 7482fab..1cea644 100644 (file)
@@ -1,3 +1,21 @@
+2017-06-27  Saam Barati  <sbarati@apple.com>
+
+        Function constructor needs to follow the spec and validate parameters and body independently
+        https://bugs.webkit.org/show_bug.cgi?id=173303
+        <rdar://problem/32732526>
+
+        Reviewed by Keith Miller.
+
+        * ChakraCore/test/Function/FuncBodyES5.baseline-jsc:
+        * stress/function-constructor-semantics.js: Added.
+        (assert):
+        (hasSyntaxError):
+        (foo):
+        (async.foo):
+        (testError):
+        (testOK.toString):
+        (toString):
+
 2017-06-26  Saam Barati  <sbarati@apple.com>
 
         RegExpPrototype.js builtin uses for-of iteration which is almost certainly incorrect
diff --git a/JSTests/stress/function-constructor-semantics.js b/JSTests/stress/function-constructor-semantics.js
new file mode 100644 (file)
index 0000000..8b35be4
--- /dev/null
@@ -0,0 +1,64 @@
+function assert(b) {
+    if (!b)
+        throw new Error("Bad");
+}
+
+function hasSyntaxError(f) {
+    let threw = false;
+    try {
+        f();
+    } catch(e) {
+        threw = e instanceof SyntaxError;
+    }
+    return threw;
+}
+
+let functions = [
+    Function,
+    (function*foo(){}).__proto__.constructor,
+    (async function foo(){}).__proto__.constructor,
+];
+
+function testError(...args) {
+    for (let f of functions) {
+        assert(hasSyntaxError(() => (f(...args))));
+    }
+}
+
+function testOK(...args) {
+    for (let f of functions) {
+        assert(!hasSyntaxError(() => (f(...args))));
+    }
+}
+
+testError("a", "b", "/*", "");
+testError("/*", "*/){");
+testError("a=super()", "body;");
+testError("a=super.foo", "body;");
+testError("super();");
+testError("super.foo;");
+testError("a", "b", "/*", "");
+testError("a", "'use strict'; let a;");
+testError("/*", "*/");
+testError("/*", "*/");
+testError("a=20", "'use strict';");
+testError("{a}", "'use strict';");
+testError("...args", "'use strict';");
+testError("...args", "b", "");
+testError("//", "b", "");
+
+testOK("/*", "*/", "");
+testOK("a", "/*b", "*/", "'use strict'; let b");
+testOK("{a}", "return a;");
+testOK("a", "...args", "");
+testOK("");
+testOK("let a");
+testOK(undefined);
+testOK("//");
+
+let str = "";
+testOK({toString() { str += "1"; return "a"}}, {toString() { str += "2"; return "b"}}, {toString() { str += "3"; return "body;"}});
+let target = "";
+for (let i = 0; i < functions.length; ++i)
+    target += "123";
+assert(str === target);
index 6a68fcc..5e23013 100644 (file)
@@ -1,3 +1,16 @@
+2017-06-27  Saam Barati  <sbarati@apple.com>
+
+        Function constructor needs to follow the spec and validate parameters and body independently
+        https://bugs.webkit.org/show_bug.cgi?id=173303
+        <rdar://problem/32732526>
+
+        Reviewed by Keith Miller.
+
+        * fast/dom/attribute-event-listener-errors-expected.txt:
+        * fast/events/attribute-listener-deletion-crash-expected.txt:
+        * fast/events/window-onerror-syntax-error-in-attr-expected.txt:
+        * js/dom/invalid-syntax-for-function-expected.txt:
+
 2017-06-27  John Wilander  <wilander@apple.com>
 
         Resource Load Statistics: Add telemetry
index 4236767..1df9397 100644 (file)
@@ -1,4 +1,4 @@
 CONSOLE MESSAGE: line 4: ReferenceError: Can't find variable: error
-CONSOLE MESSAGE: line 5: SyntaxError: Invalid character: '@'
+CONSOLE MESSAGE: line 9: SyntaxError: Invalid character: '@'
 This test verifies that an attribute event listener error shows the right line number even if the attribute contains newlines.
   
index 5824baa..0db7728 100644 (file)
@@ -1,21 +1,21 @@
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 2: SyntaxError: Unexpected token '|'
 PASS
index 68edd01..5ac9501 100644 (file)
@@ -1,6 +1,6 @@
 Test that window.onerror is called on window object when there is a syntax error in attribute handler. Bug 70991.
 
-Main frame window.onerror: SyntaxError: Unexpected token '%' at window-onerror-syntax-error-in-attr.html:10:38 SyntaxError: Unexpected token '%'
-Main frame window.onerror: SyntaxError: Unexpected token '%' at window-onerror-syntax-error-in-attr.html:36:38 SyntaxError: Unexpected token '%'
-Main frame window.onerror: SyntaxError: Unexpected token '%' at window-onerror-syntax-error-in-attr.html:36:14 SyntaxError: Unexpected token '%'
+Main frame window.onerror: SyntaxError: Unexpected token '%' at window-onerror-syntax-error-in-attr.html:11:38 SyntaxError: Unexpected token '%'
+Main frame window.onerror: SyntaxError: Unexpected token '%' at window-onerror-syntax-error-in-attr.html:37:38 SyntaxError: Unexpected token '%'
+Main frame window.onerror: SyntaxError: Unexpected token '%' at window-onerror-syntax-error-in-attr.html:37:14 SyntaxError: Unexpected token '%'
 Button 1 Button 2 Button 3
index 1e69013..0366903 100644 (file)
@@ -1,3 +1,16 @@
+2017-06-27  Saam Barati  <sbarati@apple.com>
+
+        Function constructor needs to follow the spec and validate parameters and body independently
+        https://bugs.webkit.org/show_bug.cgi?id=173303
+        <rdar://problem/32732526>
+
+        Reviewed by Keith Miller.
+
+        * web-platform-tests/html/webappapis/scripting/events/inline-event-handler-ordering-expected.txt:
+        * web-platform-tests/html/webappapis/scripting/events/invalid-uncompiled-raw-handler-compiled-late-expected.txt:
+        * web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-attribute-expected.txt:
+        * web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-body-onerror-expected.txt:
+
 2017-06-27  Frederic Wang  <fwang@igalia.com>
 
         Some tests to verify forbidden frame navigation time out
index 43095a3..6199cde 100644 (file)
@@ -1,5 +1,5 @@
 CONSOLE MESSAGE: line 19: SyntaxError: Unexpected token '}'
-CONSOLE MESSAGE: line 52: SyntaxError: Unexpected token '}'
+CONSOLE MESSAGE: line 54: SyntaxError: Unexpected token '}'
 
 PASS Inline event handlers retain their ordering when invalid and force-compiled 
 PASS Inline event handlers retain their ordering when invalid and force-compiled via dispatch 
index 9ad5ac6..a86e064 100644 (file)
@@ -1,5 +1,5 @@
-CONSOLE MESSAGE: line 24: SyntaxError: Parser error
-CONSOLE MESSAGE: line 21: SyntaxError: Parser error
+CONSOLE MESSAGE: line 26: SyntaxError: Unexpected token '}'. Expected ')' to end a compound expression.
+CONSOLE MESSAGE: line 21: SyntaxError: Unexpected token '}'. Expected ')' to end a compound expression.
 
 FAIL Invalid uncompiled raw handlers should only be compiled when about to call them. assert_array_equals: lengths differ, expected 3 got 4
 
index f50fce7..329a417 100644 (file)
@@ -1,3 +1,3 @@
-CONSOLE MESSAGE: line 1: SyntaxError: Invalid character: '#'
+CONSOLE MESSAGE: line 2: SyntaxError: Invalid character: '#'
 This test ensures we don't crash when we are given garbage for an attribute expecting a function.
 https://bugs.webkit.org/show_bug.cgi?id=19025
index 58d7a1d..f25dcf1 100644 (file)
@@ -1,3 +1,28 @@
+2017-06-27  Saam Barati  <sbarati@apple.com>
+
+        Function constructor needs to follow the spec and validate parameters and body independently
+        https://bugs.webkit.org/show_bug.cgi?id=173303
+        <rdar://problem/32732526>
+
+        Reviewed by Keith Miller.
+
+        The Function constructor must check the arguments and body strings
+        independently for syntax errors. People rely on this specified behavior
+        to verify that a particular string is a valid function body. We used
+        to check these things strings concatenated together, instead of
+        independently. For example, this used to be valid: `Function("/*", "*/){")`.
+        However, we should throw a syntax error here since "(/*)" is not a valid
+        parameter list, and "*/){" is not a valid body.
+        
+        To implement the specified behavior, we check the syntax independently of
+        both the body and the parameter list. To check that the parameter list has
+        valid syntax, we check that it is valid if in a function with an empty body.
+        To check that the body has valid syntax, we check it is valid in a function
+        with an empty parameter list.
+
+        * runtime/FunctionConstructor.cpp:
+        (JSC::constructFunctionSkippingEvalEnabledCheck):
+
 2017-06-27  Ting-Wei Lan  <lantw44@gmail.com>
 
         Add missing includes to fix compilation error on FreeBSD
index 0fb8b78..5189d57 100644 (file)
@@ -21,6 +21,7 @@
 #include "config.h"
 #include "FunctionConstructor.h"
 
+#include "Completion.h"
 #include "ExceptionHelpers.h"
 #include "FunctionPrototype.h"
 #include "JSAsyncFunction.h"
@@ -98,45 +99,79 @@ JSObject* constructFunctionSkippingEvalEnabledCheck(
     switch (functionConstructionMode) {
     case FunctionConstructionMode::Function:
         structure = globalObject->functionStructure();
-        prefix = "{function ";
+        prefix = "function ";
         break;
     case FunctionConstructionMode::Generator:
         structure = globalObject->generatorFunctionStructure();
-        prefix = "{function *";
+        prefix = "function *";
         break;
     case FunctionConstructionMode::Async:
         structure = globalObject->asyncFunctionStructure();
-        prefix = "{async function ";
+        prefix = "async function ";
         break;
     }
 
+    auto checkBody = [&] (const String& body) {
+        // The spec mandates that the body parses a valid function body independent
+        // of the parameters.
+        String program = makeString("(", prefix, "(){\n", body, "\n})");
+        SourceCode source = makeSource(program, sourceOrigin, sourceURL, position);
+        JSValue exception;
+        checkSyntax(exec, source, &exception);
+        if (exception) {
+            scope.throwException(exec, exception);
+            return;
+        }
+    };
+
     // How we stringify functions is sometimes important for web compatibility.
     // See https://bugs.webkit.org/show_bug.cgi?id=24350.
     String program;
     if (args.isEmpty())
-        program = makeString(prefix, functionName.string(), "() {\n\n}}");
+        program = makeString("{", prefix, functionName.string(), "() {\n\n}}");
     else if (args.size() == 1) {
         auto body = args.at(0).toWTFString(exec);
         RETURN_IF_EXCEPTION(scope, nullptr);
-        program = makeString(prefix, functionName.string(), "() {\n", body, "\n}}");
+        checkBody(body);
+        RETURN_IF_EXCEPTION(scope, nullptr);
+        program = makeString("{", prefix, functionName.string(), "() {\n", body, "\n}}");
     } else {
         StringBuilder builder;
+        builder.append("{");
         builder.append(prefix);
         builder.append(functionName.string());
         builder.append('(');
+        StringBuilder parameterBuilder;
         auto viewWithString = args.at(0).toString(exec)->viewWithUnderlyingString(exec);
         RETURN_IF_EXCEPTION(scope, nullptr);
-        builder.append(viewWithString.view);
+        parameterBuilder.append(viewWithString.view);
         for (size_t i = 1; i < args.size() - 1; i++) {
-            builder.appendLiteral(", ");
+            parameterBuilder.appendLiteral(", ");
             auto viewWithString = args.at(i).toString(exec)->viewWithUnderlyingString(exec);
             RETURN_IF_EXCEPTION(scope, nullptr);
-            builder.append(viewWithString.view);
+            parameterBuilder.append(viewWithString.view);
         }
+
+        {
+            // The spec mandates that the parameters parse as a valid parameter list
+            // independent of the function body.
+            String program = makeString("(", prefix, "(", parameterBuilder.toString(), "){\n\n})");
+            SourceCode source = makeSource(program, sourceOrigin, sourceURL, position);
+            JSValue exception;
+            checkSyntax(exec, source, &exception);
+            if (exception) {
+                scope.throwException(exec, exception);
+                return nullptr;
+            }
+        }
+
+        builder.append(parameterBuilder);
         builder.appendLiteral(") {\n");
-        viewWithString = args.at(args.size() - 1).toString(exec)->viewWithUnderlyingString(exec);
+        auto body = args.at(args.size() - 1).toWTFString(exec);
+        RETURN_IF_EXCEPTION(scope, nullptr);
+        checkBody(body);
         RETURN_IF_EXCEPTION(scope, nullptr);
-        builder.append(viewWithString.view);
+        builder.append(body);
         builder.appendLiteral("\n}}");
         program = builder.toString();
     }