[PATCH] D109221: [LowerConstantIntrinsics] Fix heap-use-after-free bug in worklist

[David Stenberg via Phabricator via llvm-commits]

dstenb added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LowerConstantIntrinsics.cpp:61-68
+  // UnsimplifiedUsers can contain PHI nodes that may be removed when
+  // replacing the branch instructions, so use a value handle worklist
+  // to handle those possibly removed instructions.
+  SmallVector<WeakVH, 8> Worklist(UnsimplifiedUsers.begin(),
+                                  UnsimplifiedUsers.end());
+
+  for (auto &VH : Worklist) {
----------------
nickdesaulniers wrote:
> Why make a copy of the worklist? Should we just be using `dyn_cast_or_null` rather than `dyn_cast` (ie. without all of the other changes)?
The extra code is needed because the worklist would not contain null pointers but rather stale pointers to deleted instructions.

With the attached reproducer we will end up with the following two instructions in the worklist:

```
* br i1 false, label %handler.type_mismatch3.i, label %cont4.i
  ptr=0x564e36fac680, parent=cont2.i

* %1 = phi i1 [ true, %cont2.thread.i ], [ false, %cont2.i ]
  ptr=0x564e36fac6d8, parent=handler.type_mismatch3.i
```

In the first iteration, the branch instruction is replaced with an
unconditional branch to %cont4.i. As a part of that, %cont2.i is removed as a
predecessor to %handler.type_mismatch3.i via removePredecessor(), which results
in the PHI node, which is in the worklist, being deleted. Before this patch, we then got a
heap-use-after-free bug in the next iteration when trying to use dyn_cast on the
deleted PHI.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109221/new/

https://reviews.llvm.org/D109221