Page MenuHomePhabricator

[LowerConstantIntrinsics] Fix heap-use-after-free bug in worklist
ClosedPublic

Authored by dstenb on Sep 3 2021, 3:49 AM.

Details

Summary

This fixes PR51730, a heap-use-after-free bug in
replaceConditionalBranchesOnConstant().

With the attached reproducer we were left with a function looking
something like this after replaceAndRecursivelySimplify():

[...]

cont2.i:
  br i1 %.not1.i, label %handler.type_mismatch3.i, label %cont4.i

handler.type_mismatch3.i:
  %3 = phi i1 [ %2, %cont2.thread.i ], [ false, %cont2.i ]
  unreachable

cont4.i:
  unreachable

[...]

with both the branch instruction and PHI node being in the worklist. As
a result of replacing the branch instruction with an unconditional
branch, the PHI node in %handler.type_mismatch3.i would be removed. This
then resulted in a heap-use-after-free bug due to accessing that removed
PHI node in the next worklist iteration.

This is solved by using a value handle worklist. I am a unsure if this
is the most idiomatic solution. Another solution could have been to
produce a worklist just containing the interesting branch instructions,
but I thought that it perhaps was a bit cleaner to keep all worklist
filtering in the loop that does the rewrites.

Diff Detail

Event Timeline

dstenb created this revision.Sep 3 2021, 3:49 AM
dstenb requested review of this revision.Sep 3 2021, 3:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2021, 3:49 AM
nickdesaulniers added inline comments.Sep 7 2021, 2:30 PM
llvm/lib/Transforms/Scalar/LowerConstantIntrinsics.cpp
61–68

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)?

dstenb added inline comments.Sep 9 2021, 6:49 AM
llvm/lib/Transforms/Scalar/LowerConstantIntrinsics.cpp
61–68

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.

lebedev.ri accepted this revision.Sep 9 2021, 6:54 AM

LGTM
While you could use CallbackVH with a lambda that would erase from the set, the set contains instructions, not VH's, so this seems like the smallest fix.

This revision is now accepted and ready to land.Sep 9 2021, 6:54 AM

Thanks for the reviews! I'll land this shortly.

This revision was landed with ongoing or failed builds.Sep 21 2021, 2:33 AM
This revision was automatically updated to reflect the committed changes.