[sandbox] Forbid double-initialization of ExternalPointerSlots

Double initialization may cause the ExternalPointerTable compaction
algorithm to behave non-optimally: Consider the case of an Entry E1 that
is owned by a HeapObject O and is marked for evacuation during GC
marking. In that case, a new entry E2 is allocated for it, and during
sweeping, E1 will be evacuated into E2 and the Handle in O updated to
point to E2. However, if a new entry E3 for O is allocated before
sweeping, then during sweeping E3 (instead of E1) will be moved into E2.
This may then violate the invariant that the compaction algorithms
always evacuates an entry out of the evacuation area.

This CL therefore forbids double initializaiton of external pointer
slots and adds DCHECKs to attempt to catch these in debug builds.

Bug: v8:10391
Change-Id: I128dc930e8b3f863dab18ba648f34d68d8cb276b
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/+/3856563Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Samuel Groß <saelo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82729}

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

@@ -115,8 +115,11 @@ bool EmbedderDataSlot::store_aligned_pointer(Isolate* isolate, void* ptr) {
   if (!HAS_SMI_TAG(value)) return false;
 #ifdef V8_SANDBOXED_EXTERNAL_POINTERS
   DCHECK_EQ(0, value & kExternalPointerTagMask);
-  // This also mark the entry as alive until the next GC.
-  InitExternalPointerField<kEmbedderDataSlotPayloadTag>(
+  // When the sandbox is enabled, the external pointer handles in
+  // EmbedderDataSlots are lazily initialized: initially they contain the null
+  // external pointer handle (see EmbedderDataSlot::Initialize), and only once
+  // an external pointer is stored in them are they properly initialized.
+  WriteLazilyInitializedExternalPointerField<kEmbedderDataSlotPayloadTag>(
       address() + kExternalPointerOffset, isolate, value);
   ObjectSlot(address() + kTaggedPayloadOffset).Relaxed_Store(Smi::zero());
   return true;

src/objects/slots-inl.h

@@ -158,6 +158,11 @@ void ExternalPointerSlot::init(Isolate* isolate, Address value,
                                ExternalPointerTag tag) {
 #ifdef V8_ENABLE_SANDBOX
   if (IsSandboxedExternalPointerType(tag)) {
+    // Re-initialization of external pointer slots is forbidden as it would
+    // interfere with table compaction. See the explanation of the table
+    // compaction algorithm in external-poiner-table.h.
+    DCHECK(IsUninitializedExternalPointerFieldInDebugBuilds(address()));
+
     ExternalPointerTable& table = GetExternalPointerTableForTag(isolate, tag);
     ExternalPointerHandle handle = table.AllocateAndInitializeEntry(value, tag);
     // Use a Release_Store to ensure that the store of the pointer into the

src/objects/string-inl.h

@@ -1100,6 +1100,19 @@ bool ExternalString::is_uncached() const {
   return (type & kUncachedExternalStringMask) == kUncachedExternalStringTag;
 }
 
+void ExternalString::ClearExternalPointerFields() {
+  // Clearing external pointer fields prior to initialization is only required
+  // in debug builds.
+#if DEBUG && V8_ENABLE_SANDBOX
+  WriteField<ExternalPointerHandle>(kResourceOffset,
+                                    kNullExternalPointerHandle);
+  if (!is_uncached()) {
+    WriteField<ExternalPointerHandle>(kResourceDataOffset,
+                                      kNullExternalPointerHandle);
+  }
+#endif  // DEBUG && V8_ENABLE_SANDBOX
+}
+
 void ExternalString::InitExternalPointerFields(Isolate* isolate) {
   InitExternalPointerField<kExternalStringResourceTag>(kResourceOffset, isolate,
                                                        kNullAddress);

src/objects/string.cc

@@ -309,6 +309,9 @@ bool String::MakeExternal(v8::String::ExternalStringResource* resource) {
   this->set_map(new_map, kReleaseStore);
 
   ExternalTwoByteString self = ExternalTwoByteString::cast(*this);
+  // Need to clear external pointer fields before initializing them here.
+  // See the comment for ClearExternalPointerFields.
+  self.ClearExternalPointerFields();
   self.InitExternalPointerFields(isolate);
   self.SetResource(isolate, resource);
   isolate->heap()->RegisterExternalString(*this);
@@ -393,6 +396,9 @@ bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) {
   this->set_map(new_map, kReleaseStore);
 
   ExternalOneByteString self = ExternalOneByteString::cast(*this);
+  // Need to clear external pointer fields before initializing them here.
+  // See the comment for ClearExternalPointerFields.
+  self.ClearExternalPointerFields();
   self.InitExternalPointerFields(isolate);
   self.SetResource(isolate, resource);
   isolate->heap()->RegisterExternalString(*this);

src/objects/string.h

@@ -910,6 +910,12 @@ class ExternalString
   static const int kUncachedSize =
       kResourceOffset + FIELD_SIZE(kResourceOffset);
 
+  // When converting a String to an ExternalString, the slots that now contain
+  // external pointers may have previously contained arbitrary data. This may
+  // incorrectly trigger the external-pointer-double-initialization checker,
+  // so this method can be used to clear the fields first.
+  inline void ClearExternalPointerFields();
+
   inline void InitExternalPointerFields(Isolate* isolate);
 
   // Return whether the external string data pointer is not cached.

src/sandbox/external-pointer-inl.h

@@ -30,11 +30,46 @@ ExternalPointerTable& GetExternalPointerTable(Isolate* isolate) {
 }
 #endif  // V8_ENABLE_SANDBOX
 
+#if DEBUG && V8_ENABLE_SANDBOX
+// Helper routines to detect double-initialization of external pointer slots.
+V8_INLINE bool IsUninitializedExternalPointerFieldInDebugBuilds(
+    Address field_address) {
+  auto MayBeInitializedExternalPointerHandle =
+      [](ExternalPointerHandle handle) constexpr {
+    uint32_t index = handle >> kExternalPointerIndexShift;
+    return index != 0 && index < kMaxExternalPointers &&
+           (index << kExternalPointerIndexShift) == handle;
+  };
+
+  // In debug builds, an uninitialized ExternalPointerSlot (on the V8 heap) will
+  // always contain one of these values.
+  static_assert(
+      !MayBeInitializedExternalPointerHandle(kNullExternalPointerHandle));
+  static_assert(!MayBeInitializedExternalPointerHandle(
+      static_cast<ExternalPointerHandle>(kZapValue)));
+  static_assert(!MayBeInitializedExternalPointerHandle(
+      static_cast<ExternalPointerHandle>(kZapValue >> 32)));
+  static_assert(!MayBeInitializedExternalPointerHandle(
+      static_cast<ExternalPointerHandle>(kClearedFreeMemoryValue)));
+  static_assert(!MayBeInitializedExternalPointerHandle(
+      static_cast<ExternalPointerHandle>(kClearedFreeMemoryValue >> 32)));
+
+  auto location = reinterpret_cast<ExternalPointerHandle*>(field_address);
+  ExternalPointerHandle handle = base::AsAtomic32::Relaxed_Load(location);
+  return !MayBeInitializedExternalPointerHandle(handle);
+}
+#endif  // DEBUG && V8_ENABLE_SANDBOX
+
 template <ExternalPointerTag tag>
 V8_INLINE void InitExternalPointerField(Address field_address, Isolate* isolate,
                                         Address value) {
 #ifdef V8_ENABLE_SANDBOX
   if (IsSandboxedExternalPointerType(tag)) {
+    // Re-initialization of external pointer slots is forbidden as it would
+    // interfere with table compaction. See the explanation of the table
+    // compaction algorithm in external-poiner-table.h.
+    DCHECK(IsUninitializedExternalPointerFieldInDebugBuilds(field_address));
+
     ExternalPointerTable& table = GetExternalPointerTable<tag>(isolate);
     ExternalPointerHandle handle = table.AllocateAndInitializeEntry(value, tag);
     // Use a Release_Store to ensure that the store of the pointer into the
@@ -80,6 +115,30 @@ V8_INLINE void WriteExternalPointerField(Address field_address,
   WriteMaybeUnalignedValue<Address>(field_address, value);
 }
 
+template <ExternalPointerTag tag>
+V8_INLINE void WriteLazilyInitializedExternalPointerField(Address field_address,
+                                                          Isolate* isolate,
+                                                          Address value) {
+#ifdef V8_ENABLE_SANDBOX
+  if (IsSandboxedExternalPointerType(tag)) {
+    // See comment above for why this uses a Relaxed_Load and Release_Store.
+    ExternalPointerTable& table = GetExternalPointerTable<tag>(isolate);
+    auto location = reinterpret_cast<ExternalPointerHandle*>(field_address);
+    ExternalPointerHandle handle = base::AsAtomic32::Relaxed_Load(location);
+    if (handle == kNullExternalPointerHandle) {
+      // Field has not been initialized yet.
+      ExternalPointerHandle handle =
+          table.AllocateAndInitializeEntry(value, tag);
+      base::AsAtomic32::Release_Store(location, handle);
+    } else {
+      table.Set(handle, value, tag);
+    }
+    return;
+  }
+#endif  // V8_ENABLE_SANDBOX
+  WriteMaybeUnalignedValue<Address>(field_address, value);
+}
+
 }  // namespace internal
 }  // namespace v8
 

src/sandbox/external-pointer-table.cc

@@ -163,6 +163,13 @@ uint32_t ExternalPointerTable::SweepAndCompact(Isolate* isolate) {
 
       ExternalPointerHandle old_handle = *handle_location;
       ExternalPointerHandle new_handle = index_to_handle(i);
+
+      // The following DCHECKs assert that the compaction algorithm works
+      // correctly: it always moves an entry from the evacuation area to the
+      // front of the table. One reason this invariant can be broken is if an
+      // external pointer slot is re-initialized, in which case the old_handle
+      // may now also point before the evacuation area. For that reason,
+      // re-initialization of external pointer slots is forbidden.
       DCHECK_GE(handle_to_index(old_handle), first_block_of_evacuation_area);
       DCHECK_LT(handle_to_index(new_handle), first_block_of_evacuation_area);
 

src/sandbox/external-pointer-table.h

@@ -397,7 +397,7 @@ class V8_EXPORT_PRIVATE ExternalPointerTable {
   }
 
   static bool is_evacuation_entry(Address entry) {
-    return (entry & kEvacuationEntryTag) == kEvacuationEntryTag;
+    return (entry & kExternalPointerTagMask) == kEvacuationEntryTag;
   }
 
   static Address extract_handle_location_from_evacuation_entry(Address entry) {

src/sandbox/external-pointer.h

@@ -34,6 +34,17 @@ template <ExternalPointerTag tag>
 V8_INLINE void WriteExternalPointerField(Address field_address,
                                          Isolate* isolate, Address value);
 
+// Writes and possibly initialized a lazily initialized external pointer field.
+// When the sandbox is enabled, a lazily initialized external pointer field
+// initially contains the kNullExternalPointerHandle and will only be properly
+// initialized (i.e. allocate an entry in the external pointer table) once a
+// value is written into it for the first time.
+// If the sandbox is disabled, this is equivalent to WriteExternalPointerField.
+template <ExternalPointerTag tag>
+V8_INLINE void WriteLazilyInitializedExternalPointerField(Address field_address,
+                                                          Isolate* isolate,
+                                                          Address value);
+
 }  // namespace internal
 }  // namespace v8