From a5941ac99f91e4e21b9e0651161c5422fcb6163a Mon Sep 17 00:00:00 2001 From: Michael Achenbach Date: Thu, 25 Apr 2019 11:22:22 +0000 Subject: [PATCH] Revert "[typedarray] Fix crash when sorting SharedArrayBuffers" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 3d846115d6b7d1a680cad775ff3c6f0ca1768451. Reason for revert: The test hangs flakily on windows: https://ci.chromium.org/p/v8/builders/ci/V8%20Win32/20612 https://ci.chromium.org/p/v8/builders/ci/V8%20Win32%20-%20nosnap%20-%20shared/33147 https://ci.chromium.org/p/v8/builders/ci/V8%20Win32%20-%20debug/19945 Original change's description: > [typedarray] Fix crash when sorting SharedArrayBuffers > > TypedArray#sort has a fast-path when the user does not provide a > comparison function. This fast-path utilizes std::sort which operates > directly on the raw data. Per spec, std::sort requires the "less than" > operation to be anti-symmetric and transitive. > > When sorting SharedArrayBuffers (SAB) that are concurrently modified during > sorting, the "less than" operator stops being consistent as the > underlying data is constantly modified. This breaks some invariants > in std::sort resulting in infinite loops or straight out segfaults. > > This CL fixes this by copying the data before sorting SABs and > writing the sorted result back. > > Note: The added regression test is tailored for ASAN bots as a > normal build would need too many iterations to consistently crash. > > R=​neis@chromium.org, petermarshall@chromium.org > > Bug: v8:9161 > Change-Id: Ic089928652f75865bfdb11e7453806faa6ecb988 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1581641 > Reviewed-by: Peter Marshall > Reviewed-by: Georg Neis > Commit-Queue: Simon Zünd > Cr-Commit-Position: refs/heads/master@{#61004} TBR=neis@chromium.org,petermarshall@chromium.org,szuend@chromium.org Change-Id: I046da3e4228bb1a8a3aa89d9c9d8de11875a9273 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: v8:9161 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1583725 Reviewed-by: Michael Achenbach Commit-Queue: Michael Achenbach Cr-Commit-Position: refs/heads/master@{#61007} --- src/runtime/runtime-typedarray.cc | 46 ++++------------- test/mjsunit/mjsunit.status | 3 -- test/mjsunit/regress/regress-crbug-9161.js | 59 ---------------------- 3 files changed, 11 insertions(+), 97 deletions(-) delete mode 100644 test/mjsunit/regress/regress-crbug-9161.js diff --git a/src/runtime/runtime-typedarray.cc b/src/runtime/runtime-typedarray.cc index 37dfcda2c4..c8097b5b5f 100644 --- a/src/runtime/runtime-typedarray.cc +++ b/src/runtime/runtime-typedarray.cc @@ -101,43 +101,26 @@ RUNTIME_FUNCTION(Runtime_TypedArraySortFast) { HandleScope scope(isolate); DCHECK_EQ(1, args.length()); - // Validation is handled in the Torque builtin. - CONVERT_ARG_HANDLE_CHECKED(JSTypedArray, array, 0); - DCHECK(!array->WasDetached()); + CONVERT_ARG_HANDLE_CHECKED(Object, target_obj, 0); + + Handle array; + const char* method = "%TypedArray%.prototype.sort"; + ASSIGN_RETURN_FAILURE_ON_EXCEPTION( + isolate, array, JSTypedArray::Validate(isolate, target_obj, method)); + + // This line can be removed when JSTypedArray::Validate throws + // if array.[[ViewedArrayBuffer]] is detached(v8:4648) + if (V8_UNLIKELY(array->WasDetached())) return *array; size_t length = array->length(); if (length <= 1) return *array; Handle elements( FixedTypedArrayBase::cast(array->elements()), isolate); - - // In case of a SAB, the data is copied into temporary memory, as - // std::sort might crash in case the underlying data is concurrently - // modified while sorting. - CHECK(array->buffer()->IsJSArrayBuffer()); - Handle buffer(JSArrayBuffer::cast(array->buffer()), isolate); - const bool copy_data = buffer->is_shared(); - - Handle array_copy; - if (copy_data) { - const size_t bytes = array->byte_length(); - // TODO(szuend): Re-check this approach once support for larger typed - // arrays has landed. - CHECK_LE(bytes, INT_MAX); - array_copy = isolate->factory()->NewByteArray(static_cast(bytes)); - std::memcpy(static_cast(array_copy->GetDataStartAddress()), - static_cast(elements->DataPtr()), bytes); - } - - DisallowHeapAllocation no_gc; - switch (array->type()) { #define TYPED_ARRAY_SORT(Type, type, TYPE, ctype) \ case kExternal##Type##Array: { \ - ctype* data = \ - copy_data \ - ? reinterpret_cast(array_copy->GetDataStartAddress()) \ - : static_cast(elements->DataPtr()); \ + ctype* data = static_cast(elements->DataPtr()); \ if (kExternal##Type##Array == kExternalFloat64Array || \ kExternal##Type##Array == kExternalFloat32Array) { \ if (COMPRESS_POINTERS_BOOL && alignof(ctype) > kTaggedSize) { \ @@ -163,13 +146,6 @@ RUNTIME_FUNCTION(Runtime_TypedArraySortFast) { #undef TYPED_ARRAY_SORT } - if (copy_data) { - DCHECK(!array_copy.is_null()); - const size_t bytes = array->byte_length(); - std::memcpy(static_cast(elements->DataPtr()), - static_cast(array_copy->GetDataStartAddress()), bytes); - } - return *array; } diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status index ea3a2e6974..1ce2a5a397 100644 --- a/test/mjsunit/mjsunit.status +++ b/test/mjsunit/mjsunit.status @@ -175,7 +175,6 @@ 'regress/regress-605470': [PASS, SLOW], 'regress/regress-655573': [PASS, SLOW], 'regress/regress-1200351': [PASS, SLOW], - 'regress/regress-crbug-9161': [PASS, SLOW], 'regress/wasm/regress-810973': [PASS, SLOW], 'string-replace-gc': [PASS, SLOW], 'wasm/asm-wasm-f32': [PASS, SLOW], @@ -436,7 +435,6 @@ 'regress/regress-91010': [SKIP], 'regress/regress-91013': [SKIP], 'regress/regress-99167': [SKIP], - 'regress/regress-crbug-9161': [SKIP], # BUG(v8:3457). 'deserialize-reference': [PASS, FAIL], @@ -580,7 +578,6 @@ 'big-object-literal': [SKIP], 'compiler/alloc-number': [SKIP], 'regress/regress-490': [SKIP], - 'regress/regress-crbug-9161': [SKIP], 'regress/regress-create-exception': [SKIP], 'regress/regress-3247124': [SKIP], diff --git a/test/mjsunit/regress/regress-crbug-9161.js b/test/mjsunit/regress/regress-crbug-9161.js deleted file mode 100644 index a90a8ad6ea..0000000000 --- a/test/mjsunit/regress/regress-crbug-9161.js +++ /dev/null @@ -1,59 +0,0 @@ -// Copyright 2019 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. -// -// This test is a reproduction of a crash that happens when a TypedArray -// backed by a SharedArrayBuffer is concurrently modified while sorting. -// Segfaults would need a long time to trigger in normal builds, so this -// reproduction is tailored to trigger on ASAN builds. On ASAN builds, -// out-of-bounds accesses while sorting would result in an immediate failure. - -const lock = new Int32Array(new SharedArrayBuffer(4)); - -const kIterations = 5000; -const kLength = 2000; - -const kStageIndex = 0; -const kStageInit = 0; -const kStageRunning = 1; -const kStageDone = 2; - -Atomics.store(lock, kStageIndex, kStageInit); - -function WaitUntil(expected) { - while (true) { - const value = Atomics.load(lock, kStageIndex); - if (value === expected) break; - } -} - -const workerScript = ` - onmessage = function([sab, lock]) { - const i32a = new Int32Array(sab); - Atomics.store(lock, ${kStageIndex}, ${kStageRunning}); - - for (let j = 1; j < ${kIterations}; ++j) { - for (let i = 0; i < i32a.length; ++i) { - i32a[i] = j; - } - } - - postMessage("done"); - Atomics.store(lock, ${kStageIndex}, ${kStageDone}); - };`; - -const worker = new Worker(workerScript, {type: 'string'}); - -const i32a = new Int32Array( - new SharedArrayBuffer(Int32Array.BYTES_PER_ELEMENT * kLength) -); - -worker.postMessage([i32a.buffer, lock]); -WaitUntil(kStageRunning); - -for (let i = 0; i < kIterations; ++i) { - i32a.sort(); -} - -WaitUntil(kStageDone); -assertEquals(worker.getMessage(), "done");