From d86038db25064fb40fbce6fc3b2fb67831f9e7b1 Mon Sep 17 00:00:00 2001 From: bmeurer Date: Mon, 19 Sep 2016 22:58:39 -0700 Subject: [PATCH] [crankshaft] Protect against deopt loops from string length overflows. Crankshaft just unconditionally deoptimizes the code when the length of a string addition result would overflow. In order to protect against deopt loops we insert a global protector cell. We will use the same mechanism for inlining certain string additions into TurboFan as well, and protecting against overflow (we will also extend this to deal with String.prototype.concat and friends once we get there). BUG=v8:5404 R=jarin@chromium.org,hpayer@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux64_msan_rel Committed: https://crrev.com/cb19257a926a55209a6d6858ce26d51a0447ba71 Review-Url: https://codereview.chromium.org/2348293002 Cr-Original-Commit-Position: refs/heads/master@{#39511} Cr-Commit-Position: refs/heads/master@{#39525} --- src/crankshaft/hydrogen.cc | 4 +++- src/crankshaft/hydrogen.h | 8 ++++++++ src/factory.cc | 7 +++++++ src/factory.h | 4 +--- src/heap/heap.cc | 4 ++++ src/heap/heap.h | 1 + src/isolate-inl.h | 5 +++++ src/isolate.cc | 9 +++++++++ src/isolate.h | 2 ++ src/runtime/runtime-internal.cc | 3 +-- test/mjsunit/regress/regress-5404.js | 21 +++++++++++++++++++++ 11 files changed, 62 insertions(+), 6 deletions(-) create mode 100644 test/mjsunit/regress/regress-5404.js diff --git a/src/crankshaft/hydrogen.cc b/src/crankshaft/hydrogen.cc index e5332a2f5d..f1521c8d91 100644 --- a/src/crankshaft/hydrogen.cc +++ b/src/crankshaft/hydrogen.cc @@ -2368,7 +2368,7 @@ HValue* HGraphBuilder::BuildAddStringLengths(HValue* left_length, HValue* length = AddUncasted(left_length, right_length); // Check that length <= kMaxLength <=> length < MaxLength + 1. HValue* max_length = Add(String::kMaxLength + 1); - if (top_info()->IsStub()) { + if (top_info()->IsStub() || !isolate()->IsStringLengthOverflowIntact()) { // This is a mitigation for crbug.com/627934; the real fix // will be to migrate the StringAddStub to TurboFan one day. IfBuilder if_invalid(this); @@ -2380,6 +2380,7 @@ HValue* HGraphBuilder::BuildAddStringLengths(HValue* left_length, } if_invalid.End(); } else { + graph()->MarkDependsOnStringLengthOverflow(); Add(length, max_length); } return length; @@ -3570,6 +3571,7 @@ HGraph::HGraph(CompilationInfo* info, CallInterfaceDescriptor descriptor) allow_code_motion_(false), use_optimistic_licm_(false), depends_on_empty_array_proto_elements_(false), + depends_on_string_length_overflow_(false), type_change_checksum_(0), maximum_environment_size_(0), no_side_effects_scope_count_(0), diff --git a/src/crankshaft/hydrogen.h b/src/crankshaft/hydrogen.h index d01958456d..9f7ef0bf82 100644 --- a/src/crankshaft/hydrogen.h +++ b/src/crankshaft/hydrogen.h @@ -440,6 +440,13 @@ class HGraph final : public ZoneObject { return depends_on_empty_array_proto_elements_; } + void MarkDependsOnStringLengthOverflow() { + if (depends_on_string_length_overflow_) return; + info()->dependencies()->AssumePropertyCell( + isolate()->factory()->string_length_protector()); + depends_on_string_length_overflow_ = true; + } + bool has_uint32_instructions() { DCHECK(uint32_instructions_ == NULL || !uint32_instructions_->is_empty()); return uint32_instructions_ != NULL; @@ -515,6 +522,7 @@ class HGraph final : public ZoneObject { bool allow_code_motion_; bool use_optimistic_licm_; bool depends_on_empty_array_proto_elements_; + bool depends_on_string_length_overflow_; int type_change_checksum_; int maximum_environment_size_; int no_side_effects_scope_count_; diff --git a/src/factory.cc b/src/factory.cc index 1c31840a55..589f36d46d 100644 --- a/src/factory.cc +++ b/src/factory.cc @@ -1214,6 +1214,13 @@ Handle Factory::NewError(Handle constructor, return maybe_error.ToHandleChecked(); } +Handle Factory::NewInvalidStringLengthError() { + // Invalidate the "string length" protector. + if (isolate()->IsStringLengthOverflowIntact()) { + isolate()->InvalidateStringLengthOverflowProtector(); + } + return NewRangeError(MessageTemplate::kInvalidStringLength); +} #define DEFINE_ERROR(NAME, name) \ Handle Factory::New##NAME(MessageTemplate::Template template_index, \ diff --git a/src/factory.h b/src/factory.h index fc7870496e..94ae55850a 100644 --- a/src/factory.h +++ b/src/factory.h @@ -586,9 +586,7 @@ class Factory final { Handle NewError(Handle constructor, Handle message); - Handle NewInvalidStringLengthError() { - return NewRangeError(MessageTemplate::kInvalidStringLength); - } + Handle NewInvalidStringLengthError(); Handle NewURIError() { return NewError(isolate()->uri_error_function(), diff --git a/src/heap/heap.cc b/src/heap/heap.cc index f5e043c93c..e2b82df047 100644 --- a/src/heap/heap.cc +++ b/src/heap/heap.cc @@ -2911,6 +2911,10 @@ void Heap::CreateInitialObjects() { handle(Smi::FromInt(Isolate::kArrayProtectorValid), isolate())); set_species_protector(*species_cell); + cell = factory->NewPropertyCell(); + cell->set_value(Smi::FromInt(Isolate::kArrayProtectorValid)); + set_string_length_protector(*cell); + set_serialized_templates(empty_fixed_array()); set_weak_stack_trace_list(Smi::FromInt(0)); diff --git a/src/heap/heap.h b/src/heap/heap.h index 025ff43e38..81d65a4d52 100644 --- a/src/heap/heap.h +++ b/src/heap/heap.h @@ -166,6 +166,7 @@ using v8::MemoryPressureLevel; V(Cell, is_concat_spreadable_protector, IsConcatSpreadableProtector) \ V(PropertyCell, has_instance_protector, HasInstanceProtector) \ V(Cell, species_protector, SpeciesProtector) \ + V(PropertyCell, string_length_protector, StringLengthProtector) \ /* Special numbers */ \ V(HeapNumber, nan_value, NanValue) \ V(HeapNumber, hole_nan_value, HoleNanValue) \ diff --git a/src/isolate-inl.h b/src/isolate-inl.h index 5c71d9188e..7011c0d35a 100644 --- a/src/isolate-inl.h +++ b/src/isolate-inl.h @@ -147,6 +147,11 @@ bool Isolate::IsHasInstanceLookupChainIntact() { return has_instance_cell->value() == Smi::FromInt(kArrayProtectorValid); } +bool Isolate::IsStringLengthOverflowIntact() { + PropertyCell* has_instance_cell = heap()->string_length_protector(); + return has_instance_cell->value() == Smi::FromInt(kArrayProtectorValid); +} + } // namespace internal } // namespace v8 diff --git a/src/isolate.cc b/src/isolate.cc index 02e0877117..cf319454d3 100644 --- a/src/isolate.cc +++ b/src/isolate.cc @@ -2818,6 +2818,15 @@ void Isolate::InvalidateArraySpeciesProtector() { DCHECK(!IsArraySpeciesLookupChainIntact()); } +void Isolate::InvalidateStringLengthOverflowProtector() { + DCHECK(factory()->string_length_protector()->value()->IsSmi()); + DCHECK(IsStringLengthOverflowIntact()); + PropertyCell::SetValueWithInvalidation( + factory()->string_length_protector(), + handle(Smi::FromInt(kArrayProtectorInvalid), this)); + DCHECK(!IsStringLengthOverflowIntact()); +} + bool Isolate::IsAnyInitialArrayPrototype(Handle array) { DisallowHeapAllocation no_gc; return IsInAnyContext(*array, Context::INITIAL_ARRAY_PROTOTYPE_INDEX); diff --git a/src/isolate.h b/src/isolate.h index 95c300f3a9..d8a50c1567 100644 --- a/src/isolate.h +++ b/src/isolate.h @@ -1010,6 +1010,7 @@ class Isolate { inline bool IsHasInstanceLookupChainIntact(); bool IsIsConcatSpreadableLookupChainIntact(); bool IsIsConcatSpreadableLookupChainIntact(JSReceiver* receiver); + inline bool IsStringLengthOverflowIntact(); // On intent to set an element in object, make sure that appropriate // notifications occur if the set is on the elements of the array or @@ -1028,6 +1029,7 @@ class Isolate { void InvalidateArraySpeciesProtector(); void InvalidateHasInstanceProtector(); void InvalidateIsConcatSpreadableProtector(); + void InvalidateStringLengthOverflowProtector(); // Returns true if array is the initial array prototype in any native context. bool IsAnyInitialArrayPrototype(Handle array); diff --git a/src/runtime/runtime-internal.cc b/src/runtime/runtime-internal.cc index a094d938d0..aa4675b713 100644 --- a/src/runtime/runtime-internal.cc +++ b/src/runtime/runtime-internal.cc @@ -234,8 +234,7 @@ RUNTIME_FUNCTION(Runtime_ThrowIncompatibleMethodReceiver) { RUNTIME_FUNCTION(Runtime_ThrowInvalidStringLength) { HandleScope scope(isolate); - THROW_NEW_ERROR_RETURN_FAILURE( - isolate, NewRangeError(MessageTemplate::kInvalidStringLength)); + THROW_NEW_ERROR_RETURN_FAILURE(isolate, NewInvalidStringLengthError()); } RUNTIME_FUNCTION(Runtime_ThrowIteratorResultNotAnObject) { diff --git a/test/mjsunit/regress/regress-5404.js b/test/mjsunit/regress/regress-5404.js new file mode 100644 index 0000000000..652db4bdb5 --- /dev/null +++ b/test/mjsunit/regress/regress-5404.js @@ -0,0 +1,21 @@ +// 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 + +function foo(a, b) { + return a + "0123456789012"; +} + +foo("a"); +foo("a"); +%OptimizeFunctionOnNextCall(foo); +foo("a"); + +var a = "a".repeat(268435440); +assertThrows(function() { foo(a); }); + +%OptimizeFunctionOnNextCall(foo); +assertThrows(function() { foo(a); }); +assertOptimized(foo);