From 0855fb151bcd914d3322886bdce097ec359db85e Mon Sep 17 00:00:00 2001 From: Jakob Gruber Date: Thu, 25 Oct 2018 13:21:32 +0200 Subject: [PATCH] [array] Ensure PrepareElementsForSort returns a legal value PrepareElementsForSort must return a number less than or equal the array length. Bug: chromium:897512, v8:7382 Change-Id: If5f9c4d052e623ab9f3300b8534603abbee859fa Reviewed-on: https://chromium-review.googlesource.com/c/1297958 Commit-Queue: Jakob Gruber Reviewed-by: Camillo Bruni Cr-Commit-Position: refs/heads/master@{#56982} --- src/runtime/runtime-array.cc | 12 +++++++- test/mjsunit/regress/regress-897512.js | 24 +++++++++++++++ third_party/v8/builtins/array-sort.tq | 41 ++++++++++---------------- 3 files changed, 50 insertions(+), 27 deletions(-) create mode 100644 test/mjsunit/regress/regress-897512.js diff --git a/src/runtime/runtime-array.cc b/src/runtime/runtime-array.cc index 595e9f3564..7b4e06e68f 100644 --- a/src/runtime/runtime-array.cc +++ b/src/runtime/runtime-array.cc @@ -154,7 +154,15 @@ Object* RemoveArrayHolesGeneric(Isolate* isolate, Handle receiver, MAYBE_RETURN(delete_result, ReadOnlyRoots(isolate).exception()); } - return *isolate->factory()->NewNumberFromUint(result); + // TODO(jgruber, szuend, chromium:897512): This is a workaround to prevent + // returning a number greater than array.length to Array.p.sort, which could + // trigger OOB accesses. There is still a correctness bug here though in + // how we shift around undefineds and delete elements in the two blocks above. + // This needs to be fixed soon. + const uint32_t number_of_non_undefined_elements = std::min(limit, result); + + return *isolate->factory()->NewNumberFromUint( + number_of_non_undefined_elements); } // Collects all defined (non-hole) and non-undefined (array) elements at the @@ -171,6 +179,7 @@ Object* RemoveArrayHoles(Isolate* isolate, Handle receiver, Handle object = Handle::cast(receiver); if (object->HasStringWrapperElements()) { int len = String::cast(Handle::cast(object)->value())->length(); + DCHECK_LE(len, limit); return Smi::FromInt(len); } @@ -293,6 +302,7 @@ Object* RemoveArrayHoles(Isolate* isolate, Handle receiver, } } + DCHECK_LE(result, limit); return *isolate->factory()->NewNumberFromUint(result); } diff --git a/test/mjsunit/regress/regress-897512.js b/test/mjsunit/regress/regress-897512.js new file mode 100644 index 0000000000..0e676a06c2 --- /dev/null +++ b/test/mjsunit/regress/regress-897512.js @@ -0,0 +1,24 @@ +// Copyright 2018 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. + +// Fill up the Array prototype's elements. +for (let i = 0; i < 100; i++) Array.prototype.unshift(3.14); + +// Create a holey double elements array. +const o31 = [1.1]; +o31[37] = 2.2; + +// Concat converts to dictionary elements. +const o51 = o31.concat(false); + +// Set one element to undefined to trigger the movement bug. +o51[0] = undefined; + +assertEquals(o51.length, 39); + +// Sort triggers the bug. +o51.sort(); + +// TODO(chromium:897512): The length should be 39. +assertEquals(o51.length, 101); diff --git a/third_party/v8/builtins/array-sort.tq b/third_party/v8/builtins/array-sort.tq index ac457a6ff1..1993ac0076 100644 --- a/third_party/v8/builtins/array-sort.tq +++ b/third_party/v8/builtins/array-sort.tq @@ -1716,7 +1716,6 @@ module array { // 2. Let obj be ? ToObject(this value). const obj: JSReceiver = ToObject(context, receiver); - let map: Map = obj.map; const sortState: FixedArray = AllocateZeroedFixedArray(kSortStateSize); @@ -1724,22 +1723,24 @@ module array { sortState[kUserCmpFnIdx] = comparefnObj; sortState[kSortComparePtrIdx] = comparefnObj != Undefined ? SortCompareUserFn : SortCompareDefault; - sortState[kInitialReceiverMapIdx] = map; sortState[kBailoutStatusIdx] = kSuccess; + // 3. Let len be ? ToLength(? Get(obj, "length")). + const len: Number = GetLengthProperty(obj); + + if (len < 2) return receiver; + + // TODO(szuend): Investigate performance tradeoff of skipping this step + // for PACKED_* and handling Undefineds during sorting. + const nofNonUndefined: Smi = PrepareElementsForSort(context, obj, len); + assert(nofNonUndefined <= len); + + let map: Map = obj.map; + sortState[kInitialReceiverMapIdx] = map; + sortState[kInitialReceiverLengthIdx] = len; + try { - const a: FastJSArray = Cast(receiver) otherwise Slow; - - // 3. Let len be ? ToLength(? Get(obj, "length")). - const len: Smi = a.length; - if (len < 2) return receiver; - - // TODO(szuend): Investigate performance tradeoff of skipping this step - // for PACKED_* and handling Undefineds during sorting. - const nofNonUndefined: Smi = PrepareElementsForSort(context, obj, len); - assert(a.map == map); - - sortState[kInitialReceiverLengthIdx] = len; + let a: FastJSArray = Cast(receiver) otherwise Slow; const elementsKind: ElementsKind = map.elements_kind; if (IsDoubleElementsKind(elementsKind)) { @@ -1752,18 +1753,6 @@ module array { ArrayTimSort(context, sortState, nofNonUndefined); } label Slow { - // 3. Let len be ? ToLength(? Get(obj, "length")). - const len: Number = GetLengthProperty(obj); - - if (len < 2) return receiver; - const nofNonUndefined: Smi = PrepareElementsForSort(context, obj, len); - - sortState[kInitialReceiverLengthIdx] = len; - - // Reload the map, PrepareElementsForSort might have changed the - // elements kind. - map = obj.map; - if (map.elements_kind == DICTIONARY_ELEMENTS && IsExtensibleMap(map) && !IsCustomElementsReceiverInstanceType(map.instance_type)) { InitializeSortStateAccessor(sortState);