Skip to content

Commit

Permalink
deps: backport 2bd7464 from upstream V8
Browse files Browse the repository at this point in the history
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}

PR-URL: #10169
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>
  • Loading branch information
cristiancavalli authored and ofrobots committed Dec 13, 2016
1 parent 168241a commit ee09828
Show file tree
Hide file tree
Showing 7 changed files with 87 additions and 10 deletions.
2 changes: 1 addition & 1 deletion deps/v8/include/v8-version.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.)
Expand Down
1 change: 1 addition & 0 deletions deps/v8/src/bailout-reason.h
Original file line number Diff line number Diff line change
Expand Up @@ -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") \
Expand Down
9 changes: 7 additions & 2 deletions deps/v8/src/compiler/js-global-object-specialization.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<Map> property_cell_value_map(
Handle<HeapObject>::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);
Expand All @@ -190,8 +197,6 @@ Reduction JSGlobalObjectSpecialization::ReduceJSStoreGlobal(Node* node) {
Node* value_map = effect =
graph()->NewNode(simplified()->LoadField(AccessBuilder::ForMap()),
value, effect, control);
Handle<Map> property_cell_value_map(
Handle<HeapObject>::cast(property_cell_value)->map(), isolate());
check = graph()->NewNode(
simplified()->ReferenceEqual(Type::Any()), value_map,
jsgraph()->HeapConstant(property_cell_value_map));
Expand Down
16 changes: 12 additions & 4 deletions deps/v8/src/crankshaft/hydrogen.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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> 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<HeapObject> cell_value(HeapObject::cast(cell->value()));
Handle<Map> 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<HCheckHeapObject>(value);
value = Add<HCheckMaps>(value, map);
value = Add<HCheckMaps>(value, cell_value_map);
access = access.WithRepresentation(Representation::HeapObject());
break;
}
Expand Down
8 changes: 5 additions & 3 deletions deps/v8/src/runtime/runtime-utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<LanguageMode>(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<LanguageMode>(__tmp_##name);


// Assert that the given argument is a number within the Int32 range
Expand Down
30 changes: 30 additions & 0 deletions deps/v8/test/mjsunit/regress/regress-crbug-659475-1.js
Original file line number Diff line number Diff line change
@@ -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');
31 changes: 31 additions & 0 deletions deps/v8/test/mjsunit/regress/regress-crbug-659475-2.js
Original file line number Diff line number Diff line change
@@ -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');

0 comments on commit ee09828

Please sign in to comment.