From 7bdad948c81675f033c6039dbe54d3a04793fca1 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 21 Dec 2022 13:00:47 +0100 Subject: [PATCH] deps: V8: backport 8ca9f77d0f7c MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a crash when loading snapshots that contain empty ArrayBuffer instances: ```js const X = []; X.push(new ArrayBuffer()); v8.startupSnapshot.setDeserializeMainFunction(() => { for (let i = 0; i < 1000000; i++) { // trigger GC X.push(new ArrayBuffer()); } }) ``` Original commit message: [sandbox] Sandboxify JSArrayBuffer::extension external pointer Bug: chromium:1335043 Change-Id: Id8e6883fc652b144f6380ff09b1c18e777e041c2 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3706626 Reviewed-by: Michael Lippautz Reviewed-by: Igor Sheludko Commit-Queue: Samuel Groß Cr-Commit-Position: refs/heads/main@{#84544} Refs: https://github.com/v8/v8/commit/8ca9f77d0f7c2d5ec5ef8255b9689f5ac1c547a3 PR-URL: https://github.com/nodejs/node/pull/45871 Reviewed-By: Joyee Cheung Reviewed-By: Michaël Zasso Reviewed-By: James M Snell --- common.gypi | 2 +- deps/v8/include/v8-internal.h | 3 +- .../src/builtins/builtins-typed-array-gen.cc | 7 ++ deps/v8/src/compiler/code-assembler.h | 3 + deps/v8/src/objects/js-array-buffer-inl.h | 89 +++++++++---------- deps/v8/src/objects/js-array-buffer.h | 15 ++-- deps/v8/src/objects/js-array-buffer.tq | 2 +- .../objects/objects-body-descriptors-inl.h | 2 + deps/v8/src/sandbox/external-pointer-table.h | 2 +- deps/v8/src/snapshot/deserializer.cc | 1 + .../cctest/heap/test-array-buffer-tracker.cc | 8 +- 11 files changed, 71 insertions(+), 63 deletions(-) diff --git a/common.gypi b/common.gypi index 0253b99242e74e..b3718cff39effb 100644 --- a/common.gypi +++ b/common.gypi @@ -36,7 +36,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.10', + 'v8_embedder_string': '-node.11', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/include/v8-internal.h b/deps/v8/include/v8-internal.h index a52d066c835e2d..5e8b1e47f855b4 100644 --- a/deps/v8/include/v8-internal.h +++ b/deps/v8/include/v8-internal.h @@ -398,7 +398,8 @@ constexpr uint64_t kAllExternalPointerTypeTags[] = { V(kWasmInternalFunctionCallTargetTag, sandboxed, TAG(17)) \ V(kWasmTypeInfoNativeTypeTag, sandboxed, TAG(18)) \ V(kWasmExportedFunctionDataSignatureTag, sandboxed, TAG(19)) \ - V(kWasmContinuationJmpbufTag, sandboxed, TAG(20)) + V(kWasmContinuationJmpbufTag, sandboxed, TAG(20)) \ + V(kArrayBufferExtensionTag, sandboxed, TAG(21)) // All external pointer tags. #define ALL_EXTERNAL_POINTER_TAGS(V) \ diff --git a/deps/v8/src/builtins/builtins-typed-array-gen.cc b/deps/v8/src/builtins/builtins-typed-array-gen.cc index 4d2f0b541b7b50..e24a4996fd90fb 100644 --- a/deps/v8/src/builtins/builtins-typed-array-gen.cc +++ b/deps/v8/src/builtins/builtins-typed-array-gen.cc @@ -68,8 +68,15 @@ TNode TypedArrayBuiltinsAssembler::AllocateEmptyOnHeapBuffer( UintPtrConstant(0)); StoreSandboxedPointerToObject(buffer, JSArrayBuffer::kBackingStoreOffset, EmptyBackingStoreBufferConstant()); +#ifdef V8_COMPRESS_POINTERS + // When pointer compression is enabled, the extension slot contains a + // (lazily-initialized) external pointer handle. + StoreObjectFieldNoWriteBarrier(buffer, JSArrayBuffer::kExtensionOffset, + ExternalPointerHandleNullConstant()); +#else StoreObjectFieldNoWriteBarrier(buffer, JSArrayBuffer::kExtensionOffset, IntPtrConstant(0)); +#endif for (int offset = JSArrayBuffer::kHeaderSize; offset < JSArrayBuffer::kSizeWithEmbedderFields; offset += kTaggedSize) { // TODO(v8:10391, saelo): Handle external pointers in EmbedderDataSlot diff --git a/deps/v8/src/compiler/code-assembler.h b/deps/v8/src/compiler/code-assembler.h index 66dbe828536a5e..d7102ad6eb5d42 100644 --- a/deps/v8/src/compiler/code-assembler.h +++ b/deps/v8/src/compiler/code-assembler.h @@ -552,6 +552,9 @@ class V8_EXPORT_PRIVATE CodeAssembler { TNode BoolConstant(bool value) { return value ? Int32TrueConstant() : Int32FalseConstant(); } + TNode ExternalPointerHandleNullConstant() { + return ReinterpretCast(Uint32Constant(0)); + } bool IsMapOffsetConstant(Node* node); diff --git a/deps/v8/src/objects/js-array-buffer-inl.h b/deps/v8/src/objects/js-array-buffer-inl.h index d30fd968400cc3..bb59ca00a43a08 100644 --- a/deps/v8/src/objects/js-array-buffer-inl.h +++ b/deps/v8/src/objects/js-array-buffer-inl.h @@ -79,65 +79,60 @@ void JSArrayBuffer::SetBackingStoreRefForSerialization(uint32_t ref) { ArrayBufferExtension* JSArrayBuffer::extension() const { #if V8_COMPRESS_POINTERS - // With pointer compression the extension-field might not be - // pointer-aligned. However on ARM64 this field needs to be aligned to - // perform atomic operations on it. Therefore we split the pointer into two - // 32-bit words that we update atomically. We don't have an ABA problem here - // since there can never be an Attach() after Detach() (transitions only - // from NULL --> some ptr --> NULL). - - // Synchronize with publishing release store of non-null extension - uint32_t lo = base::AsAtomic32::Acquire_Load(extension_lo()); - if (lo & kUninitializedTagMask) return nullptr; - - // Synchronize with release store of null extension - uint32_t hi = base::AsAtomic32::Acquire_Load(extension_hi()); - uint32_t verify_lo = base::AsAtomic32::Relaxed_Load(extension_lo()); - if (lo != verify_lo) return nullptr; - - uintptr_t address = static_cast(lo); - address |= static_cast(hi) << 32; - return reinterpret_cast(address); + // We need Acquire semantics here when loading the entry, see below. + // Consider adding respective external pointer accessors if non-relaxed + // ordering semantics are ever needed in other places as well. + Isolate* isolate = GetIsolateFromWritableObject(*this); + ExternalPointerHandle handle = + base::AsAtomic32::Acquire_Load(extension_handle_location()); + return reinterpret_cast( + isolate->external_pointer_table().Get(handle, kArrayBufferExtensionTag)); #else - return base::AsAtomicPointer::Acquire_Load(extension_location()); -#endif + return base::AsAtomicPointer::Acquire_Load(extension_location()); +#endif // V8_COMPRESS_POINTERS } void JSArrayBuffer::set_extension(ArrayBufferExtension* extension) { #if V8_COMPRESS_POINTERS - if (extension != nullptr) { - uintptr_t address = reinterpret_cast(extension); - base::AsAtomic32::Relaxed_Store(extension_hi(), - static_cast(address >> 32)); - base::AsAtomic32::Release_Store(extension_lo(), - static_cast(address)); - } else { - base::AsAtomic32::Relaxed_Store(extension_lo(), - 0 | kUninitializedTagMask); - base::AsAtomic32::Release_Store(extension_hi(), 0); - } + if (extension != nullptr) { + Isolate* isolate = GetIsolateFromWritableObject(*this); + ExternalPointerTable& table = isolate->external_pointer_table(); + + // The external pointer handle for the extension is initialized lazily and + // so has to be zero here since, once set, the extension field can only be + // cleared, but not changed. + DCHECK_EQ(0, base::AsAtomic32::Relaxed_Load(extension_handle_location())); + + // We need Release semantics here, see above. + ExternalPointerHandle handle = table.AllocateAndInitializeEntry( + isolate, reinterpret_cast
(extension), + kArrayBufferExtensionTag); + base::AsAtomic32::Release_Store(extension_handle_location(), handle); + } else { + // This special handling of nullptr is required as it is used to initialize + // the slot, but is also beneficial when an ArrayBuffer is detached as it + // allows the external pointer table entry to be reclaimed while the + // ArrayBuffer is still alive. + base::AsAtomic32::Release_Store(extension_handle_location(), + kNullExternalPointerHandle); + } #else - base::AsAtomicPointer::Release_Store(extension_location(), extension); -#endif - WriteBarrier::Marking(*this, extension); -} - -ArrayBufferExtension** JSArrayBuffer::extension_location() const { - Address location = field_address(kExtensionOffset); - return reinterpret_cast(location); + base::AsAtomicPointer::Release_Store(extension_location(), extension); +#endif // V8_COMPRESS_POINTERS + WriteBarrier::Marking(*this, extension); } #if V8_COMPRESS_POINTERS -uint32_t* JSArrayBuffer::extension_lo() const { +ExternalPointerHandle* JSArrayBuffer::extension_handle_location() const { Address location = field_address(kExtensionOffset); - return reinterpret_cast(location); + return reinterpret_cast(location); } - -uint32_t* JSArrayBuffer::extension_hi() const { - Address location = field_address(kExtensionOffset) + sizeof(uint32_t); - return reinterpret_cast(location); +#else +ArrayBufferExtension** JSArrayBuffer::extension_location() const { + Address location = field_address(kExtensionOffset); + return reinterpret_cast(location); } -#endif +#endif // V8_COMPRESS_POINTERS void JSArrayBuffer::clear_padding() { if (FIELD_SIZE(kOptionalPaddingOffset) != 0) { diff --git a/deps/v8/src/objects/js-array-buffer.h b/deps/v8/src/objects/js-array-buffer.h index ad6bc4fbf66511..3b1c06f90c27a5 100644 --- a/deps/v8/src/objects/js-array-buffer.h +++ b/deps/v8/src/objects/js-array-buffer.h @@ -160,14 +160,15 @@ class JSArrayBuffer class BodyDescriptor; private: - inline ArrayBufferExtension** extension_location() const; - #if V8_COMPRESS_POINTERS - static const int kUninitializedTagMask = 1; - - inline uint32_t* extension_lo() const; - inline uint32_t* extension_hi() const; -#endif + // When pointer compression is enabled, the pointer to the extension is + // stored in the external pointer table and the object itself only contains a + // 32-bit external pointer handles. This simplifies alignment requirements + // and is also necessary for the sandbox. + inline ExternalPointerHandle* extension_handle_location() const; +#else + inline ArrayBufferExtension** extension_location() const; +#endif // V8_COMPRESS_POINTERS TQ_OBJECT_CONSTRUCTORS(JSArrayBuffer) }; diff --git a/deps/v8/src/objects/js-array-buffer.tq b/deps/v8/src/objects/js-array-buffer.tq index 415b15d5c56fbc..004468caeebf6e 100644 --- a/deps/v8/src/objects/js-array-buffer.tq +++ b/deps/v8/src/objects/js-array-buffer.tq @@ -18,7 +18,7 @@ extern class JSArrayBuffer extends JSObjectWithEmbedderSlots { raw_max_byte_length: uintptr; // A SandboxedPtr if the sandbox is enabled backing_store: RawPtr; - extension: RawPtr; + extension: ExternalPointer; bit_field: JSArrayBufferFlags; // Pads header size to be a multiple of kTaggedSize. @if(TAGGED_SIZE_8_BYTES) optional_padding: uint32; diff --git a/deps/v8/src/objects/objects-body-descriptors-inl.h b/deps/v8/src/objects/objects-body-descriptors-inl.h index 4496fdedcf20c7..d0e2e95e19c3fd 100644 --- a/deps/v8/src/objects/objects-body-descriptors-inl.h +++ b/deps/v8/src/objects/objects-body-descriptors-inl.h @@ -387,6 +387,8 @@ class JSArrayBuffer::BodyDescriptor final : public BodyDescriptorBase { // JSArrayBuffer instances contain raw data that the GC does not know about. IteratePointers(obj, kPropertiesOrHashOffset, kEndOfTaggedFieldsOffset, v); IterateJSObjectBodyImpl(map, obj, kHeaderSize, object_size, v); + v->VisitExternalPointer(map, obj.RawExternalPointerField(kExtensionOffset), + kArrayBufferExtensionTag); } static inline int SizeOf(Map map, HeapObject object) { diff --git a/deps/v8/src/sandbox/external-pointer-table.h b/deps/v8/src/sandbox/external-pointer-table.h index eb76c35e54fc66..d76bd771857cf4 100644 --- a/deps/v8/src/sandbox/external-pointer-table.h +++ b/deps/v8/src/sandbox/external-pointer-table.h @@ -98,7 +98,7 @@ class V8_EXPORT_PRIVATE ExternalPointerTable { // returning the previous value. The same tag is applied both to decode the // previous value and encode the given value. // - // This method is atomic and can call be called from background threads. + // This method is atomic and can be called from background threads. inline Address Exchange(ExternalPointerHandle handle, Address value, ExternalPointerTag tag); diff --git a/deps/v8/src/snapshot/deserializer.cc b/deps/v8/src/snapshot/deserializer.cc index d6592a5eeb0503..3dcb340c858263 100644 --- a/deps/v8/src/snapshot/deserializer.cc +++ b/deps/v8/src/snapshot/deserializer.cc @@ -397,6 +397,7 @@ void Deserializer::PostProcessNewJSReceiver( auto buffer = JSArrayBuffer::cast(*obj); uint32_t store_index = buffer.GetBackingStoreRefForDeserialization(); if (store_index == kEmptyBackingStoreRefSentinel) { + buffer.set_extension(nullptr); buffer.set_backing_store(main_thread_isolate(), EmptyBackingStoreBuffer()); } else { diff --git a/deps/v8/test/cctest/heap/test-array-buffer-tracker.cc b/deps/v8/test/cctest/heap/test-array-buffer-tracker.cc index bcdb1415389da3..6d5da43a91bd96 100644 --- a/deps/v8/test/cctest/heap/test-array-buffer-tracker.cc +++ b/deps/v8/test/cctest/heap/test-array-buffer-tracker.cc @@ -218,7 +218,6 @@ TEST(ArrayBuffer_NonLivePromotion) { v8::Isolate* isolate = env->GetIsolate(); Heap* heap = reinterpret_cast(isolate)->heap(); - JSArrayBuffer raw_ab; { v8::HandleScope handle_scope(isolate); Handle root = @@ -235,13 +234,12 @@ TEST(ArrayBuffer_NonLivePromotion) { CHECK(IsTracked(heap, JSArrayBuffer::cast(root->get(0)))); heap::GcAndSweep(heap, NEW_SPACE); CHECK(IsTracked(heap, JSArrayBuffer::cast(root->get(0)))); - raw_ab = JSArrayBuffer::cast(root->get(0)); + ArrayBufferExtension* extension = + JSArrayBuffer::cast(root->get(0)).extension(); root->set(0, ReadOnlyRoots(heap).undefined_value()); heap::SimulateIncrementalMarking(heap, true); - // Prohibit page from being released. - Page::FromHeapObject(raw_ab)->MarkNeverEvacuate(); heap::GcAndSweep(heap, OLD_SPACE); - CHECK(!IsTracked(heap, raw_ab)); + CHECK(!IsTracked(heap, extension)); } }