[sandbox] Atomically load/store ExternalPointerHandles

Since those are accessed from background threads during marking, they
should generally be loaded and stored using atomic operations. Further,
when an external pointer slot is initialized, the handle should be
stored using release semantics to prevent reordering of the store into
the pointer table after the store of the handle to the object.

Bug: v8:10391, v8:13156
Change-Id: I5c33b4e791482f84e2770cd047a11f5762a0aa65
Cq-Include-Trybots: luci.v8.try:v8_linux64_heap_sandbox_dbg_ng,v8_linux_arm64_sim_heap_sandbox_dbg_ng
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3812035Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Samuel Groß <saelo@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82280}

src/heap/marking-visitor-inl.h

@@ -146,7 +146,7 @@ void MarkingVisitorBase<ConcreteVisitor, MarkingState>::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 {

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<ExternalPointerHandle>(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<ExternalPointerHandle*>(location());
+  return base::AsAtomic32::Relaxed_Load(handle_location);
 }
 
-void ExternalPointerSlot::store_handle(ExternalPointerHandle handle) const {
-  base::Memory<ExternalPointerHandle>(address()) = handle;
+void ExternalPointerSlot::Relaxed_StoreHandle(
+    ExternalPointerHandle handle) const {
+  auto handle_location = reinterpret_cast<ExternalPointerHandle*>(location());
+  return base::AsAtomic32::Relaxed_Store(handle_location, handle);
+}
+
+void ExternalPointerSlot::Release_StoreHandle(
+    ExternalPointerHandle handle) const {
+  auto handle_location = reinterpret_cast<ExternalPointerHandle*>(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>(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

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);

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<tag>(isolate);
     ExternalPointerHandle handle = table.Allocate();
     table.Set(handle, value, tag);
-    base::Memory<ExternalPointerHandle>(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<ExternalPointerHandle*>(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<ExternalPointerHandle>(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<ExternalPointerHandle*>(field_address);
+    ExternalPointerHandle handle = base::AsAtomic32::Relaxed_Load(location);
     return GetExternalPointerTable<tag>(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<ExternalPointerHandle>(field_address);
+    // See comment above for why this is a Relaxed_Load.
+    auto location = reinterpret_cast<ExternalPointerHandle*>(field_address);
+    ExternalPointerHandle handle = base::AsAtomic32::Relaxed_Load(location);
     GetExternalPointerTable<tag>(isolate).Set(handle, value, tag);
     return;
   }