[sandbox] Fix DCHECK failure in ExternalPointerTable

When compaction is aborted during marking, the
start_of_evacuation_area_ value would previously be set to -1. This
would, however, cause some DCHECK failures during sweeping, which
expect this value to contain the (previous) start value. This is now
fixed by just setting the top bits of the start_of_evacuation_area_
value when aborting compaction. During sweeping, these bits are
cleared again and the DCHECKs work as expected.

Bug: v8:10391, chromium:1355640
Change-Id: Id48ee71a3942b3e0b88e8e1667a8f9e109a68bb3
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/+/3849650
Commit-Queue: Samuel Groß <saelo@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82661}

src/sandbox/external-pointer-table-inl.h

@@ -217,7 +217,9 @@ void ExternalPointerTable::Mark(ExternalPointerHandle handle,
       // abort compaction here. Entries that have already been visited will
       // still be compacted during Sweep, but there is no guarantee that any
       // blocks at the end of the table will now be completely free.
-      start_of_evacuation_area_ = kTableCompactionAbortedMarker;
+      uint32_t compaction_aborted_marker =
+          start_of_evacuation_area_ | kCompactionAbortedMarker;
+      start_of_evacuation_area_ = compaction_aborted_marker;
     }
   }
   // Even if the entry is marked for evacuation, it still needs to be marked as
@@ -237,6 +239,15 @@ void ExternalPointerTable::Mark(ExternalPointerHandle handle,
   USE(val);
 }
 
+bool ExternalPointerTable::IsCompacting() {
+  return start_of_evacuation_area_ != kNotCompactingMarker;
+}
+
+bool ExternalPointerTable::CompactingWasAbortedDuringMarking() {
+  return (start_of_evacuation_area_ & kCompactionAbortedMarker) ==
+         kCompactionAbortedMarker;
+}
+
 }  // namespace internal
 }  // namespace v8
 

src/sandbox/external-pointer-table.cc

