Revert "[sandbox] Forbid double-initialization of ExternalPointerSlots"

This reverts commit a31e8f24.

Reason for revert: Causes DCHECK failures with --stress-snapshot

Original change's description:
> [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/+/3856563
> Reviewed-by: Igor Sheludko <ishell@chromium.org>
> Commit-Queue: Samuel Groß <saelo@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#82729}

Bug: v8:10391
Change-Id: I37e6728cc16fe79fa7d743417dc9938d58fb0474
Cq-Include-Trybots: luci.v8.try:v8_linux64_heap_sandbox_dbg_ng,v8_linux_arm64_sim_heap_sandbox_dbg_ng
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3857422
Commit-Queue: Samuel Groß <saelo@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#82732}

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

@@ -115,11 +115,8 @@ 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);
-  // 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>(
+  // This also mark the entry as alive until the next GC.
+  InitExternalPointerField<kEmbedderDataSlotPayloadTag>(
       address() + kExternalPointerOffset, isolate, value);
   ObjectSlot(address() + kTaggedPayloadOffset).Relaxed_Store(Smi::zero());
   return true;

src/objects/slots-inl.h

@@ -158,11 +158,6 @@ 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,19 +1100,6 @@ 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,9 +309,6 @@ 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);
@@ -396,9 +393,6 @@ 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,12 +910,6 @@ 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,46 +30,11 @@ 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
@@ -115,30 +80,6 @@ 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,13 +163,6 @@ 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 & kExternalPointerTagMask) == kEvacuationEntryTag;
+    return (entry & kEvacuationEntryTag) == kEvacuationEntryTag;
   }
 
   static Address extract_handle_location_from_evacuation_entry(Address entry) {

src/sandbox/external-pointer.h

@@ -34,17 +34,6 @@ 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