diff --git a/src/heap/marking-visitor-inl.h b/src/heap/marking-visitor-inl.h index 83a64b284c..5e83e9e269 100644 --- a/src/heap/marking-visitor-inl.h +++ b/src/heap/marking-visitor-inl.h @@ -146,7 +146,7 @@ void MarkingVisitorBase::VisitExternalPointer( HeapObject host, ExternalPointerSlot slot, ExternalPointerTag tag) { #ifdef V8_ENABLE_SANDBOX if (IsSandboxedExternalPointerType(tag)) { - ExternalPointerHandle handle = slot.load_handle(); + ExternalPointerHandle handle = slot.Relaxed_LoadHandle(); if (IsSharedExternalPointerType(tag)) { shared_external_pointer_table_->Mark(handle); } else { diff --git a/src/objects/slots-inl.h b/src/objects/slots-inl.h index 8315914c3f..d5b609300b 100644 --- a/src/objects/slots-inl.h +++ b/src/objects/slots-inl.h @@ -161,7 +161,10 @@ void ExternalPointerSlot::init(Isolate* isolate, Address value, ExternalPointerTable& table = GetExternalPointerTableForTag(isolate, tag); ExternalPointerHandle handle = table.Allocate(); table.Set(handle, value, tag); - store_handle(handle); + // Use a Release_Store to ensure that the store of the pointer into the + // table is not reordered after the store of the handle. Otherwise, other + // threads may access an uninitialized table entry and crash. + Release_StoreHandle(handle); return; } #endif // V8_ENABLE_SANDBOX @@ -169,12 +172,23 @@ void ExternalPointerSlot::init(Isolate* isolate, Address value, } #ifdef V8_ENABLE_SANDBOX -ExternalPointerHandle ExternalPointerSlot::load_handle() const { - return base::Memory(address()); +ExternalPointerHandle ExternalPointerSlot::Relaxed_LoadHandle() const { + // TODO(saelo): here and below: remove cast once ExternalPointerHandle is + // always 32 bit large. + auto handle_location = reinterpret_cast(location()); + return base::AsAtomic32::Relaxed_Load(handle_location); } -void ExternalPointerSlot::store_handle(ExternalPointerHandle handle) const { - base::Memory(address()) = handle; +void ExternalPointerSlot::Relaxed_StoreHandle( + ExternalPointerHandle handle) const { + auto handle_location = reinterpret_cast(location()); + return base::AsAtomic32::Relaxed_Store(handle_location, handle); +} + +void ExternalPointerSlot::Release_StoreHandle( + ExternalPointerHandle handle) const { + auto handle_location = reinterpret_cast(location()); + return base::AsAtomic32::Release_Store(handle_location, handle); } #endif // V8_ENABLE_SANDBOX @@ -184,7 +198,8 @@ Address ExternalPointerSlot::load(const Isolate* isolate, if (IsSandboxedExternalPointerType(tag)) { const ExternalPointerTable& table = GetExternalPointerTableForTag(isolate, tag); - return table.Get(load_handle(), tag); + ExternalPointerHandle handle = Relaxed_LoadHandle(); + return table.Get(handle, tag); } #endif // V8_ENABLE_SANDBOX return ReadMaybeUnalignedValue
(address()); @@ -195,7 +210,8 @@ void ExternalPointerSlot::store(Isolate* isolate, Address value, #ifdef V8_ENABLE_SANDBOX if (IsSandboxedExternalPointerType(tag)) { ExternalPointerTable& table = GetExternalPointerTableForTag(isolate, tag); - table.Set(load_handle(), value, tag); + ExternalPointerHandle handle = Relaxed_LoadHandle(); + table.Set(handle, value, tag); return; } #endif // V8_ENABLE_SANDBOX diff --git a/src/objects/slots.h b/src/objects/slots.h index c7f1737ccf..13a13bb469 100644 --- a/src/objects/slots.h +++ b/src/objects/slots.h @@ -299,8 +299,11 @@ class ExternalPointerSlot // entry in an ExternalPointerTable. These methods allow access to the // underlying handle while the load/store methods below resolve the handle to // the real pointer. - inline ExternalPointerHandle load_handle() const; - inline void store_handle(ExternalPointerHandle handle) const; + // Handles should generally be accessed atomically as they may be accessed + // from other threads, for example GC marking threads. + inline ExternalPointerHandle Relaxed_LoadHandle() const; + inline void Relaxed_StoreHandle(ExternalPointerHandle handle) const; + inline void Release_StoreHandle(ExternalPointerHandle handle) const; #endif // V8_ENABLE_SANDBOX inline Address load(const Isolate* isolate, ExternalPointerTag tag); diff --git a/src/sandbox/external-pointer-inl.h b/src/sandbox/external-pointer-inl.h index 94000f1fdb..b3d8cf5179 100644 --- a/src/sandbox/external-pointer-inl.h +++ b/src/sandbox/external-pointer-inl.h @@ -6,6 +6,7 @@ #define V8_SANDBOX_EXTERNAL_POINTER_INL_H_ #include "include/v8-internal.h" +#include "src/base/atomic-utils.h" #include "src/execution/isolate.h" #include "src/sandbox/external-pointer-table-inl.h" #include "src/sandbox/external-pointer.h" @@ -43,7 +44,11 @@ V8_INLINE void InitExternalPointerField(Address field_address, Isolate* isolate, ExternalPointerTable& table = GetExternalPointerTable(isolate); ExternalPointerHandle handle = table.Allocate(); table.Set(handle, value, tag); - base::Memory(field_address) = handle; + // Use a Release_Store to ensure that the store of the pointer into the + // table is not reordered after the store of the handle. Otherwise, other + // threads may access an uninitialized table entry and crash. + auto location = reinterpret_cast(field_address); + base::AsAtomic32::Release_Store(location, handle); return; } #endif // V8_ENABLE_SANDBOX @@ -55,8 +60,12 @@ V8_INLINE Address ReadExternalPointerField(Address field_address, const Isolate* isolate) { #ifdef V8_ENABLE_SANDBOX if (IsSandboxedExternalPointerType(tag)) { - ExternalPointerHandle handle = - base::Memory(field_address); + // Handles may be written to objects from other threads so the handle needs + // to be loaded atomically. We assume that the load from the table cannot + // be reordered before the load of the handle due to the data dependency + // between the two loads and therefore use relaxed memory ordering. + auto location = reinterpret_cast(field_address); + ExternalPointerHandle handle = base::AsAtomic32::Relaxed_Load(location); return GetExternalPointerTable(isolate).Get(handle, tag); } #endif // V8_ENABLE_SANDBOX @@ -68,8 +77,9 @@ V8_INLINE void WriteExternalPointerField(Address field_address, Isolate* isolate, Address value) { #ifdef V8_ENABLE_SANDBOX if (IsSandboxedExternalPointerType(tag)) { - ExternalPointerHandle handle = - base::Memory(field_address); + // See comment above for why this is a Relaxed_Load. + auto location = reinterpret_cast(field_address); + ExternalPointerHandle handle = base::AsAtomic32::Relaxed_Load(location); GetExternalPointerTable(isolate).Set(handle, value, tag); return; }