@@ -32,18 +32,19 @@ uint32_t ExternalPointerTable::SweepAndCompact(Isolate* isolate) {
 
   // When compacting, we can compute the number of unused blocks at the end of
   // the table and skip those during sweeping.
+  uint32_t first_block_of_evacuation_area = start_of_evacuation_area_;
   if (IsCompacting()) {
-    DCHECK(IsAligned(start_of_evacuation_area_, kEntriesPerBlock));
-
     TableCompactionOutcome outcome;
-    if (start_of_evacuation_area_ == kTableCompactionAbortedMarker) {
+    if (CompactingWasAbortedDuringMarking()) {
       // Compaction was aborted during marking because the freelist grew to
-      // short. This is not great because there is now no guarantee that any
-      // blocks will be completely emtpy and so the entire table needs to be
-      // swept.
+      // short. This is not great because now there is no guarantee that any
+      // blocks will be emtpy and so the entire table needs to be swept.
       outcome = TableCompactionOutcome::kAbortedDuringMarking;
+      // Extract the original start_of_evacuation_area value so that the
+      // DCHECKs below work correctly.
+      first_block_of_evacuation_area &= ~kCompactionAbortedMarker;
     } else if (!old_freelist_head ||
-               old_freelist_head > start_of_evacuation_area_) {
+               old_freelist_head > first_block_of_evacuation_area) {
       // In this case, marking finished successfully, but the application
       // afterwards allocated entries inside the area that is being compacted.
       // In this case, we can still compute how many blocks at the end of the
@@ -53,13 +54,13 @@ uint32_t ExternalPointerTable::SweepAndCompact(Isolate* isolate) {
       }
       outcome = TableCompactionOutcome::kPartialSuccess;
     } else {
-      // Marking was successful so the entire area that we are compacting is now
-      // free.
-      last_in_use_block = start_of_evacuation_area_ - kEntriesPerBlock;
+      // Marking was successful so the entire evacuation area is now free.
+      last_in_use_block = first_block_of_evacuation_area - kEntriesPerBlock;
       outcome = TableCompactionOutcome::kSuccess;
     }
     isolate->counters()->external_pointer_table_compaction_outcome()->AddSample(
         static_cast<int>(outcome));
+    DCHECK(IsAligned(first_block_of_evacuation_area, kEntriesPerBlock));
   }
 
   // Sweep top to bottom and rebuild the freelist from newly dead and
@@ -93,8 +94,8 @@ uint32_t ExternalPointerTable::SweepAndCompact(Isolate* isolate) {
 
       ExternalPointerHandle old_handle = *handle_location;
       ExternalPointerHandle new_handle = index_to_handle(i);
-      DCHECK_GE(handle_to_index(old_handle), start_of_evacuation_area_);
-      DCHECK_LT(handle_to_index(new_handle), start_of_evacuation_area_);
+      DCHECK_GE(handle_to_index(old_handle), first_block_of_evacuation_area);
+      DCHECK_LT(handle_to_index(new_handle), first_block_of_evacuation_area);
 
       Address entry_to_evacuate = load(handle_to_index(old_handle));
       store(i, clear_mark_bit(entry_to_evacuate));

src/sandbox/external-pointer-table.h

@@ -184,7 +184,10 @@ class V8_EXPORT_PRIVATE ExternalPointerTable {
   void StartCompactingIfNeeded();
 
   // Returns true if table compaction is currently running.
-  bool IsCompacting() { return start_of_evacuation_area_ != 0; }
+  inline bool IsCompacting();
+
+  // Returns true if table compaction was aborted during the GC marking phase.
+  inline bool CompactingWasAbortedDuringMarking();
 
  private:
   // Required for Isolate::CheckIsolateLayout().
@@ -202,12 +205,19 @@ class V8_EXPORT_PRIVATE ExternalPointerTable {
   static constexpr uint32_t kTableIsCurrentlySweepingMarker =
       (kExternalPointerTableReservationSize / kSystemPointerSize) - 1;
 
-  // During table compaction, this value is used to indicate that table
-  // compaction had to be aborted during the marking phase because the freelist
-  // grew to short. See Mark() for more details.
-  static constexpr uint32_t kTableCompactionAbortedMarker =
+  // This value is used for start_of_evacuation_area to indicate that the table
+  // is not currently being compacted. It is set to uint32_t max so that
+  // determining whether an entry should be evacuated becomes a single
+  // comparison: `bool should_be_evacuated = index >= start_of_evacuation_area`.
+  static constexpr uint32_t kNotCompactingMarker =
       std::numeric_limits<uint32_t>::max();
 
+  // This value may be ORed into the start_of_evacuation_area value during the
+  // GC marking phase to indicate that table compaction has been aborted
+  // because the freelist grew to short. This will prevent further evacuation
+  // attempts as `should_be_evacuated` (see above) will always be false.
+  static constexpr uint32_t kCompactionAbortedMarker = 0xf0000000;
+
   // Outcome of external pointer table compaction to use for the
   // ExternalPointerTableCompactionOutcome histogram.
   enum class TableCompactionOutcome {
@@ -410,7 +420,14 @@ class V8_EXPORT_PRIVATE ExternalPointerTable {
   // entry in the evacuation area. The evacuation area is the region at the end
   // of the table from which entries are moved out of so that the underyling
   // memory pages can be freed after sweeping.
-  uint32_t start_of_evacuation_area_ = 0;
+  // This field can have the following values:
+  // - kNotCompactingMarker: compaction is not currently running.
+  // - A kEntriesPerBlock aligned value within (0, capacity): table compaction
+  //   is running and all entries after this value should be evacuated.
+  // - A value that has kCompactionAbortedMarker in its top bits: table
+  //   compaction has been aborted during marking. The original start of the
+  //   evacuation area is still contained in the lower bits.
+  uint32_t start_of_evacuation_area_ = kNotCompactingMarker;
 
   // Lock protecting the slow path for entry allocation, in particular Grow().
   // As the size of this structure must be predictable (it's part of