[PA] Fix over-copying on realloc expansion

Currently, on realloc expansion, GetSize() bytes are copied. This is the
maximum size that an allocation can be stretched to within a slot, not
the actual size available to the application at the moment. These are
equivalent for small buckets, but not for single-slot slot spans where
extras can be glued immediately after the allocation.

The added test would fail without this fix.

Change-Id: Ic83e79c11e0a5b3b70f5c90a0bd1f157cbeae703
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2636101Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Bartek Nowierski <bartekn@chromium.org>
Auto-Submit: Bartek Nowierski <bartekn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845502}

base/allocator/partition_allocator/partition_alloc_unittest.cc

@@ -18,6 +18,7 @@
 #include "base/allocator/partition_allocator/page_allocator_constants.h"
 #include "base/allocator/partition_allocator/partition_alloc_constants.h"
 #include "base/allocator/partition_allocator/partition_alloc_features.h"
+#include "base/allocator/partition_allocator/partition_cookie.h"
 #include "base/allocator/partition_allocator/partition_page.h"
 #include "base/allocator/partition_allocator/partition_ref_count.h"
 #include "base/bits.h"
@@ -1023,7 +1024,8 @@ TEST_F(PartitionAllocTest, Realloc) {
   // Test that growing an allocation with realloc() copies everything from the
   // old allocation.
   size_t size = SystemPageSize() - kExtraAllocSize;
-  EXPECT_EQ(size, allocator.root()->ActualSize(size));
+  // Confirm size fills the entire slot.
+  ASSERT_EQ(size, allocator.root()->ActualSize(size));
   ptr = allocator.root()->Alloc(size, type_name);
   memset(ptr, 'A', size);
   ptr2 = allocator.root()->Realloc(ptr, size + 1, type_name);
@@ -1036,7 +1038,8 @@ TEST_F(PartitionAllocTest, Realloc) {
 #endif
 
   // Test that shrinking an allocation with realloc() also copies everything
-  // from the old allocation.
+  // from the old allocation. Use |size - 1| to test what happens to the extra
+  // space before the cookie.
   ptr = allocator.root()->Realloc(ptr2, size - 1, type_name);
   EXPECT_NE(ptr2, ptr);
   char* char_ptr = static_cast<char*>(ptr);
@@ -1048,6 +1051,38 @@ TEST_F(PartitionAllocTest, Realloc) {
 
   allocator.root()->Free(ptr);
 
+  // Single-slot slot spans...
+  // Test that growing an allocation with realloc() copies everything from the
+  // old allocation.
+  size = 200000;
+  // Confirm size doesn't fill the entire slot.
+  ASSERT_LT(size, allocator.root()->ActualSize(size));
+  ptr = allocator.root()->Alloc(size, type_name);
+  memset(ptr, 'A', size);
+  ptr2 = allocator.root()->Realloc(ptr, size * 2, type_name);
+  EXPECT_NE(ptr, ptr2);
+  char_ptr2 = static_cast<char*>(ptr2);
+  EXPECT_EQ('A', char_ptr2[0]);
+  EXPECT_EQ('A', char_ptr2[size - 1]);
+#if DCHECK_IS_ON()
+  EXPECT_EQ(kUninitializedByte, static_cast<unsigned char>(char_ptr2[size]));
+#endif
+
+  // Test that shrinking an allocation with realloc() also copies everything
+  // from the old allocation.
+  ptr = allocator.root()->Realloc(ptr2, size / 2, type_name);
+  EXPECT_NE(ptr2, ptr);
+  char_ptr = static_cast<char*>(ptr);
+  EXPECT_EQ('A', char_ptr[0]);
+  EXPECT_EQ('A', char_ptr[size / 2 - 1]);
+#if DCHECK_IS_ON()
+  // For single-slot slot spans, the cookie is always placed immediately after
+  // the allocation.
+  EXPECT_EQ(kCookieValue[0], static_cast<unsigned char>(char_ptr[size / 2]));
+#endif
+
+  allocator.root()->Free(ptr);
+
   // Test that shrinking a direct mapped allocation happens in-place.
   size = kMaxBucketed + 16 * SystemPageSize();
   ptr = allocator.root()->Alloc(size, type_name);

base/allocator/partition_allocator/partition_root.cc

@@ -571,10 +571,10 @@ void* PartitionRoot<thread_safe>::ReallocFlags(int flags,
 
   const bool hooks_enabled = PartitionAllocHooks::AreHooksEnabled();
   bool overridden = false;
-  size_t actual_old_size;
+  size_t old_usable_size;
   if (UNLIKELY(!no_hooks && hooks_enabled)) {
     overridden = PartitionAllocHooks::ReallocOverrideHookIfEnabled(
-        &actual_old_size, ptr);
+        &old_usable_size, ptr);
   }
   if (LIKELY(!overridden)) {
     auto* slot_span =
@@ -584,6 +584,7 @@ void* PartitionRoot<thread_safe>::ReallocFlags(int flags,
       internal::ScopedGuard<thread_safe> guard{lock_};
       // TODO(palmer): See if we can afford to make this a CHECK.
       PA_DCHECK(IsValidSlotSpan(slot_span));
+      old_usable_size = GetUsableSize(ptr);
 
       if (UNLIKELY(slot_span->bucket->is_direct_mapped())) {
         // We may be able to perform the realloc in place by changing the
@@ -601,7 +602,7 @@ void* PartitionRoot<thread_safe>::ReallocFlags(int flags,
     }
 
     const size_t actual_new_size = ActualSize(new_size);
-    actual_old_size = GetSize(ptr);
+    const size_t actual_old_size = GetSize(ptr);
 
     // TODO: note that tcmalloc will "ignore" a downsizing realloc() unless the
     // new size is a significant percentage smaller. We could do the same if we
@@ -635,11 +636,7 @@ void* PartitionRoot<thread_safe>::ReallocFlags(int flags,
     internal::PartitionExcessiveAllocationSize(new_size);
   }
 
-  size_t copy_size = actual_old_size;
-  if (new_size < copy_size)
-    copy_size = new_size;
-
-  memcpy(ret, ptr, copy_size);
+  memcpy(ret, ptr, std::min(old_usable_size, new_size));
   Free(ptr);
   return ret;
 #endif