Avoid UB in raw_ptr::operator-=()

When making the method constexpr and calling it, the compiler generates
the following error if you call

raw_ptr -= 1u;

"Cannot refer to element 4294967296 of array of 3 elements in a
constant expression"

Negating a size_t variable is not what we want to do here, as that
produces an index out of bounds. Add a Retreat() method to raw_ptr
implementations to avoid trying to negate, as that can produce
overflow with both a signed or unsigned input.

This also requires a parameter to PartitionAlloc's IsValidDelta
paths, to say whether the delta should be added or subtracted from
the original pointer. This does avoid any CHECKs for invalid
negations.

Change-Id: I9ee8650e9658fae113d0d42b8cc8f2af7e077e48
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4284957
Reviewed-by: Bartek Nowierski <bartekn@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1112433}

base/allocator/partition_allocator/partition_alloc_constants.h

@@ -503,18 +503,6 @@ using ::partition_alloc::internal::kSuperPageSize;
 using ::partition_alloc::internal::MaxDirectMapped;
 using ::partition_alloc::internal::PartitionPageSize;
 
-// Return values to indicate where a pointer is pointing relative to the bounds
-// of an allocation.
-enum class PtrPosWithinAlloc {
-  // When PA_USE_OOB_POISON is disabled, end-of-allocation pointers are also
-  // considered in-bounds.
-  kInBounds,
-#if PA_CONFIG(USE_OOB_POISON)
-  kAllocEnd,
-#endif
-  kFarOOB
-};
-
 }  // namespace partition_alloc
 
 #endif  // BASE_ALLOCATOR_PARTITION_ALLOCATOR_PARTITION_ALLOC_CONSTANTS_H_

base/allocator/partition_allocator/partition_alloc_forward.h

@@ -51,27 +51,9 @@ void CheckThatSlotOffsetIsZero(uintptr_t address);
 // We support pointer offsets in signed (ptrdiff_t) or unsigned (size_t) values.
 // Smaller types are also allowed.
 template <typename Z>
-static constexpr bool offset_type =
+static constexpr bool is_offset_type =
     std::is_integral_v<Z> && sizeof(Z) <= sizeof(ptrdiff_t);
 
-template <typename Z, typename = std::enable_if_t<offset_type<Z>, void>>
-struct PtrDelta {
-  Z delta_in_bytes;
-#if PA_CONFIG(USE_OOB_POISON)
-  // Size of the element type referenced by the pointer
-  size_t type_size;
-#endif
-
-  constexpr PtrDelta(Z delta_in_bytes, size_t type_size)
-      : delta_in_bytes(delta_in_bytes)
-#if PA_CONFIG(USE_OOB_POISON)
-        ,
-        type_size(type_size)
-#endif
-  {
-  }
-};
-
 }  // namespace internal
 
 class PartitionStatsDumper;

base/allocator/partition_allocator/partition_alloc_unittest.cc

@@ -1409,7 +1409,7 @@ TEST_P(PartitionAllocTest, MTEProtectsFreedPtr) {
 #endif  // PA_CONFIG(HAS_MEMORY_TAGGING)
 
 #if BUILDFLAG(ENABLE_BACKUP_REF_PTR_SUPPORT)
-TEST_P(PartitionAllocTest, IsValidPtrDelta) {
+TEST_P(PartitionAllocTest, IsPtrWithinSameAlloc) {
   if (!UseBRPPool()) {
     return;
   }
@@ -1461,100 +1461,88 @@ TEST_P(PartitionAllocTest, IsValidPtrDelta) {
       }
 
       uintptr_t address = UntagPtr(ptr);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address,
-                                              PtrDelta(-kFarFarAwayDelta, 0)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(address, address - kFarFarAwayDelta, 0u),
                 PtrPosWithinAlloc::kFarOOB);
-      EXPECT_EQ(
-          PartitionAllocIsValidPtrDelta(address, PtrDelta(-kSuperPageSize, 0)),
-          PtrPosWithinAlloc::kFarOOB);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address, PtrDelta(-1, 0)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(address, address - kSuperPageSize, 0u),
+                PtrPosWithinAlloc::kFarOOB);
+      EXPECT_EQ(IsPtrWithinSameAlloc(address, address - 1, 0u),
                 PtrPosWithinAlloc::kFarOOB);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address, PtrDelta(0, 0)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(address, address, 0u),
                 PtrPosWithinAlloc::kInBounds);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address,
-                                              PtrDelta(requested_size / 2, 0)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(address, address + requested_size / 2, 0u),
                 PtrPosWithinAlloc::kInBounds);
 #if PA_CONFIG(USE_OOB_POISON)
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address,
-                                              PtrDelta(requested_size - 1, 1)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(address, address + requested_size - 1, 1u),
                 PtrPosWithinAlloc::kInBounds);
