[sandbox] Store external pointers in EmbedderDataSlots in shifted form

Similar to other external pointers, the indices into the external
pointer table are stored shifted to the left to guarantee an upper
bound.

Bug: v8:10391
Change-Id: I079dc1568f49ae349c326a8e83fc32c93bdb35cf
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/+/3455152Reviewed-by: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Samuel Groß <saelo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79209}

include/v8-context.h

@@ -368,17 +368,17 @@ Local<Value> Context::GetEmbedderData(int index) {
 }
 
 void* Context::GetAlignedPointerFromEmbedderData(int index) {
-  using I = internal::Internals;
-  static_assert(I::kEmbedderDataSlotSize == internal::kApiSystemPointerSize,
-                "Enable fast path with sandboxed external pointers enabled "
-                "once embedder data slots are 32 bits large");
-#if !defined(V8_ENABLE_CHECKS) && !defined(V8_SANDBOXED_EXTERNAL_POINTERS)
+#if !defined(V8_ENABLE_CHECKS)
   using A = internal::Address;
+  using I = internal::Internals;
   A ctx = *reinterpret_cast<const A*>(this);
   A embedder_data =
       I::ReadTaggedPointerField(ctx, I::kNativeContextEmbedderDataOffset);
   int value_offset =
       I::kEmbedderDataArrayHeaderSize + (I::kEmbedderDataSlotSize * index);
+#ifdef V8_SANDBOXED_EXTERNAL_POINTERS
+  value_offset += I::kEmbedderDataSlotRawPayloadOffset;
+#endif
   internal::Isolate* isolate = I::GetIsolateForSandbox(ctx);
   return reinterpret_cast<void*>(
       I::ReadExternalPointerField(isolate, embedder_data, value_offset,

include/v8-object.h

@@ -735,18 +735,18 @@ Local<Value> Object::GetInternalField(int index) {
 }
 
 void* Object::GetAlignedPointerFromInternalField(int index) {
-  using I = internal::Internals;
-  static_assert(I::kEmbedderDataSlotSize == internal::kApiSystemPointerSize,
-                "Enable fast path with sandboxed external pointers enabled "
-                "once embedder data slots are 32 bits large");
-#if !defined(V8_ENABLE_CHECKS) && !defined(V8_SANDBOXED_EXTERNAL_POINTERS)
+#if !defined(V8_ENABLE_CHECKS)
   using A = internal::Address;
+  using I = internal::Internals;
   A obj = *reinterpret_cast<A*>(this);
   // Fast path: If the object is a plain JSObject, which is the common case, we
   // know where to find the internal fields and can return the value directly.
   auto instance_type = I::GetInstanceType(obj);
   if (v8::internal::CanHaveInternalField(instance_type)) {
     int offset = I::kJSObjectHeaderSize + (I::kEmbedderDataSlotSize * index);
+#ifdef V8_SANDBOXED_EXTERNAL_POINTERS
+    offset += I::kEmbedderDataSlotRawPayloadOffset;
+#endif
     internal::Isolate* isolate = I::GetIsolateForSandbox(obj);
     A value = I::ReadExternalPointerField(
         isolate, obj, offset, internal::kEmbedderDataSlotPayloadTag);

src/heap/factory.cc

@@ -527,10 +527,6 @@ Handle<EmbedderDataArray> Factory::NewEmbedderDataArray(int length) {
     ObjectSlot end(array.slots_end());
     size_t slot_count = end - start;
     MemsetTagged(start, *undefined_value(), slot_count);
-    for (int i = 0; i < length; i++) {
-      // TODO(v8:10391, saelo): Handle external pointers in EmbedderDataSlot
-      EmbedderDataSlot(array, i).AllocateExternalPointerEntry(isolate());
-    }
   }
   return handle(array, isolate());
 }

src/heap/marking-visitor.h

@@ -222,10 +222,11 @@ class MarkingVisitorBase : public HeapVisitor<int, ConcreteVisitor> {
 #ifdef V8_SANDBOXED_EXTERNAL_POINTERS
     // When sandboxed external pointers are enabled, EmbedderDataSlots may
     // contain an external pointer, which must be marked as alive.
-    uint32_t maybe_index = base::Relaxed_Load(reinterpret_cast<base::Atomic32*>(
-        slot.address() + EmbedderDataSlot::kRawPayloadOffset));
-    if (external_pointer_table_->IsValidIndex(maybe_index)) {
-      external_pointer_table_->Mark(maybe_index);
+    base::Atomic32* ptr = reinterpret_cast<base::Atomic32*>(
+        slot.address() + EmbedderDataSlot::kRawPayloadOffset);
+    uint32_t index = base::Relaxed_Load(ptr) >> kExternalPointerIndexShift;
+    if (external_pointer_table_->IsValidIndex(index)) {
+      external_pointer_table_->Mark(index);
     }
 #endif  // V8_SANDBOXED_EXTERNAL_POINTERS
   }

src/objects/embedder-data-slot-inl.h

@@ -12,6 +12,7 @@
 #include "src/objects/embedder-data-slot.h"
 #include "src/objects/js-objects-inl.h"
 #include "src/objects/objects-inl.h"
+#include "src/sandbox/external-pointer-inl.h"
 
 // Has to be the last include (doesn't have include guards):
 #include "src/objects/object-macros.h"
@@ -30,19 +31,6 @@ EmbedderDataSlot::EmbedderDataSlot(JSObject object, int embedder_field_index)
 EmbedderDataSlot::EmbedderDataSlot(const EmbedderDataSlotSnapshot& snapshot)
     : SlotBase(reinterpret_cast<Address>(&snapshot)) {}
 
-void EmbedderDataSlot::AllocateExternalPointerEntry(Isolate* isolate) {
-#ifdef V8_SANDBOXED_EXTERNAL_POINTERS
-  // TODO(v8:10391, saelo): Use InitExternalPointerField() once
-  // ExternalPointer_t is 4-bytes.
-  uint32_t index = isolate->external_pointer_table().Allocate();
-  // Object slots don't support storing raw values, so we just "reinterpret
-  // cast" the index value to Object.
-  Object index_as_object(index);
-  ObjectSlot(address() + kRawPayloadOffset).Relaxed_Store(index_as_object);
-  ObjectSlot(address() + kTaggedPayloadOffset).Relaxed_Store(Smi::zero());
-#endif
-}
-
 Object EmbedderDataSlot::load_tagged() const {
   return ObjectSlot(address() + kTaggedPayloadOffset).Relaxed_Load();
 }
@@ -89,12 +77,14 @@ bool EmbedderDataSlot::ToAlignedPointer(Isolate* isolate,
   // are accessed this way only from the main thread via API during "mutator"
   // phase which is propely synched with GC (concurrent marker may still look
   // at the tagged part of the embedder slot but read-only access is ok).
-  Address raw_value;
 #ifdef V8_SANDBOXED_EXTERNAL_POINTERS
-  uint32_t index = base::Memory<uint32_t>(address() + kRawPayloadOffset);
-  raw_value =
-      isolate->external_pointer_table().Get(index, kEmbedderDataSlotPayloadTag);
+  // This might crash if the value is not a valid index.
+  *out_pointer = reinterpret_cast<void*>(ReadExternalPointerField(
+      address() + kRawPayloadOffset, isolate, kEmbedderDataSlotPayloadTag));
+  // We don't actually care if the pointer is aligned or not...
+  return true;
 #else
+  Address raw_value;
   if (COMPRESS_POINTERS_BOOL) {
     // TODO(ishell, v8:8875): When pointer compression is enabled 8-byte size
     // fields (external pointers, doubles and BigInt data) are only kTaggedSize
@@ -104,15 +94,17 @@ bool EmbedderDataSlot::ToAlignedPointer(Isolate* isolate,
   } else {
     raw_value = *location();
   }
-#endif
   *out_pointer = reinterpret_cast<void*>(raw_value);
   return HAS_SMI_TAG(raw_value);
+#endif  // V8_SANDBOXED_EXTERNAL_POINTERS
 }
 
 bool EmbedderDataSlot::ToAlignedPointerSafe(Isolate* isolate,
                                             void** out_pointer) const {
 #ifdef V8_SANDBOXED_EXTERNAL_POINTERS
   uint32_t index = base::Memory<uint32_t>(address() + kRawPayloadOffset);
+  if (!HAS_SMI_TAG(index)) return false;
+  index >>= kExternalPointerIndexShift;
   Address raw_value;
   if (isolate->external_pointer_table().IsValidIndex(index)) {
     raw_value = isolate->external_pointer_table().Get(
@@ -137,22 +129,16 @@ bool EmbedderDataSlot::store_aligned_pointer(Isolate* isolate, void* ptr) {
   Address value = reinterpret_cast<Address>(ptr);
   if (!HAS_SMI_TAG(value)) return false;
 #ifdef V8_SANDBOXED_EXTERNAL_POINTERS
-  if (V8_SANDBOXED_EXTERNAL_POINTERS_BOOL) {
-    DCHECK_EQ(0, value & kExternalPointerTagMask);
-    AllocateExternalPointerEntry(isolate);
-    // Raw payload contains the table index. Object slots don't support loading
-    // of raw values, so we just "reinterpret cast" Object value to index.
-    Object index_as_object =
-        ObjectSlot(address() + kRawPayloadOffset).Relaxed_Load();
-    uint32_t index = static_cast<uint32_t>(index_as_object.ptr());
-    // This also mark the entry as alive until the next GC.
-    isolate->external_pointer_table().Set(index, value,
-                                          kEmbedderDataSlotPayloadTag);
-    return true;
-  }
-#endif
+  DCHECK_EQ(0, value & kExternalPointerTagMask);
+  // This also mark the entry as alive until the next GC.
+  InitExternalPointerField(address() + kRawPayloadOffset, isolate, value,
+                           kEmbedderDataSlotPayloadTag);
+  ObjectSlot(address() + kTaggedPayloadOffset).Relaxed_Store(Smi::zero());
+  return true;
+#else
   gc_safe_store(isolate, value);
   return true;
+#endif  // V8_SANDBOXED_EXTERNAL_POINTERS
 }
 
 EmbedderDataSlot::RawData EmbedderDataSlot::load_raw(

src/objects/embedder-data-slot.h

@@ -62,8 +62,6 @@ class EmbedderDataSlot
   // Opaque type used for storing raw embedder data.
   using RawData = Address;
 
-  V8_INLINE void AllocateExternalPointerEntry(Isolate* isolate);
-
   V8_INLINE Object load_tagged() const;
   V8_INLINE void store_smi(Smi value);
 

src/objects/js-objects-inl.h

@@ -308,10 +308,6 @@ int JSObject::GetEmbedderFieldOffset(int index) {
   return GetEmbedderFieldsStartOffset() + (kEmbedderDataSlotSize * index);
 }
 
-void JSObject::InitializeEmbedderField(Isolate* isolate, int index) {
-  EmbedderDataSlot(*this, index).AllocateExternalPointerEntry(isolate);
-}
-
 Object JSObject::GetEmbedderField(int index) {
   return EmbedderDataSlot(*this, index).load_tagged();
 }

src/objects/js-objects.h

@@ -611,7 +611,6 @@ class JSObject : public TorqueGeneratedJSObject<JSObject, JSReceiver> {
   static inline int GetEmbedderFieldCount(Map map);
   inline int GetEmbedderFieldCount() const;
   inline int GetEmbedderFieldOffset(int index);
-  inline void InitializeEmbedderField(Isolate* isolate, int index);
   inline Object GetEmbedderField(int index);
   inline void SetEmbedderField(int index, Object value);
   inline void SetEmbedderField(int index, Smi value);