[sandbox] Make ExternalPointerTable::capacity_ atomic

The capacity may be modified on one thread when growing the table while
being used in a DCHECK (to sanity-check a provided
ExternalPointerHandle) on another thread, resulting in TSan failures.
This CL turns these accesses into atomic accesses and adds a comment
explaining when the capacity value can be used reliably.

Bug: chromium:1352148
Change-Id: I0b86a47e16cfa14ff2d296e7e507e38a3fb5893c
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/+/3826244
Commit-Queue: Samuel Groß <saelo@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82444}

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

@@ -87,7 +87,7 @@ void ExternalPointerTable::TearDown() {
 Address ExternalPointerTable::Get(ExternalPointerHandle handle,
                                   ExternalPointerTag tag) const {
   uint32_t index = handle >> kExternalPointerIndexShift;
-  DCHECK_LT(index, capacity_);
+  DCHECK_LT(index, capacity());
 
   Address entry = load_atomic(index);
   DCHECK(!is_free(entry));
@@ -102,7 +102,7 @@ void ExternalPointerTable::Set(ExternalPointerHandle handle, Address value,
   DCHECK(is_marked(tag));
 
   uint32_t index = handle >> kExternalPointerIndexShift;
-  DCHECK_LT(index, capacity_);
+  DCHECK_LT(index, capacity());
 
   store_atomic(index, value | tag);
 }
@@ -114,7 +114,7 @@ Address ExternalPointerTable::Exchange(ExternalPointerHandle handle,
   DCHECK(is_marked(tag));
 
   uint32_t index = handle >> kExternalPointerIndexShift;
-  DCHECK_LT(index, capacity_);
+  DCHECK_LT(index, capacity());
 
   Address entry = exchange_atomic(index, value | tag);
   DCHECK(!is_free(entry));
@@ -149,7 +149,7 @@ ExternalPointerHandle ExternalPointerTable::Allocate() {
 
     DCHECK(freelist_head);
     DCHECK_NE(freelist_head, kTableIsCurrentlySweepingMarker);
-    DCHECK_LT(freelist_head, capacity_);
+    DCHECK_LT(freelist_head, capacity());
     index = freelist_head;
 
     // The next free element is stored in the lower 32 bits of the entry.
@@ -167,7 +167,7 @@ void ExternalPointerTable::Mark(ExternalPointerHandle handle) {
   static_assert(sizeof(base::Atomic64) == sizeof(Address));
 
   uint32_t index = handle >> kExternalPointerIndexShift;
-  DCHECK_LT(index, capacity_);
+  DCHECK_LT(index, capacity());
 
   base::Atomic64 old_val = load_atomic(index);
   DCHECK(!is_free(old_val));

src/sandbox/external-pointer-table.cc

@@ -25,8 +25,8 @@ uint32_t ExternalPointerTable::Sweep(Isolate* isolate) {
 
   // Keep track of the last block (identified by the index of its first entry)
   // that has live entries. Used to decommit empty blocks at the end.
-  DCHECK_GE(capacity_, kEntriesPerBlock);
-  const uint32_t last_block = capacity_ - kEntriesPerBlock;
+  DCHECK_GE(capacity(), kEntriesPerBlock);
+  const uint32_t last_block = capacity() - kEntriesPerBlock;
   uint32_t last_in_use_block = last_block;
 
   // Sweep top to bottom and rebuild the freelist from newly dead and
@@ -39,8 +39,8 @@ uint32_t ExternalPointerTable::Sweep(Isolate* isolate) {
 
   // Skip the special null entry. This also guarantees that the first block
   // will never be decommitted.
-  DCHECK_GE(capacity_, 1);
-  for (uint32_t i = capacity_ - 1; i > 0; i--) {
+  DCHECK_GE(capacity(), 1);
+  for (uint32_t i = capacity() - 1; i > 0; i--) {
     // No other threads are active during sweep, so there is no need to use
     // atomic operations here.
     Address entry = load(i);
@@ -67,10 +67,10 @@ uint32_t ExternalPointerTable::Sweep(Isolate* isolate) {
   // Decommit all blocks at the end of the table that are not used anymore.
   if (last_in_use_block != last_block) {
     uint32_t new_capacity = last_in_use_block + kEntriesPerBlock;
-    DCHECK_LT(new_capacity, capacity_);
+    DCHECK_LT(new_capacity, capacity());
     Address new_table_end = buffer_ + new_capacity * sizeof(Address);
-    uint32_t bytes_to_decommit = (capacity_ - new_capacity) * sizeof(Address);
-    capacity_ = new_capacity;
+    uint32_t bytes_to_decommit = (capacity() - new_capacity) * sizeof(Address);
+    set_capacity(new_capacity);
 
     VirtualAddressSpace* root_space = GetPlatformVirtualAddressSpace();
     CHECK(root_space->DecommitPages(new_table_end, bytes_to_decommit));
@@ -78,7 +78,7 @@ uint32_t ExternalPointerTable::Sweep(Isolate* isolate) {
 
   base::Release_Store(&freelist_head_, current_freelist_head);
 
-  uint32_t num_active_entries = capacity_ - freelist_size;
+  uint32_t num_active_entries = capacity() - freelist_size;
   isolate->counters()->external_pointers_count()->AddSample(num_active_entries);
   return num_active_entries;
 }
@@ -90,7 +90,7 @@ uint32_t ExternalPointerTable::Grow() {
   mutex_->AssertHeld();
 
   // Grow the table by one block.
-  uint32_t old_capacity = capacity_;
+  uint32_t old_capacity = capacity();
   uint32_t new_capacity = old_capacity + kEntriesPerBlock;
   CHECK_LE(new_capacity, kMaxExternalPointers);
 
@@ -100,7 +100,7 @@ uint32_t ExternalPointerTable::Grow() {
   CHECK(root_space->SetPagePermissions(buffer_ + old_capacity * sizeof(Address),
                                        kBlockSize,
                                        PagePermissions::kReadWrite));
-  capacity_ = new_capacity;
+  set_capacity(new_capacity);
 
   // Build freelist bottom to top, which might be more cache friendly.
   uint32_t start = std::max<uint32_t>(old_capacity, 1);  // Skip entry zero

src/sandbox/external-pointer-table.h

@@ -144,6 +144,20 @@ class V8_EXPORT_PRIVATE ExternalPointerTable {
   // Returns true if this external pointer table has been initialized.
   bool is_initialized() { return buffer_ != kNullAddress; }
 
+  // Table capacity accesors.
+  // The capacity of the table may increase during entry allocation (if the
+  // table is grown) and may decrease during sweeping (if blocks at the end are
+  // free). As the former may happen concurrently, the capacity can only be
+  // used reliably if either the table mutex is held or if all mutator threads
+  // are currently stopped. However, it is fine to use this value to
+  // sanity-check incoming ExternalPointerHandles in debug builds (there's no
+  // need for actual bounds-checks because out-of-bounds accesses are guaranteed
+  // to result in a harmless crash).
+  uint32_t capacity() const { return base::Relaxed_Load(&capacity_); }
+  void set_capacity(uint32_t new_capacity) {
+    base::Relaxed_Store(&capacity_, new_capacity);
+  }
+
   // Extends the table and adds newly created entries to the freelist. Returns
   // the new freelist head. When calling this method, mutex_ must be locked.
   //
@@ -230,7 +244,7 @@ class V8_EXPORT_PRIVATE ExternalPointerTable {
   Address buffer_ = kNullAddress;
 
   // The current capacity of this table, which is the number of usable entries.
-  uint32_t capacity_ = 0;
+  base::Atomic32 capacity_ = 0;
 
   // The index of the first entry on the freelist or zero if the list is empty.
   base::Atomic32 freelist_head_ = 0;