-      EXPECT_EQ(
-          PartitionAllocIsValidPtrDelta(address, PtrDelta(requested_size, 1)),
-          PtrPosWithinAlloc::kAllocEnd);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address,
-                                              PtrDelta(requested_size - 4, 4)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(address, address + requested_size, 1u),
+                PtrPosWithinAlloc::kAllocEnd);
+      EXPECT_EQ(IsPtrWithinSameAlloc(address, address + requested_size - 4, 4u),
                 PtrPosWithinAlloc::kInBounds);
       for (size_t subtrahend = 0; subtrahend < 4; subtrahend++) {
-        EXPECT_EQ(PartitionAllocIsValidPtrDelta(
-                      address, PtrDelta(requested_size - subtrahend, 4)),
+        EXPECT_EQ(IsPtrWithinSameAlloc(
+                      address, address + requested_size - subtrahend, 4u),
                   PtrPosWithinAlloc::kAllocEnd);
       }
 #else
-      EXPECT_EQ(
-          PartitionAllocIsValidPtrDelta(address, PtrDelta(requested_size, 0)),
-          PtrPosWithinAlloc::kInBounds);
+      EXPECT_EQ(IsPtrWithinSameAlloc(address, address + requested_size, 0u),
+                PtrPosWithinAlloc::kInBounds);
 #endif
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address,
-                                              PtrDelta(requested_size + 1, 0)),
-                PtrPosWithinAlloc::kFarOOB);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(
-                    address, PtrDelta(requested_size + kSuperPageSize, 0)),
-                PtrPosWithinAlloc::kFarOOB);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(
-                    address, PtrDelta(requested_size + kFarFarAwayDelta, 0)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(address, address + requested_size + 1, 0u),
                 PtrPosWithinAlloc::kFarOOB);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address + requested_size,
-                                              PtrDelta(kFarFarAwayDelta, 0)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(
+                    address, address + requested_size + kSuperPageSize, 0u),
                 PtrPosWithinAlloc::kFarOOB);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address + requested_size,
-                                              PtrDelta(kSuperPageSize, 0)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(
+                    address, address + requested_size + kFarFarAwayDelta, 0u),
                 PtrPosWithinAlloc::kFarOOB);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address + requested_size,
-                                              PtrDelta(1, 0)),
+      EXPECT_EQ(
+          IsPtrWithinSameAlloc(address + requested_size,
+                               address + requested_size + kFarFarAwayDelta, 0u),
+          PtrPosWithinAlloc::kFarOOB);
+      EXPECT_EQ(
+          IsPtrWithinSameAlloc(address + requested_size,
+                               address + requested_size + kSuperPageSize, 0u),
+          PtrPosWithinAlloc::kFarOOB);
+      EXPECT_EQ(IsPtrWithinSameAlloc(address + requested_size,
+                                     address + requested_size + 1, 0u),
                 PtrPosWithinAlloc::kFarOOB);
 #if PA_CONFIG(USE_OOB_POISON)
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address + requested_size - 1,
-                                              PtrDelta(0, 1)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(address + requested_size - 1,
+                                     address + requested_size - 1, 1u),
                 PtrPosWithinAlloc::kInBounds);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address + requested_size - 1,
-                                              PtrDelta(1, 1)),
-                PtrPosWithinAlloc::kAllocEnd);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address + requested_size,
-                                              PtrDelta(0, 1)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(address + requested_size - 1,
+                                     address + requested_size, 1u),
                 PtrPosWithinAlloc::kAllocEnd);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address + requested_size - 1,
-                                              PtrDelta(1, 1)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(address + requested_size,
+                                     address + requested_size, 1u),
                 PtrPosWithinAlloc::kAllocEnd);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address + requested_size - 4,
-                                              PtrDelta(0, 4)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(address + requested_size - 4,
+                                     address + requested_size - 4, 4u),
                 PtrPosWithinAlloc::kInBounds);
       for (size_t addend = 1; addend < 4; addend++) {
-        EXPECT_EQ(PartitionAllocIsValidPtrDelta(address + requested_size - 4,
-                                                PtrDelta(addend, 4)),
-                  PtrPosWithinAlloc::kAllocEnd);
+        EXPECT_EQ(
+            IsPtrWithinSameAlloc(address + requested_size - 4,
+                                 address + requested_size - 4 + addend, 4u),
+            PtrPosWithinAlloc::kAllocEnd);
       }
 #else
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address + requested_size,
-                                              PtrDelta(0, 0)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(address + requested_size,
+                                     address + requested_size, 0u),
                 PtrPosWithinAlloc::kInBounds);
 #endif
