From 7d2d0839ad6095f943648011003dbbe67ed54e9d Mon Sep 17 00:00:00 2001 From: "mvstanton@chromium.org" Date: Fri, 6 Jun 2014 09:12:16 +0000 Subject: [PATCH] Clusterfuzz identified overflow check needed in dehoisting. BUG=380092 R=danno@chromium.org LOG=N Review URL: https://codereview.chromium.org/315593002 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@21708 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/hydrogen-dehoist.cc | 18 ++++++++++---- src/hydrogen-instructions.h | 44 ++++++++++++++++++++++------------ test/mjsunit/regress/regress-380092.js | 14 +++++++++++ 3 files changed, 56 insertions(+), 20 deletions(-) create mode 100644 test/mjsunit/regress/regress-380092.js diff --git a/src/hydrogen-dehoist.cc b/src/hydrogen-dehoist.cc index fe0ae76..fe68e46 100644 --- a/src/hydrogen-dehoist.cc +++ b/src/hydrogen-dehoist.cc @@ -28,15 +28,23 @@ static void DehoistArrayIndex(ArrayInstructionInterface* array_operation) { if (!constant->HasInteger32Value()) return; int32_t sign = binary_operation->IsSub() ? -1 : 1; int32_t value = constant->Integer32Value() * sign; - // We limit offset values to 30 bits because we want to avoid the risk of - // overflows when the offset is added to the object header size. - if (value >= 1 << array_operation->MaxBaseOffsetBits() || value < 0) return; + if (value < 0) return; + + // Check for overflow. + int32_t shift_amount = + 1 << ElementsKindToShiftSize(array_operation->elements_kind()); + int32_t multiplication_result = value * shift_amount; + if ((multiplication_result / shift_amount) != value) return; + value = multiplication_result; + + // Ensure that the array operation can add value to existing base offset + // without overflowing. + if (!array_operation->CanIncreaseBaseOffset(value)) return; array_operation->SetKey(subexpression); if (binary_operation->HasNoUses()) { binary_operation->DeleteAndReplaceWith(NULL); } - value <<= ElementsKindToShiftSize(array_operation->elements_kind()); - array_operation->IncreaseBaseOffset(static_cast(value)); + array_operation->IncreaseBaseOffset(value); array_operation->SetDehoisted(true); } diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h index d1f2372..1fc800e 100644 --- a/src/hydrogen-instructions.h +++ b/src/hydrogen-instructions.h @@ -6404,8 +6404,9 @@ class ArrayInstructionInterface { virtual HValue* GetKey() = 0; virtual void SetKey(HValue* key) = 0; virtual ElementsKind elements_kind() const = 0; - virtual void IncreaseBaseOffset(uint32_t base_offset) = 0; - virtual int MaxBaseOffsetBits() = 0; + // increase_by_value should be non-negative + virtual bool CanIncreaseBaseOffset(int32_t increase_by_value) = 0; + virtual void IncreaseBaseOffset(int32_t increase_by_value) = 0; virtual bool IsDehoisted() = 0; virtual void SetDehoisted(bool is_dehoisted) = 0; virtual ~ArrayInstructionInterface() { } @@ -6451,13 +6452,20 @@ class HLoadKeyed V8_FINAL return OperandAt(2); } bool HasDependency() const { return OperandAt(0) != OperandAt(2); } - uint32_t base_offset() { return BaseOffsetField::decode(bit_field_); } - void IncreaseBaseOffset(uint32_t base_offset) { - base_offset += BaseOffsetField::decode(bit_field_); - bit_field_ = BaseOffsetField::update(bit_field_, base_offset); + uint32_t base_offset() { + int32_t base_offset_value = BaseOffsetField::decode(bit_field_); + ASSERT(base_offset_value >= 0); + return static_cast(base_offset_value); } - virtual int MaxBaseOffsetBits() { - return kBitsForBaseOffset; + bool CanIncreaseBaseOffset(int32_t increase_by_value) { + ASSERT(increase_by_value >= 0); + int32_t new_value = BaseOffsetField::decode(bit_field_) + increase_by_value; + return (new_value >= 0 && BaseOffsetField::is_valid(new_value)); + } + void IncreaseBaseOffset(int32_t increase_by_value) { + ASSERT(increase_by_value >= 0); + increase_by_value += BaseOffsetField::decode(bit_field_); + bit_field_ = BaseOffsetField::update(bit_field_, increase_by_value); } HValue* GetKey() { return key(); } void SetKey(HValue* key) { SetOperandAt(1, key); } @@ -6607,7 +6615,7 @@ class HLoadKeyed V8_FINAL public BitField {}; // NOLINT class BaseOffsetField: - public BitField + public BitField {}; // NOLINT class IsDehoistedField: public BitField @@ -6921,12 +6929,18 @@ class HStoreKeyed V8_FINAL } StoreFieldOrKeyedMode store_mode() const { return store_mode_; } ElementsKind elements_kind() const { return elements_kind_; } - uint32_t base_offset() { return base_offset_; } - void IncreaseBaseOffset(uint32_t base_offset) { - base_offset_ += base_offset; + uint32_t base_offset() { + ASSERT(base_offset_ >= 0); + return static_cast(base_offset_); + } + bool CanIncreaseBaseOffset(int32_t increase_by_value) { + ASSERT(increase_by_value >= 0); + // Guard against overflow + return (increase_by_value + base_offset_) >= 0; } - virtual int MaxBaseOffsetBits() { - return 31 - ElementsKindToShiftSize(elements_kind_); + void IncreaseBaseOffset(int32_t increase_by_value) { + ASSERT(increase_by_value >= 0); + base_offset_ += increase_by_value; } HValue* GetKey() { return key(); } void SetKey(HValue* key) { SetOperandAt(1, key); } @@ -7017,7 +7031,7 @@ class HStoreKeyed V8_FINAL } ElementsKind elements_kind_; - uint32_t base_offset_; + int32_t base_offset_; bool is_dehoisted_ : 1; bool is_uninitialized_ : 1; StoreFieldOrKeyedMode store_mode_: 1; diff --git a/test/mjsunit/regress/regress-380092.js b/test/mjsunit/regress/regress-380092.js new file mode 100644 index 0000000..a763435 --- /dev/null +++ b/test/mjsunit/regress/regress-380092.js @@ -0,0 +1,14 @@ +// Copyright 2014 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 many_hoist(o, index) { + return o[index + 33554427]; +} + +var obj = {}; +many_hoist(obj, 0); +%OptimizeFunctionOnNextCall(many_hoist); +many_hoist(obj, 5); -- 2.7.4