From a9b1dcd48c82fdccbde7fb7725a555cd046e5a7c Mon Sep 17 00:00:00 2001 From: Cristian Cavalli Date: Wed, 16 Nov 2016 13:14:46 -0800 Subject: [PATCH] deps: backport 2bd7464 from upstream V8 Original commit message: For global object property cells, we did not check that the map on the previous object is still the same for which we actually optimized. So the optimized code was not in sync with the actual state of the property cell. When loading from such a global object property cell, Crankshaft optimizes away any map checks (based on the stable map assumption), leading to arbitrary memory access in the worst case. TurboFan has the same bug for stores, but is safe on loads because we do appropriate map checks there. However mixing TurboFan and Crankshaft still exposes the bug. R=yangguo@chromium.org BUG=chromium:659475 Review-Url: https://codereview.chromium.org/2444233004 Cr-Commit-Position: refs/heads/master@{#40592} --- deps/v8/include/v8-version.h | 2 +- deps/v8/src/bailout-reason.h | 1 + .../js-global-object-specialization.cc | 9 ++++-- deps/v8/src/crankshaft/hydrogen.cc | 16 +++++++--- deps/v8/src/runtime/runtime-utils.h | 8 +++-- .../mjsunit/regress/regress-crbug-659475-1.js | 30 ++++++++++++++++++ .../mjsunit/regress/regress-crbug-659475-2.js | 31 +++++++++++++++++++ 7 files changed, 87 insertions(+), 10 deletions(-) create mode 100644 deps/v8/test/mjsunit/regress/regress-crbug-659475-1.js create mode 100644 deps/v8/test/mjsunit/regress/regress-crbug-659475-2.js diff --git a/deps/v8/include/v8-version.h b/deps/v8/include/v8-version.h index 4723fe3b9d4a98..fb83ab5c6ca287 100644 --- a/deps/v8/include/v8-version.h +++ b/deps/v8/include/v8-version.h @@ -11,7 +11,7 @@ #define V8_MAJOR_VERSION 5 #define V8_MINOR_VERSION 1 #define V8_BUILD_NUMBER 281 -#define V8_PATCH_LEVEL 88 +#define V8_PATCH_LEVEL 89 // Use 1 for candidates and 0 otherwise. // (Boolean macro values are not supported by all preprocessors.) diff --git a/deps/v8/src/bailout-reason.h b/deps/v8/src/bailout-reason.h index 92929cf38c24bb..37fb64a87aac31 100644 --- a/deps/v8/src/bailout-reason.h +++ b/deps/v8/src/bailout-reason.h @@ -257,6 +257,7 @@ namespace internal { V(kUnexpectedReturnFromThrow, "Unexpectedly returned from a throw") \ V(kUnsupportedSwitchStatement, "Unsupported switch statement") \ V(kUnsupportedTaggedImmediate, "Unsupported tagged immediate") \ + V(kUnstableConstantTypeHeapObject, "Unstable constant-type heap object") \ V(kVariableResolvedToWithContext, "Variable resolved to with context") \ V(kWeShouldNotHaveAnEmptyLexicalContext, \ "We should not have an empty lexical context") \ diff --git a/deps/v8/src/compiler/js-global-object-specialization.cc b/deps/v8/src/compiler/js-global-object-specialization.cc index d8c9f17fd4c207..6239bf66d2881d 100644 --- a/deps/v8/src/compiler/js-global-object-specialization.cc +++ b/deps/v8/src/compiler/js-global-object-specialization.cc @@ -182,6 +182,13 @@ Reduction JSGlobalObjectSpecialization::ReduceJSStoreGlobal(Node* node) { Node* check = graph()->NewNode(simplified()->ObjectIsSmi(), value); Type* property_cell_value_type = Type::TaggedSigned(); if (property_cell_value->IsHeapObject()) { + // We cannot do anything if the {property_cell_value}s map is no + // longer stable. + Handle property_cell_value_map( + Handle::cast(property_cell_value)->map(), isolate()); + if (!property_cell_value_map->is_stable()) return NoChange(); + dependencies()->AssumeMapStable(property_cell_value_map); + // Deoptimize if the {value} is a Smi. control = graph()->NewNode(common()->DeoptimizeIf(), check, frame_state, effect, control); @@ -190,8 +197,6 @@ Reduction JSGlobalObjectSpecialization::ReduceJSStoreGlobal(Node* node) { Node* value_map = effect = graph()->NewNode(simplified()->LoadField(AccessBuilder::ForMap()), value, effect, control); - Handle property_cell_value_map( - Handle::cast(property_cell_value)->map(), isolate()); check = graph()->NewNode( simplified()->ReferenceEqual(Type::Any()), value_map, jsgraph()->HeapConstant(property_cell_value_map)); diff --git a/deps/v8/src/crankshaft/hydrogen.cc b/deps/v8/src/crankshaft/hydrogen.cc index 295b2c94557ecc..5a37411834e72c 100644 --- a/deps/v8/src/crankshaft/hydrogen.cc +++ b/deps/v8/src/crankshaft/hydrogen.cc @@ -6908,11 +6908,19 @@ void HOptimizedGraphBuilder::HandleGlobalVariableAssignment( access = access.WithRepresentation(Representation::Smi()); break; case PropertyCellConstantType::kStableMap: { - // The map may no longer be stable, deopt if it's ever different from - // what is currently there, which will allow for restablization. - Handle map(HeapObject::cast(cell->value())->map()); + // First check that the previous value of the {cell} still has the + // map that we are about to check the new {value} for. If not, then + // the stable map assumption was invalidated and we cannot continue + // with the optimized code. + Handle cell_value(HeapObject::cast(cell->value())); + Handle cell_value_map(cell_value->map()); + if (!cell_value_map->is_stable()) { + return Bailout(kUnstableConstantTypeHeapObject); + } + top_info()->dependencies()->AssumeMapStable(cell_value_map); + // Now check that the new {value} is a HeapObject with the same map. Add(value); - value = Add(value, map); + value = Add(value, cell_value_map); access = access.WithRepresentation(Representation::HeapObject()); break; } diff --git a/deps/v8/src/runtime/runtime-utils.h b/deps/v8/src/runtime/runtime-utils.h index 17c78d5a0b5ca5..7f2829861d55b2 100644 --- a/deps/v8/src/runtime/runtime-utils.h +++ b/deps/v8/src/runtime/runtime-utils.h @@ -115,9 +115,11 @@ namespace internal { // Assert that the given argument has a valid value for a LanguageMode // and store it in a LanguageMode variable with the given name. #define CONVERT_LANGUAGE_MODE_ARG_CHECKED(name, index) \ - RUNTIME_ASSERT(args[index]->IsSmi()); \ - RUNTIME_ASSERT(is_valid_language_mode(args.smi_at(index))); \ - LanguageMode name = static_cast(args.smi_at(index)); + RUNTIME_ASSERT(args[index]->IsNumber()); \ + int32_t __tmp_##name = 0; \ + RUNTIME_ASSERT(args[index]->ToInt32(&__tmp_##name)); \ + RUNTIME_ASSERT(is_valid_language_mode(__tmp_##name)); \ + LanguageMode name = static_cast(__tmp_##name); // Assert that the given argument is a number within the Int32 range diff --git a/deps/v8/test/mjsunit/regress/regress-crbug-659475-1.js b/deps/v8/test/mjsunit/regress/regress-crbug-659475-1.js new file mode 100644 index 00000000000000..7b996237412e25 --- /dev/null +++ b/deps/v8/test/mjsunit/regress/regress-crbug-659475-1.js @@ -0,0 +1,30 @@ +// Copyright 2016 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --allow-natives-syntax + +var n; + +function Ctor() { + n = new Set(); +} + +function Check() { + n.xyz = 0x826852f4; +} + +Ctor(); +Ctor(); +%OptimizeFunctionOnNextCall(Ctor); +Ctor(); + +Check(); +Check(); +%OptimizeFunctionOnNextCall(Check); +Check(); + +Ctor(); +Check(); + +parseInt('AAAAAAAA'); \ No newline at end of file diff --git a/deps/v8/test/mjsunit/regress/regress-crbug-659475-2.js b/deps/v8/test/mjsunit/regress/regress-crbug-659475-2.js new file mode 100644 index 00000000000000..a992570af4e09b --- /dev/null +++ b/deps/v8/test/mjsunit/regress/regress-crbug-659475-2.js @@ -0,0 +1,31 @@ +// Copyright 2016 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --allow-natives-syntax + +var n; + +function Ctor() { + try { } catch (e) {} + n = new Set(); +} + +function Check() { + n.xyz = 0x826852f4; +} + +Ctor(); +Ctor(); +%OptimizeFunctionOnNextCall(Ctor); +Ctor(); + +Check(); +Check(); +%OptimizeFunctionOnNextCall(Check); +Check(); + +Ctor(); +Check(); + +parseInt('AAAAAAAA'); \ No newline at end of file