-      EXPECT_EQ(
-          PartitionAllocIsValidPtrDelta(address + requested_size,
-                                        PtrDelta(-(requested_size / 2), 0)),
-          PtrPosWithinAlloc::kInBounds);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address + requested_size,
-                                              PtrDelta(-requested_size, 0)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(
+                    address + requested_size,
+                    address + requested_size - (requested_size / 2), 0u),
+                PtrPosWithinAlloc::kInBounds);
+      EXPECT_EQ(IsPtrWithinSameAlloc(address + requested_size, address, 0u),
                 PtrPosWithinAlloc::kInBounds);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(address + requested_size,
-                                              PtrDelta(-requested_size - 1, 0)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(address + requested_size, address - 1, 0u),
                 PtrPosWithinAlloc::kFarOOB);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(
-                    address + requested_size,
-                    PtrDelta(-requested_size - kSuperPageSize, 0)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(address + requested_size,
+                                     address - kSuperPageSize, 0u),
                 PtrPosWithinAlloc::kFarOOB);
-      EXPECT_EQ(PartitionAllocIsValidPtrDelta(
-                    address + requested_size,
-                    PtrDelta(-requested_size - kFarFarAwayDelta, 0)),
+      EXPECT_EQ(IsPtrWithinSameAlloc(address + requested_size,
+                                     address - kFarFarAwayDelta, 0u),
                 PtrPosWithinAlloc::kFarOOB);
     }
 

base/allocator/partition_allocator/partition_root.cc

@@ -24,6 +24,7 @@
 #include "base/allocator/partition_allocator/partition_cookie.h"
 #include "base/allocator/partition_allocator/partition_oom.h"
 #include "base/allocator/partition_allocator/partition_page.h"
+#include "base/allocator/partition_allocator/partition_ref_count.h"
 #include "base/allocator/partition_allocator/pkey.h"
 #include "base/allocator/partition_allocator/reservation_offset_table.h"
 #include "base/allocator/partition_allocator/tagging.h"
@@ -46,9 +47,9 @@
 #include <pthread.h>
 #endif
 
-#if BUILDFLAG(RECORD_ALLOC_INFO)
 namespace partition_alloc::internal {
 
+#if BUILDFLAG(RECORD_ALLOC_INFO)
 // Even if this is not hidden behind a BUILDFLAG, it should not use any memory
 // when recording is disabled, since it ends up in the .bss section.
 AllocInfo g_allocs = {};
@@ -57,9 +58,47 @@ void RecordAllocOrFree(uintptr_t addr, size_t size) {
   g_allocs.allocs[g_allocs.index.fetch_add(1, std::memory_order_relaxed) %
                   kAllocInfoSize] = {addr, size};
 }
+#endif  // BUILDFLAG(RECORD_ALLOC_INFO)
+
+#if BUILDFLAG(ENABLE_BACKUP_REF_PTR_SUPPORT)
+PtrPosWithinAlloc IsPtrWithinSameAlloc(uintptr_t orig_address,
+                                       uintptr_t test_address,
+                                       size_t type_size) {
+  // Required for pointers right past an allocation. See
+  // |PartitionAllocGetSlotStartInBRPPool()|.
+  uintptr_t adjusted_address =
+      orig_address - kPartitionPastAllocationAdjustment;
+  PA_DCHECK(IsManagedByNormalBucketsOrDirectMap(adjusted_address));
+  DCheckIfManagedByPartitionAllocBRPPool(adjusted_address);
+
+  uintptr_t slot_start = PartitionAllocGetSlotStartInBRPPool(adjusted_address);
+  // Don't use |adjusted_address| beyond this point at all. It was needed to
+  // pick the right slot, but now we're dealing with very concrete addresses.
+  // Zero it just in case, to catch errors.
+  adjusted_address = 0;
+
+  auto* slot_span = SlotSpanMetadata<ThreadSafe>::FromSlotStart(slot_start);
+  auto* root = PartitionRoot<ThreadSafe>::FromSlotSpan(slot_span);
+  // Double check that ref-count is indeed present.
+  PA_DCHECK(root->brp_enabled());
+
+  uintptr_t object_addr = root->SlotStartToObjectAddr(slot_start);
+  uintptr_t object_end = object_addr + slot_span->GetUsableSize(root);
+  if (test_address < object_addr || object_end < test_address) {
+    return PtrPosWithinAlloc::kFarOOB;
+#if PA_CONFIG(USE_OOB_POISON)
+  } else if (object_end - type_size < test_address) {
+    // Not even a single element of the type referenced by the pointer can fit
+    // between the pointer and the end of the object.
+    return PtrPosWithinAlloc::kAllocEnd;
+#endif
+  } else {
+    return PtrPosWithinAlloc::kInBounds;
+  }
+}
+#endif  // BUILDFLAG(ENABLE_BACKUP_REF_PTR_SUPPORT)
 
 }  // namespace partition_alloc::internal
