[BackupRefPtr] Support pointers past the end of allocation

It's common for C++ code to have pointers right past the end of an
allocation, i.e. of the form |allocation_start + allocation_size|. When
|PartitionAllocGetSlotStart| receives such a pointer and the size of the
allocation equals the size of its bucket, the function returns the
address of the next slot (or even the area outside the slot span).
Subsequent |PartitionRefCount| operations may then modify the reference
count of an unrelated object or corrupt the slot's free list pointer (if
it is in the "freed" state).

Instead of adding more fields to the CheckedPtr ignore list, we modify
|PartitionAllocGetSlotStart| to support past-the-end pointers.

In addition, remove the workaround for a bug that has been caused by the
past-the-end pointer in |base::BigEndianWriter::ptr_|.

Bug: 1073933, 1164636
Change-Id: Ia582680d9d6c83357f45123416a86a52661fa71c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2636354
Commit-Queue: Sergei Glazunov <glazunov@google.com>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Bartek Nowierski <bartekn@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#846076}

base/allocator/partition_allocator/partition_ref_count.h

@@ -85,16 +85,6 @@ class BASE_EXPORT PartitionRefCount {
   }
 
  private:
-#if defined(__clang__)
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wunused-private-field"
-#endif
-  void* padding_;  // TODO(crbug.com/1164636): This "workaround" is meant to
-                   // reduce the number of freelist corruption crashes we see in
-                   // experiments. Remove once root cause has been found.
-#if defined(__clang__)
-#pragma clang diagnostic pop
-#endif
   std::atomic<int32_t> count_{1};
 };
 

base/allocator/partition_allocator/partition_root.h

@@ -686,6 +686,16 @@ ALWAYS_INLINE size_t PartitionAllocGetSlotOffset(void* ptr) {
 // (thread-safe or not) of the allocator. This relies on the two PartitionRoot<>
 // having the same layout, which is enforced by static_assert().
 ALWAYS_INLINE void* PartitionAllocGetSlotStart(void* ptr) {
+#if ENABLE_REF_COUNT_FOR_BACKUP_REF_PTR
+  // Adjust to support pointers right past the end of an allocation, which in
+  // some cases appear to point outside the designated allocation slot.
+  // There is no risk of going too far i.e. confusing |ptr -
+  // kPartitionRefCountOffset| with the previous allocation because |ptr|, which
+  // is a pointer within the user-accessible area, is at least
+  // |kPartitionRefCountOffset| bytes away from the beginning of its slot.
+  ptr = reinterpret_cast<char*>(ptr) - kPartitionRefCountOffset;
+#endif
+
   internal::DCheckIfManagedByPartitionAllocNormalBuckets(ptr);
   auto* slot_span =
       internal::PartitionAllocGetSlotSpanForSizeQuery<internal::ThreadSafe>(

base/memory/checked_ptr_unittest.cc

@@ -774,6 +774,45 @@ TEST(BackupRefPtrImpl, ZeroSized) {
   }
 }
 
+TEST(BackupRefPtrImpl, EndPointer) {
+  // This test works only if GigaCage is enabled. Bail out otherwise.
+  if (!features::IsPartitionAllocGigaCageEnabled())
+    return;
+
+  // This test requires a fresh partition with an empty free list.
+  PartitionAllocGlobalInit(HandleOOM);
+  PartitionAllocator<ThreadSafe> allocator;
+  allocator.init({});
+
+  // Check multiple size buckets and levels of slot filling.
+  for (int size = 0; size < 1024; size += sizeof(void*)) {
+    // Creating a CheckedPtr from an address right past the end of an allocation
+    // should not result in a crash or corrupt the free list.
+    char* raw_ptr1 = reinterpret_cast<char*>(allocator.root()->Alloc(size, ""));
+    CheckedPtr<char> checked_ptr = raw_ptr1 + size;
+    checked_ptr = nullptr;
+    // We need to make two more allocations to turn the possible free list
+    // corruption into an observable crash.
+    char* raw_ptr2 = reinterpret_cast<char*>(allocator.root()->Alloc(size, ""));
+    char* raw_ptr3 = reinterpret_cast<char*>(allocator.root()->Alloc(size, ""));
+
+    // Similarly for operator+=.
+    char* raw_ptr4 = reinterpret_cast<char*>(allocator.root()->Alloc(size, ""));
+    checked_ptr = raw_ptr4;
+    checked_ptr += size;
+    checked_ptr = nullptr;
+    char* raw_ptr5 = reinterpret_cast<char*>(allocator.root()->Alloc(size, ""));
+    char* raw_ptr6 = reinterpret_cast<char*>(allocator.root()->Alloc(size, ""));
+
+    allocator.root()->Free(raw_ptr1);
+    allocator.root()->Free(raw_ptr2);
+    allocator.root()->Free(raw_ptr3);
+    allocator.root()->Free(raw_ptr4);
+    allocator.root()->Free(raw_ptr5);
+    allocator.root()->Free(raw_ptr6);
+  }
+}
+
 #endif  // BUILDFLAG(USE_PARTITION_ALLOC) && ENABLE_BACKUP_REF_PTR_IMPL &&
         // !defined(MEMORY_TOOL_REPLACES_ALLOCATOR)
 }  // namespace internal

tools/clang/rewrite_raw_ptr_fields/manual-fields-to-ignore.txt

@@ -128,11 +128,6 @@ views::internal::ClassPropertyValueSetter::property_  # passed to templated para
 (anonymous namespace)::ScopedFunctionHelper::function_  # function pointer template
 KeyedServiceBaseFactory::service_name_ # used in decltype
 
-# Populated manually - this field points past the end of an allocation;
-# therefore, it can refer to an invalid allocation slot
-base::BigEndianWriter::end_
-base::BigEndianWriter::ptr_
-
 # ELEMENT() treats the CheckedPtr as a void*, and so when a pointer is written
 # AddRef() won't be called, causing AddRef/Deref mismatch.
 device::AttestedCredentialData::ConsumeFromCtapResponse(base::span<const uint8_t>)::COSEKey::alg