From d70f78827e5882e542a7aa6c67ab73e999ffadbc Mon Sep 17 00:00:00 2001 From: "marja@chromium.org" Date: Mon, 14 Apr 2014 07:35:46 +0000 Subject: [PATCH] Fail the compilation if the cached data is invalid. R=svenpanne@chromium.org BUG= Review URL: https://codereview.chromium.org/234953002 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@20703 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/api.cc | 38 +++++++++++++++++++++----------------- src/messages.js | 3 ++- src/parser.cc | 29 +++++++++++------------------ src/parser.h | 8 ++++---- test/cctest/test-api.cc | 48 +++++++++++++++++++++++++++++++++++++----------- 5 files changed, 75 insertions(+), 51 deletions(-) diff --git a/src/api.cc b/src/api.cc index d682078..bce5f67 100644 --- a/src/api.cc +++ b/src/api.cc @@ -1694,40 +1694,44 @@ Local ScriptCompiler::CompileUnbound( CompileOptions options) { i::ScriptData* script_data_impl = NULL; i::CachedDataMode cached_data_mode = i::NO_CACHED_DATA; + i::Isolate* isolate = reinterpret_cast(v8_isolate); + ON_BAILOUT(isolate, "v8::ScriptCompiler::CompileUnbound()", + return Local()); if (options & kProduceDataToCache) { cached_data_mode = i::PRODUCE_CACHED_DATA; ASSERT(source->cached_data == NULL); if (source->cached_data) { // Asked to produce cached data even though there is some already -> not - // good. In release mode, try to do the right thing: Just regenerate the - // data. - delete source->cached_data; - source->cached_data = NULL; + // good. Fail the compilation. + EXCEPTION_PREAMBLE(isolate); + i::Handle result = isolate->factory()->NewSyntaxError( + "invalid_cached_data", isolate->factory()->NewJSArray(0)); + isolate->Throw(*result); + isolate->ReportPendingMessages(); + has_pending_exception = true; + EXCEPTION_BAILOUT_CHECK(isolate, Local()); } } else if (source->cached_data) { + cached_data_mode = i::CONSUME_CACHED_DATA; // ScriptData takes care of aligning, in case the data is not aligned // correctly. script_data_impl = i::ScriptData::New( reinterpret_cast(source->cached_data->data), source->cached_data->length); - // We assert that the pre-data is sane, even though we can actually - // handle it if it turns out not to be in release mode. - ASSERT(script_data_impl->SanityCheck()); - if (script_data_impl->SanityCheck()) { - cached_data_mode = i::CONSUME_CACHED_DATA; - } else { - // If the pre-data isn't sane we simply ignore it. + // If the cached data is not valid, fail the compilation. + if (script_data_impl == NULL || !script_data_impl->SanityCheck()) { + EXCEPTION_PREAMBLE(isolate); + i::Handle result = isolate->factory()->NewSyntaxError( + "invalid_cached_data", isolate->factory()->NewJSArray(0)); + isolate->Throw(*result); + isolate->ReportPendingMessages(); delete script_data_impl; - script_data_impl = NULL; - delete source->cached_data; - source->cached_data = NULL; + has_pending_exception = true; + EXCEPTION_BAILOUT_CHECK(isolate, Local()); } } i::Handle str = Utils::OpenHandle(*(source->source_string)); - i::Isolate* isolate = reinterpret_cast(v8_isolate); - ON_BAILOUT(isolate, "v8::ScriptCompiler::CompileUnbound()", - return Local()); LOG_API(isolate, "ScriptCompiler::CompileUnbound"); ENTER_V8(isolate); i::SharedFunctionInfo* raw_result = NULL; diff --git a/src/messages.js b/src/messages.js index ff108d6..d402731 100644 --- a/src/messages.js +++ b/src/messages.js @@ -154,7 +154,8 @@ var kMessages = { array_indexof_not_defined: ["Array.getIndexOf: Argument undefined"], object_not_extensible: ["Can't add property ", "%0", ", object is not extensible"], illegal_access: ["Illegal access"], - invalid_preparser_data: ["Invalid preparser data for function ", "%0"], + invalid_cached_data_function: ["Invalid cached data for function ", "%0"], + invalid_cached_data: ["Invalid cached data"], strict_mode_with: ["Strict mode code may not include a with statement"], strict_eval_arguments: ["Unexpected eval or arguments in strict mode"], too_many_arguments: ["Too many arguments in function call (only 65535 allowed)"], diff --git a/src/parser.cc b/src/parser.cc index 30c2c63..90171ef 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -227,15 +227,13 @@ Handle Parser::LookupCachedSymbol(int symbol_id) { ScriptData* ScriptData::New(const char* data, int length) { // The length is obviously invalid. if (length % sizeof(unsigned) != 0) { - return new ScriptData(); + return NULL; } int deserialized_data_length = length / sizeof(unsigned); unsigned* deserialized_data; - ScriptData* script_data = new ScriptData(); - script_data->owns_store_ = - reinterpret_cast(data) % sizeof(unsigned) != 0; - if (script_data->owns_store_) { + bool owns_store = reinterpret_cast(data) % sizeof(unsigned) != 0; + if (owns_store) { // Copy the data to align it. deserialized_data = i::NewArray(deserialized_data_length); i::CopyBytes(reinterpret_cast(deserialized_data), @@ -244,9 +242,9 @@ ScriptData* ScriptData::New(const char* data, int length) { // If aligned, don't create a copy of the data. deserialized_data = reinterpret_cast(const_cast(data)); } - script_data->store_ = - Vector(deserialized_data, deserialized_data_length); - return script_data; + return new ScriptData( + Vector(deserialized_data, deserialized_data_length), + owns_store); } @@ -3138,10 +3136,10 @@ DebuggerStatement* Parser::ParseDebuggerStatement(bool* ok) { } -void Parser::ReportInvalidPreparseData(Handle name, bool* ok) { +void Parser::ReportInvalidCachedData(Handle name, bool* ok) { SmartArrayPointer name_string = name->ToCString(DISALLOW_NULLS); const char* element[1] = { name_string.get() }; - ReportMessage("invalid_preparser_data", + ReportMessage("invalid_cached_data_function", Vector(element, 1)); *ok = false; } @@ -3402,7 +3400,7 @@ FunctionLiteral* Parser::ParseFunctionLiteral( if (entry.end_pos() <= function_block_pos) { // End position greater than end of stream is safe, and hard // to check. - ReportInvalidPreparseData(function_name, CHECK_OK); + ReportInvalidCachedData(function_name, CHECK_OK); } scanner()->SeekForward(entry.end_pos() - 1); @@ -3415,13 +3413,8 @@ FunctionLiteral* Parser::ParseFunctionLiteral( scope_->SetStrictMode(entry.strict_mode()); } else { // This case happens when we have preparse data but it doesn't contain - // an entry for the function. As a safety net, fall back to eager - // parsing. It is unclear whether PreParser's laziness analysis can - // produce different results than the Parser's laziness analysis (see - // https://codereview.chromium.org/7565003 ). In this case, we must - // discard all the preparse data, since the symbol data will be wrong. - is_lazily_parsed = false; - cached_data_mode_ = NO_CACHED_DATA; + // an entry for the function. Fail the compilation. + ReportInvalidCachedData(function_name, CHECK_OK); } } else { // With no cached data, we partially parse the function, without diff --git a/src/parser.h b/src/parser.h index 7186f63..01f23f0 100644 --- a/src/parser.h +++ b/src/parser.h @@ -88,9 +88,9 @@ class ScriptData { : store_(store), owns_store_(true) { } - // Create an empty ScriptData that is guaranteed to not satisfy - // a SanityCheck. - ScriptData() : owns_store_(false) { } + ScriptData(Vector store, bool owns_store) + : store_(store), + owns_store_(owns_store) { } // The created ScriptData won't take ownership of the data. If the alignment // is not correct, this will copy the data (and the created ScriptData will @@ -667,7 +667,7 @@ class Parser : public ParserBase { Handle source); // Report syntax error - void ReportInvalidPreparseData(Handle name, bool* ok); + void ReportInvalidCachedData(Handle name, bool* ok); void SetCachedData(ScriptData** data, CachedDataMode cached_data_mode) { diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index 309040c..14aa700 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -14858,10 +14858,7 @@ TEST(PreCompileDeserializationError) { const char* data = "DONT CARE"; int invalid_size = 3; i::ScriptData* sd = i::ScriptData::New(data, invalid_size); - - CHECK_EQ(0, sd->Length()); - - delete sd; + CHECK_EQ(NULL, sd); } @@ -14911,16 +14908,18 @@ TEST(CompileWithInvalidCachedData) { v8::ScriptCompiler::CompileUnbound(isolate, &source2); CHECK(try_catch.HasCaught()); - String::Utf8Value exception_value(try_catch.Message()->Get()); - CHECK_EQ("Uncaught SyntaxError: Invalid preparser data for function bar", - *exception_value); + { + String::Utf8Value exception_value(try_catch.Message()->Get()); + CHECK_EQ("Uncaught SyntaxError: Invalid cached data for function bar", + *exception_value); + } try_catch.Reset(); delete sd; - // Overwrite function bar's start position with 200. The function entry - // will not be found when searching for it by position and we should fall - // back on eager compilation. + // Overwrite function bar's start position with 200. The function entry will + // not be found when searching for it by position, and the compilation fails. + // ScriptData does not take ownership of the buffers passed to it. sd = i::ScriptData::New(reinterpret_cast(cd->data), cd->length); sd_data = reinterpret_cast(const_cast(sd->Data())); @@ -14934,7 +14933,34 @@ TEST(CompileWithInvalidCachedData) { reinterpret_cast(sd->Data()), sd->Length())); compiled_script = v8::ScriptCompiler::CompileUnbound(isolate, &source3); - CHECK(!try_catch.HasCaught()); + CHECK(try_catch.HasCaught()); + { + String::Utf8Value exception_value(try_catch.Message()->Get()); + CHECK_EQ("Uncaught SyntaxError: Invalid cached data for function bar", + *exception_value); + } + CHECK(compiled_script.IsEmpty()); + try_catch.Reset(); + delete sd; + + // Try passing in cached data which is obviously invalid (wrong length). + sd = i::ScriptData::New(reinterpret_cast(cd->data), cd->length); + const char* script4 = + "function foo(){ return 8;}\n" + "function bar(){ return 6 + 7;} foo();"; + v8::ScriptCompiler::Source source4( + v8_str(script4), + new v8::ScriptCompiler::CachedData( + reinterpret_cast(sd->Data()), sd->Length() - 1)); + compiled_script = + v8::ScriptCompiler::CompileUnbound(isolate, &source4); + CHECK(try_catch.HasCaught()); + { + String::Utf8Value exception_value(try_catch.Message()->Get()); + CHECK_EQ("Uncaught SyntaxError: Invalid cached data", + *exception_value); + } + CHECK(compiled_script.IsEmpty()); delete sd; } -- 2.7.4