From 4b0395cc23dd052552ea79b6a254ac5caea574a2 Mon Sep 17 00:00:00 2001 From: "mstarzinger@chromium.org" Date: Thu, 7 Mar 2013 15:46:14 +0000 Subject: [PATCH] Harden Function()'s parsing of function literals. R=rossberg@chromium.org BUG=v8:2470 TEST=mjsunit/regress/regress-2470 Review URL: https://codereview.chromium.org/12613007 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@13867 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/compiler.cc | 2 ++ src/compiler.h | 23 ++++++++++++++++---- src/parser.cc | 10 +++++++++ src/runtime.cc | 10 +++++++-- src/runtime.h | 2 +- src/v8natives.js | 10 ++++++--- test/mjsunit/new-function.js | 4 ++-- test/mjsunit/regress/regress-2470.js | 42 ++++++++++++++++++++++++++++++++++++ 8 files changed, 91 insertions(+), 12 deletions(-) create mode 100644 test/mjsunit/regress/regress-2470.js diff --git a/src/compiler.cc b/src/compiler.cc index 100d100..c4ef8eb 100644 --- a/src/compiler.cc +++ b/src/compiler.cc @@ -681,6 +681,7 @@ Handle Compiler::CompileEval(Handle source, Handle context, bool is_global, LanguageMode language_mode, + ParseRestriction restriction, int scope_position) { Isolate* isolate = source->GetIsolate(); int source_length = source->length(); @@ -707,6 +708,7 @@ Handle Compiler::CompileEval(Handle source, info.MarkAsEval(); if (is_global) info.MarkAsGlobal(); info.SetLanguageMode(language_mode); + info.SetParseRestriction(restriction); info.SetContext(context); result = MakeFunctionInfo(&info); if (!result.is_null()) { diff --git a/src/compiler.h b/src/compiler.h index 0191bb2..5e69661 100644 --- a/src/compiler.h +++ b/src/compiler.h @@ -40,6 +40,13 @@ static const int kPrologueOffsetNotSet = -1; class ScriptDataImpl; class HydrogenCodeStub; +// ParseRestriction is used to restrict the set of valid statements in a +// unit of compilation. Restriction violations cause a syntax error. +enum ParseRestriction { + NO_PARSE_RESTRICTION, // All expressions are allowed. + ONLY_SINGLE_FUNCTION_LITERAL // Only a single FunctionLiteral expression. +}; + // CompilationInfo encapsulates some information known at compile time. It // is constructed based on the resources available at compile-time. class CompilationInfo { @@ -55,9 +62,7 @@ class CompilationInfo { ASSERT(Isolate::Current() == isolate_); return isolate_; } - Zone* zone() { - return zone_; - } + Zone* zone() { return zone_; } bool is_lazy() const { return IsLazy::decode(flags_); } bool is_eval() const { return IsEval::decode(flags_); } bool is_global() const { return IsGlobal::decode(flags_); } @@ -138,6 +143,14 @@ class CompilationInfo { return SavesCallerDoubles::decode(flags_); } + void SetParseRestriction(ParseRestriction restriction) { + flags_ = ParseRestricitonField::update(flags_, restriction); + } + + ParseRestriction parse_restriction() const { + return ParseRestricitonField::decode(flags_); + } + void SetFunction(FunctionLiteral* literal) { ASSERT(function_ == NULL); function_ = literal; @@ -285,7 +298,8 @@ class CompilationInfo { class IsNonDeferredCalling: public BitField {}; // If the compiled code saves double caller registers that it clobbers. class SavesCallerDoubles: public BitField {}; - + // If the set of valid statements is restricted. + class ParseRestricitonField: public BitField {}; unsigned flags_; @@ -502,6 +516,7 @@ class Compiler : public AllStatic { Handle context, bool is_global, LanguageMode language_mode, + ParseRestriction restriction, int scope_position); // Compile from function info (used for lazy compilation). Returns true on diff --git a/src/parser.cc b/src/parser.cc index 62b633e..2b2cf65 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -656,6 +656,16 @@ FunctionLiteral* Parser::DoParseProgram(CompilationInfo* info, CheckConflictingVarDeclarations(top_scope_, &ok); } + if (ok && info->parse_restriction() == ONLY_SINGLE_FUNCTION_LITERAL) { + if (body->length() != 1 || + !body->at(0)->IsExpressionStatement() || + !body->at(0)->AsExpressionStatement()-> + expression()->IsFunctionLiteral()) { + ReportMessage("unable_to_parse", Vector::empty()); + ok = false; + } + } + if (ok) { result = factory()->NewFunctionLiteral( no_name, diff --git a/src/runtime.cc b/src/runtime.cc index 5bbca4d..f88651a 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -8957,8 +8957,9 @@ bool CodeGenerationFromStringsAllowed(Isolate* isolate, RUNTIME_FUNCTION(MaybeObject*, Runtime_CompileString) { HandleScope scope(isolate); - ASSERT_EQ(1, args.length()); + ASSERT_EQ(2, args.length()); CONVERT_ARG_HANDLE_CHECKED(String, source, 0); + CONVERT_BOOLEAN_ARG_CHECKED(function_literal_only, 1); // Extract native context. Handle context(isolate->context()->native_context()); @@ -8974,8 +8975,10 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_CompileString) { } // Compile source string in the native context. + ParseRestriction restriction = function_literal_only + ? ONLY_SINGLE_FUNCTION_LITERAL : NO_PARSE_RESTRICTION; Handle shared = Compiler::CompileEval( - source, context, true, CLASSIC_MODE, RelocInfo::kNoPosition); + source, context, true, CLASSIC_MODE, restriction, RelocInfo::kNoPosition); if (shared.is_null()) return Failure::Exception(); Handle fun = isolate->factory()->NewFunctionFromSharedFunctionInfo(shared, @@ -9011,6 +9014,7 @@ static ObjectPair CompileGlobalEval(Isolate* isolate, Handle(isolate->context()), context->IsNativeContext(), language_mode, + NO_PARSE_RESTRICTION, scope_position); if (shared.is_null()) return MakePair(Failure::Exception(), NULL); Handle compiled = @@ -12003,6 +12007,7 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_DebugEvaluate) { context, context->IsNativeContext(), CLASSIC_MODE, + NO_PARSE_RESTRICTION, RelocInfo::kNoPosition); if (shared.is_null()) return Failure::Exception(); Handle compiled_function = @@ -12109,6 +12114,7 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_DebugEvaluateGlobal) { context, is_global, CLASSIC_MODE, + NO_PARSE_RESTRICTION, RelocInfo::kNoPosition); if (shared.is_null()) return Failure::Exception(); Handle compiled_function = diff --git a/src/runtime.h b/src/runtime.h index cd6805c..ac4a207 100644 --- a/src/runtime.h +++ b/src/runtime.h @@ -265,7 +265,7 @@ namespace internal { /* Numbers */ \ \ /* Globals */ \ - F(CompileString, 1, 1) \ + F(CompileString, 2, 1) \ F(GlobalPrint, 1, 1) \ \ /* Eval */ \ diff --git a/src/v8natives.js b/src/v8natives.js index f295c3a..826ae43 100644 --- a/src/v8natives.js +++ b/src/v8natives.js @@ -174,7 +174,7 @@ function GlobalEval(x) { 'be the global object from which eval originated'); } - var f = %CompileString(x); + var f = %CompileString(x, false); if (!IS_FUNCTION(f)) return f; return %_CallFunction(global_receiver, f); @@ -1704,14 +1704,18 @@ function NewFunction(arg1) { // length == 1 // character - it may make the combined function expression // compile. We avoid this problem by checking for this early on. if (p.indexOf(')') != -1) throw MakeSyntaxError('unable_to_parse',[]); + // If the formal parameters include an unbalanced block comment, the + // function must be rejected. Since JavaScript does not allow nested + // comments we can include a trailing block comment to catch this. + p += '\n/' + '**/'; } var body = (n > 0) ? ToString(%_Arguments(n - 1)) : ''; - var source = '(function(' + p + ') {\n' + body + '\n})'; + var source = '(function(\n' + p + '\n){\n' + body + '\n})'; // The call to SetNewFunctionAttributes will ensure the prototype // property of the resulting function is enumerable (ECMA262, 15.3.5.2). var global_receiver = %GlobalReceiver(global); - var f = %_CallFunction(global_receiver, %CompileString(source)); + var f = %_CallFunction(global_receiver, %CompileString(source, true)); %FunctionMarkNameShouldPrintAsAnonymous(f); return %SetNewFunctionAttributes(f); diff --git a/test/mjsunit/new-function.js b/test/mjsunit/new-function.js index 9e8cc27..794bd76 100644 --- a/test/mjsunit/new-function.js +++ b/test/mjsunit/new-function.js @@ -25,10 +25,10 @@ // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -var x; +var x = 0; try { Function("}), x = this, (function() {"); } catch(e) { print("Caught " + e); } -assertTrue(x == "[object global]"); +assertTrue(x === 0); diff --git a/test/mjsunit/regress/regress-2470.js b/test/mjsunit/regress/regress-2470.js new file mode 100644 index 0000000..eed3816 --- /dev/null +++ b/test/mjsunit/regress/regress-2470.js @@ -0,0 +1,42 @@ +// Copyright 2013 the V8 project authors. All rights reserved. +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following +// disclaimer in the documentation and/or other materials provided +// with the distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived +// from this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// Test whether the opening parenthesis can be eaten up by a comment. +assertThrows('Function("/*", "*/){");', SyntaxError); + +// Test whether the function literal can be closed prematurely. +assertThrows('Function("});(function(){");', SyntaxError); + +// Test whether block comments are handled correctly. +assertDoesNotThrow('Function("/*", "*/", "/**/");'); +assertDoesNotThrow('Function("/*", "a", "*/", "/**/");'); +assertThrows('Function("a", "/*", "*/", "/**/");', SyntaxError); + +// Test whether line comments are handled correctly. +assertDoesNotThrow('Function("//", "//")'); +assertDoesNotThrow('Function("//", "//", "//")'); +assertThrows('Function("a", "//", "//")', SyntaxError); -- 2.7.4