[llvm] [IR] Value::setNameImpl: fix use-after-free when new name aliases old storage (PR #173258)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-ir

Author: Wenju He (wenju-he)

<details>
<summary>Changes</summary>

When setName() is called with a StringRef derived from the current name, it results in a use-after-free error reported by AddressSanitizer. A newly added test ValueTest.setNameShrink demonstrates the issue. Fix by creating the new ValueName before removing/destroying the old one.

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


2 Files Affected:

- (modified) llvm/lib/IR/Value.cpp (+13-6) 
- (modified) llvm/unittests/IR/ValueTest.cpp (+17) 


``````````diff
diff --git a/llvm/lib/IR/Value.cpp b/llvm/lib/IR/Value.cpp
index b775cbb0c7920..a43d63077bf9f 100644
--- a/llvm/lib/IR/Value.cpp
+++ b/llvm/lib/IR/Value.cpp
@@ -356,21 +356,28 @@ void Value::setNameImpl(const Twine &NewName) {
   if (getSymTab(this, ST))
     return;  // Cannot set a name on this value (e.g. constant).
 
+  ValueName *NewValueName = nullptr;
   if (!ST) { // No symbol table to update?  Just do the change.
+    if (!NameRef.empty()) {
+      // Create the new name.
+      MallocAllocator Allocator;
+      NewValueName = ValueName::create(NameRef, Allocator);
+    }
     // NOTE: Could optimize for the case the name is shrinking to not deallocate
     // then reallocated.
     destroyValueName();
 
-    if (!NameRef.empty()) {
-      // Create the new name.
+    if (NewValueName) {
       assert(NeedNewName);
-      MallocAllocator Allocator;
-      setValueName(ValueName::create(NameRef, Allocator));
+      setValueName(NewValueName);
       getValueName()->setValue(this);
     }
     return;
   }
 
+  if (!NameRef.empty())
+    NewValueName = ST->createValueName(NameRef, this);
+
   // NOTE: Could optimize for the case the name is shrinking to not deallocate
   // then reallocated.
   if (hasName()) {
@@ -383,8 +390,8 @@ void Value::setNameImpl(const Twine &NewName) {
   }
 
   // Name is changing to something new.
-  assert(NeedNewName);
-  setValueName(ST->createValueName(NameRef, this));
+  assert(NeedNewName && NewValueName != nullptr);
+  setValueName(NewValueName);
 }
 
 void Value::setName(const Twine &NewName) {
diff --git a/llvm/unittests/IR/ValueTest.cpp b/llvm/unittests/IR/ValueTest.cpp
index 4d28fe019ecd2..e1df4c216089e 100644
--- a/llvm/unittests/IR/ValueTest.cpp
+++ b/llvm/unittests/IR/ValueTest.cpp
@@ -22,6 +22,23 @@ using namespace llvm;
 
 namespace {
 
+TEST(ValueTest, setNameShrink) {
+  LLVMContext C;
+
+  const char *ModuleString = "define void @f1() {\n"
+                             "bb0:\n"
+                             "  ret void\n"
+                             "}\n";
+  SMDiagnostic Err;
+  std::unique_ptr<Module> M = parseAssemblyString(ModuleString, Err, C);
+
+  Function *F = M->getFunction("f1");
+  StringRef FName = F->getName();
+  FName = FName.drop_back();
+  F->setName(FName);
+  EXPECT_EQ(F->getName(), "f");
+}
+
 TEST(ValueTest, UsedInBasicBlock) {
   LLVMContext C;
 

``````````

</details>


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