[llvm] [IR] Fix User use-after-destroy by zapping in ~User (PR #170575)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-ir

Author: Reid Kleckner (rnk)

<details>
<summary>Changes</summary>

First, this moves the removal of operands from use lists from `User::operator delete` to `User::~User`. This is straightforward, and nothing blocks that.

However, the second complication is that `User::operator delete` needs to recover the start of the allocation, and it needs to recover that information somehow without examining the fields of the `User` object. The natural way to handle this is for the destructor to return an adjusted `this` pointer, and that's in fact how deleting destructors are often implemented, but it requires making assumptions about the C++ ABI. Instead, it seems practical to store the information into the operand memory, to the left of `this`, and to reload the start of the allocation from `((void**)this)[-1]` after the destructor runs. The downside is that zero-operand Users such as `ret void` must allocate more memory. I have not yet gathered any real data on the memory usage impact, but in general, I believe there are very few zero-operand Users, so the impact should be minimal. I'm open to other creative suggestions for how to avoid this overhead, but I don't see any other good way to do it.

This makes LLVM more compatible with bug finding tools like MSan, GCC `-flifetime-dse`, and forthcoming enhancements to Clang itself through `dead_on_return` annotations.

Fixes issue #<!-- -->24952

---
Full diff: https://github.com/llvm/llvm-project/pull/170575.diff


1 Files Affected:

- (modified) llvm/lib/IR/User.cpp (+39-26) 


``````````diff
diff --git a/llvm/lib/IR/User.cpp b/llvm/lib/IR/User.cpp
index 9bb7c1298593a..e8c11b5ce82f6 100644
--- a/llvm/lib/IR/User.cpp
+++ b/llvm/lib/IR/User.cpp
@@ -144,19 +144,24 @@ void *User::allocateFixedOperandUser(size_t Size, unsigned Us,
   assert(DescBytesToAllocate % sizeof(void *) == 0 &&
          "We need this to satisfy alignment constraints for Uses");
 
-  uint8_t *Storage = static_cast<uint8_t *>(
-      ::operator new(Size + sizeof(Use) * Us + DescBytesToAllocate));
-  Use *Start = reinterpret_cast<Use *>(Storage + DescBytesToAllocate);
-  Use *End = Start + Us;
-  User *Obj = reinterpret_cast<User *>(End);
+  size_t LeadingSize = DescBytesToAllocate + sizeof(Use) * Us;
+
+  // Ensure we allocate at least one pointer's worth of space before the main
+  // user allocation. We use this memory to pass information from the destructor
+  // to the deletion operator, so it can recover the true allocation start.
+  LeadingSize = std::max(LeadingSize, sizeof(void *));
+
+  uint8_t *Storage = static_cast<uint8_t *>(::operator new(LeadingSize + Size));
+  User *Obj = reinterpret_cast<User *>(Storage + LeadingSize);
+  Use *Operands = reinterpret_cast<Use *>(Obj) - Us;
   Obj->NumUserOperands = Us;
   Obj->HasHungOffUses = false;
   Obj->HasDescriptor = DescBytes != 0;
-  for (; Start != End; Start++)
-    new (Start) Use(Obj);
+  for (unsigned I = 0; I < Us; ++I)
+    new (&Operands[I]) Use(Obj);
 
   if (DescBytes != 0) {
-    auto *DescInfo = reinterpret_cast<DescriptorInfo *>(Storage + DescBytes);
+    auto *DescInfo = reinterpret_cast<DescriptorInfo *>(Operands) - 1;
     DescInfo->SizeInBytes = DescBytes;
   }
 
@@ -189,31 +194,39 @@ void *User::operator new(size_t Size, HungOffOperandsAllocMarker) {
 //                         User operator delete Implementation
 //===----------------------------------------------------------------------===//
 
-// Repress memory sanitization, due to use-after-destroy by operator
-// delete. Bug report 24578 identifies this issue.
-LLVM_NO_SANITIZE_MEMORY_ATTRIBUTE void User::operator delete(void *Usr) {
+User::~User() {
   // Hung off uses use a single Use* before the User, while other subclasses
   // use a Use[] allocated prior to the user.
-  User *Obj = static_cast<User *>(Usr);
-  if (Obj->HasHungOffUses) {
-    assert(!Obj->HasDescriptor && "not supported!");
+  void *AllocStart = nullptr;
+  if (HasHungOffUses) {
+    assert(!HasDescriptor && "not supported!");
 
-    Use **HungOffOperandList = static_cast<Use **>(Usr) - 1;
+    Use **HungOffOperandList = reinterpret_cast<Use **>(this) - 1;
     // drop the hung off uses.
-    Use::zap(*HungOffOperandList, *HungOffOperandList + Obj->NumUserOperands,
+    Use::zap(*HungOffOperandList, *HungOffOperandList + NumUserOperands,
              /* Delete */ true);
-    ::operator delete(HungOffOperandList);
-  } else if (Obj->HasDescriptor) {
-    Use *UseBegin = static_cast<Use *>(Usr) - Obj->NumUserOperands;
-    Use::zap(UseBegin, UseBegin + Obj->NumUserOperands, /* Delete */ false);
+    AllocStart = HungOffOperandList;
+  } else if (HasDescriptor) {
+    Use *UseBegin = reinterpret_cast<Use *>(this) - NumUserOperands;
+    Use::zap(UseBegin, UseBegin + NumUserOperands, /* Delete */ false);
 
     auto *DI = reinterpret_cast<DescriptorInfo *>(UseBegin) - 1;
-    uint8_t *Storage = reinterpret_cast<uint8_t *>(DI) - DI->SizeInBytes;
-    ::operator delete(Storage);
-  } else {
-    Use *Storage = static_cast<Use *>(Usr) - Obj->NumUserOperands;
-    Use::zap(Storage, Storage + Obj->NumUserOperands,
+    AllocStart = reinterpret_cast<uint8_t *>(DI) - DI->SizeInBytes;
+  } else if (NumUserOperands > 0) {
+    Use *Storage = reinterpret_cast<Use *>(this) - NumUserOperands;
+    Use::zap(Storage, Storage + NumUserOperands,
              /* Delete */ false);
-    ::operator delete(Storage);
+    AllocStart = Storage;
+  } else {
+    // Handle the edge case where there are no operands and no descriptor.
+    AllocStart = (void **)(this) - 1;
   }
+
+  // Operator delete needs to know where the allocation started. To avoid
+  // use-after-destroy, we have to store the allocation start outside the User
+  // object memory. We always have at least one Use* before the User, so we can
+  // use that to store the allocation start.
+  ((void **)this)[-1] = AllocStart;
 }
+
+void User::operator delete(void *Usr) { ::operator delete(((void **)Usr)[-1]); }

``````````

</details>


https://github.com/llvm/llvm-project/pull/170575