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

[Sam McCall via Phabricator via llvm-commits]

sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added a project: All.
sammccall requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

BumpPtrAllocator::Allocate() is marked __attribute__((returns_nonnull)) when the
compiler supports it, which makes it UB to return null.

When there have been no allocations yet, the current slab is [nullptr, nullptr).
A zero-sized allocation fits in this range, and so Allocate(0, 1) returns null.

There's no explicit docs whether Allocate(0) is valid. I think we have to assume
that it is:

- the implementation tries to support it (e.g. >= tests instead of >)
- malloc(0) is allowed
- requiring each callsite to do a check is bug-prone
- I found real LLVM code that makes zero-sized allocations


Repository:
  rG LLVM Github Monorepo

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
@@ -154,7 +154,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.427438.patch
Type: text/x-patch
Size: 2019 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220505/2eaf1042/attachment.bin>