-#endif  // BUILDFLAG(RECORD_ALLOC_INFO)
 
 namespace partition_alloc {
 
@@ -1636,4 +1675,5 @@ static_assert(offsetof(PartitionRoot<internal::ThreadSafe>, sentinel_bucket) ==
 static_assert(
     offsetof(PartitionRoot<internal::ThreadSafe>, lock_) >= 64,
     "The lock should not be on the same cacheline as the read-mostly flags");
+
 }  // namespace partition_alloc

base/allocator/partition_allocator/partition_root.h

@@ -40,7 +40,6 @@
 #include "base/allocator/partition_allocator/chromecast_buildflags.h"
 #include "base/allocator/partition_allocator/freeslot_bitmap.h"
 #include "base/allocator/partition_allocator/page_allocator.h"
-#include "base/allocator/partition_allocator/page_allocator_constants.h"
 #include "base/allocator/partition_allocator/partition_address_space.h"
 #include "base/allocator/partition_allocator/partition_alloc-inl.h"
 #include "base/allocator/partition_allocator/partition_alloc_base/bits.h"
@@ -1045,47 +1044,34 @@ PartitionAllocGetSlotStartInBRPPool(uintptr_t address) {
          bucket->slot_size * bucket->GetSlotNumber(offset_in_slot_span);
 }
 
-// Checks whether a given address stays within the same allocation slot after
-// modification.
+// Return values to indicate where a pointer is pointing relative to the bounds
+// of an allocation.
+enum class PtrPosWithinAlloc {
+  // When PA_USE_OOB_POISON is disabled, end-of-allocation pointers are also
+  // considered in-bounds.
+  kInBounds,
+#if PA_CONFIG(USE_OOB_POISON)
+  kAllocEnd,
+#endif
+  kFarOOB
+};
+
+// Checks whether `test_address` is in the same allocation slot as
+// `orig_address`.
+//
+// This can be called after adding or subtracting from the `orig_address`
+// to produce a different pointer which must still stay in the same allocation.
+//
+// The `type_size` is the size of the type that the raw_ptr is pointing to,
+// which may be the type the allocation is holding or a compatible pointer type
+// such as a base class or char*. It is used to detect pointers near the end of
+// the allocation but not strictly beyond it.
 //
 // This isn't a general purpose function. The caller is responsible for ensuring
 // that the ref-count is in place for this allocation.
-template <typename Z>
-PA_ALWAYS_INLINE PtrPosWithinAlloc
-PartitionAllocIsValidPtrDelta(uintptr_t address, PtrDelta<Z> delta) {
-  // Required for pointers right past an allocation. See
-  // |PartitionAllocGetSlotStartInBRPPool()|.
-  uintptr_t adjusted_address = address - kPartitionPastAllocationAdjustment;
-  PA_DCHECK(IsManagedByNormalBucketsOrDirectMap(adjusted_address));
-  DCheckIfManagedByPartitionAllocBRPPool(adjusted_address);
-
-  uintptr_t slot_start = PartitionAllocGetSlotStartInBRPPool(adjusted_address);
-  // Don't use |adjusted_address| beyond this point at all. It was needed to
-  // pick the right slot, but now we're dealing with very concrete addresses.
-  // Zero it just in case, to catch errors.
-  adjusted_address = 0;
-
-  auto* slot_span = SlotSpanMetadata<ThreadSafe>::FromSlotStart(slot_start);
-  auto* root = PartitionRoot<ThreadSafe>::FromSlotSpan(slot_span);
-  // Double check that ref-count is indeed present.
-  PA_DCHECK(root->brp_enabled());
-
-  uintptr_t object_addr = root->SlotStartToObjectAddr(slot_start);
-  uintptr_t new_address =
-      address + static_cast<uintptr_t>(delta.delta_in_bytes);
-  uintptr_t object_end = object_addr + slot_span->GetUsableSize(root);
-  if (new_address < object_addr || object_end < new_address) {
-    return PtrPosWithinAlloc::kFarOOB;
-#if PA_CONFIG(USE_OOB_POISON)
-  } else if (object_end - delta.type_size < new_address) {
-    // Not even a single element of the type referenced by the pointer can fit
-    // between the pointer and the end of the object.
-    return PtrPosWithinAlloc::kAllocEnd;
-#endif
-  } else {
-    return PtrPosWithinAlloc::kInBounds;
-  }
-}
+PtrPosWithinAlloc IsPtrWithinSameAlloc(uintptr_t orig_address,
+                                       uintptr_t test_address,
+                                       size_t type_size);
 
 PA_ALWAYS_INLINE void PartitionAllocFreeForRefCounting(uintptr_t slot_start) {
   PA_DCHECK(!PartitionRefCountPointer(slot_start)->IsAlive());