[PATCH] D125040: [Support] Fix UB in BumpPtrAllocator when first allocation is zero.

[Sam McCall via Phabricator via llvm-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rGba0d50ad7ec6: [Support] Fix UB in BumpPtrAllocator when first allocation is zero. (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D125040?vs=427438&id=427536#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125040/new/

https://reviews.llvm.org/D125040

Files:
  llvm/include/llvm/Support/Allocator.h
  llvm/unittests/Support/AllocatorTest.cpp


Index: llvm/unittests/Support/AllocatorTest.cpp
===================================================================
--- llvm/unittests/Support/AllocatorTest.cpp
+++ llvm/unittests/Support/AllocatorTest.cpp
@@ -99,6 +99,31 @@
   EXPECT_EQ(0U, a & 127);
 }
 
+// Test zero-sized allocations.
+// In general we don't need to allocate memory for these.
+// However Allocate never returns null, so if the first allocation is zero-sized
+// we end up creating a slab for it.
+TEST(AllocatorTest, TestZero) {
+  BumpPtrAllocator Alloc;
+  EXPECT_EQ(0u, Alloc.GetNumSlabs());
+  EXPECT_EQ(0u, Alloc.getBytesAllocated());
+
+  void *Empty = Alloc.Allocate(0, 1);
+  EXPECT_NE(Empty, nullptr) << "Allocate is __attribute__((returns_nonnull))";
+  EXPECT_EQ(1u, Alloc.GetNumSlabs()) << "Allocated a slab to point to";
+  EXPECT_EQ(0u, Alloc.getBytesAllocated());
+
+  void *Large = Alloc.Allocate(4096, 1);
+  EXPECT_EQ(1u, Alloc.GetNumSlabs());
+  EXPECT_EQ(4096u, Alloc.getBytesAllocated());
+  EXPECT_EQ(Empty, Large);
+
+  void *Empty2 = Alloc.Allocate(0, 1);
+  EXPECT_NE(Empty2, nullptr);
+  EXPECT_EQ(1u, Alloc.GetNumSlabs());
+  EXPECT_EQ(4096u, Alloc.getBytesAllocated());
+}
+
 // Test allocating just over the slab size.  This tests a bug where before the
 // allocator incorrectly calculated the buffer end pointer.
 TEST(AllocatorTest, TestOverflow) {
Index: llvm/include/llvm/Support/Allocator.h
===================================================================
--- llvm/include/llvm/Support/Allocator.h
+++ llvm/include/llvm/Support/Allocator.h
@@ -140,6 +140,9 @@
   // This method is *not* marked noalias, because
   // SpecificBumpPtrAllocator::DestroyAll() loops over all allocations, and
   // that loop is not based on the Allocate() return value.
+  //
+  // Allocate(0, N) is valid, it returns a non-null pointer (which should not
+  // be dereferenced).
   LLVM_ATTRIBUTE_RETURNS_NONNULL void *Allocate(size_t Size, Align Alignment) {
     // Keep track of how many bytes we've allocated.
     BytesAllocated += Size;
@@ -154,7 +157,9 @@
 #endif
 
     // Check if we have enough space.
-    if (Adjustment + SizeToAllocate <= size_t(End - CurPtr)) {
+    if (Adjustment + SizeToAllocate <= size_t(End - CurPtr)
+        // We can't return nullptr even for a zero-sized allocation!
+        && CurPtr != nullptr) {
       char *AlignedPtr = CurPtr + Adjustment;
       CurPtr = AlignedPtr + SizeToAllocate;
       // Update the allocation point of this memory block in MemorySanitizer.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D125040.427536.patch
Type: text/x-patch
Size: 2501 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220506/c116f620/attachment.bin>