[clang] [Clang] Made COMDAT key of a global structor be a WeakTrackingVH (PR #219137)

[via cfe-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-codegen

Author: Soham Karandikar (skadewdl3)

<details>
<summary>Changes</summary>

FIxes #<!-- -->213520.

`CodeGenModule::Structor::AssociatedData` is a raw `llvm::Constant*` used as the COMDAT key in `@<!-- -->llvm.global_ctors`. Since it isn't an IR used, a RAUW call from `GetOrCreateLLVMGlobal` can leave it dangling. Reading this pointer leads to a segfault down the line in `AsmPrinter`.

The error would not surface in GCC, but would do so in Clang 22.1.2. The bug is reproducible on trunk Clang with ASAN enabled. See [this](https://github.com/llvm/llvm-project/issues/213520#issuecomment-5314618072) and [this](https://github.com/llvm/llvm-project/issues/213520#issuecomment-5353945202)  for more details.

The `-fopenmp` option causes emission of the global `g` to be deferred. It is emitted when `CodeGebModule::EmitDeferred` runs. In the reproducer, the deferred list emits globals as follows:
- Function `a`: Declaration for `g` is emitted with type `%T`.
- `g` (used in `a`):
  - `GetAddrOfGlobal` wants `%T`.
  - A subsequent call to `EmitGlobalVarDefinition` wants `{ i32 }`.
  - Type of `g` declaration is replacesd with `{ i32 }`, since initializer type takes precedence.
- Function `b`: `g` is already declared.
- `g` (used in `b`):
  - `GetAddrOfGlobal` wants `%T`, but `g` is defined with type `{ i32 }`.
  - `g` gets freed and a new `g` is built with type `%T`.
  - RAUW called to replace uses of the old `g` with the new one.
 
However, even after the RAUW, the COMDAT key (`Structor::AssociatedData`) is an `llvm::Constant*` - so it keeps pointing to the old (now freed) `g`. Hence, a heap use-after-free is detected by ASAN when the constructor list is being emitted in `EmitCtorList`.

This PR makes `Structor::AssociatedData` a `llvm::WeakTrackingVH`, so RAUW can fix it up too when it replaces the old `g` with the new one.

---

Assisted-by: Cursor
I used Cursor to figure out that the bug only occurred with a [specific version of Glibc](https://github.com/llvm/llvm-project/issues/213520#issuecomment-5314618072) which hadn't occurred to me. I also used Cursor to understand how Clang emits globals.

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


3 Files Affected:

- (modified) clang/lib/CodeGen/CodeGenModule.cpp (+1-1) 
- (modified) clang/lib/CodeGen/CodeGenModule.h (+4-1) 
- (added) clang/test/CodeGenCXX/gh213520-global-ctor-comdat.cpp (+22) 


``````````diff
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 348d5e579b9c3..72f7a61b89300 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -2754,7 +2754,7 @@ void CodeGenModule::EmitCtorList(CtorList &Fns, const char *GlobalName) {
     Ctor.addInt(Int32Ty, I.Priority);
     Ctor.add(I.Initializer);
     if (I.AssociatedData)
-      Ctor.add(I.AssociatedData);
+      Ctor.add(cast<llvm::Constant>(I.AssociatedData));
     else
       Ctor.addNullPointer(PtrTy);
     Ctor.finishAndAddTo(Ctors);
diff --git a/clang/lib/CodeGen/CodeGenModule.h b/clang/lib/CodeGen/CodeGenModule.h
index 1f5ecf734c528..13b31d542e39a 100644
--- a/clang/lib/CodeGen/CodeGenModule.h
+++ b/clang/lib/CodeGen/CodeGenModule.h
@@ -349,7 +349,10 @@ class CodeGenModule : public CodeGenTypeCache {
     int Priority;
     unsigned LexOrder;
     llvm::Constant *Initializer;
-    llvm::Constant *AssociatedData;
+    // GetOrCreateLLVMGlobal can replace and erase this global before
+    // EmitCtorList runs, so it should be a WeakTrackinhVH for RAUW to fix it up
+    // properly.
+    llvm::WeakTrackingVH AssociatedData;
   };
 
   typedef std::vector<Structor> CtorList;
diff --git a/clang/test/CodeGenCXX/gh213520-global-ctor-comdat.cpp b/clang/test/CodeGenCXX/gh213520-global-ctor-comdat.cpp
new file mode 100644
index 0000000000000..782ccc4760ea5
--- /dev/null
+++ b/clang/test/CodeGenCXX/gh213520-global-ctor-comdat.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++17 -fopenmp \
+// RUN:     -emit-llvm -o - %s | FileCheck %s
+
+struct Base {
+  int x;
+  constexpr Base() : x(0) {}
+};
+
+struct T : Base {
+  ~T();
+};
+
+struct S;
+
+template <class Tag>
+inline T g;
+
+T *a() { return &g<S>; }
+T *b() { return &g<S>; }
+
+// The COMDAT key must be the surviving global, not null or a freed value.
+// CHECK: @llvm.global_ctors = appending global {{.*}} { i32 65535, ptr @__cxx_global_var_init{{.*}}, ptr @_Z1gI1SE }

``````````

</details>


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