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

[Reid Kleckner via llvm-commits]

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

>From 682545aade48d3c53e7e8a30add575993f4065c5 Mon Sep 17 00:00:00 2001
From: Reid Kleckner <rkleckner at nvidia.com>
Date: Thu, 4 Dec 2025 00:02:00 +0000
Subject: [PATCH 1/3] [IR] Fix User use-after-destroy by zapping in ~User

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
---
 llvm/lib/IR/User.cpp | 65 ++++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 26 deletions(-)

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]); }

>From 2b36b86cbdfabb14b8ae76cade9d2da980b376b1 Mon Sep 17 00:00:00 2001
From: Reid Kleckner <rkleckner at nvidia.com>
Date: Thu, 4 Dec 2025 00:09:35 +0000
Subject: [PATCH 2/3] Include missing User.h change

---
 llvm/include/llvm/IR/User.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/llvm/include/llvm/IR/User.h b/llvm/include/llvm/IR/User.h
index cbb4379b68c41..d980dd194b987 100644
--- a/llvm/include/llvm/IR/User.h
+++ b/llvm/include/llvm/IR/User.h
@@ -141,7 +141,8 @@ class User : public Value {
   LLVM_ABI void growHungoffUses(unsigned N, bool IsPhi = false);
 
 protected:
-  ~User() = default; // Use deleteValue() to delete a generic Instruction.
+  // Use deleteValue() to delete a generic User.
+  ~User();
 
 public:
   User(const User &) = delete;

>From 259dc2bdf88dc94fff69dc9929e289503a66cbe0 Mon Sep 17 00:00:00 2001
From: Reid Kleckner <rnk at llvm.org>
Date: Mon, 22 Dec 2025 08:38:35 -0800
Subject: [PATCH 3/3] Special case ConstantData instances to avoid allocating
 an extra pointer of data

---
 llvm/include/llvm/IR/Constants.h |  4 ++--
 llvm/lib/IR/User.cpp             | 10 +++++++---
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/llvm/include/llvm/IR/Constants.h b/llvm/include/llvm/IR/Constants.h
index af9d25dd7fcd8..3bb2fa28cced8 100644
--- a/llvm/include/llvm/IR/Constants.h
+++ b/llvm/include/llvm/IR/Constants.h
@@ -65,10 +65,10 @@ class ConstantData : public Constant {
 protected:
   explicit ConstantData(Type *Ty, ValueTy VT) : Constant(Ty, VT, AllocMarker) {}
 
-  void *operator new(size_t S) { return User::operator new(S, AllocMarker); }
+  void *operator new(size_t S) { return ::operator new(S); }
 
 public:
-  void operator delete(void *Ptr) { User::operator delete(Ptr); }
+  void operator delete(void *Ptr) { ::operator delete(Ptr); }
 
   ConstantData(const ConstantData &) = delete;
 
diff --git a/llvm/lib/IR/User.cpp b/llvm/lib/IR/User.cpp
index 0fce676090955..e97a07fe5e7a2 100644
--- a/llvm/lib/IR/User.cpp
+++ b/llvm/lib/IR/User.cpp
@@ -8,6 +8,7 @@
 
 #include "llvm/IR/User.h"
 #include "llvm/IR/Constant.h"
+#include "llvm/IR/Constants.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/IntrinsicInst.h"
 
@@ -224,9 +225,12 @@ User::~User() {
 
   // 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;
+  // object memory. The `User` new operator always allocates least one pointer
+  // before the User, so we can use that to store the allocation start. As a
+  // special case, we avoid this extra prefix allocation for ConstantData
+  // instances, since those are extremely common.
+  if (!isa<ConstantData>(this))
+    ((void **)this)[-1] = AllocStart;
 }
 
 void User::operator delete(void *Usr) { ::operator delete(((void **)Usr)[-1]); }