Page MenuHomePhabricator

Fix use-after-free bug in AffectedValueCallbackVH::allUsesReplacedWith
ClosedPublic

Authored by hfinkel on Jan 15 2017, 2:38 PM.

Details

Summary

When transferring affected values in the cache from an old value, identified by the value of the current callback, to the specified new value we might need to insert a new entry into the DenseMap which constitutes the cache. Doing so might delete the current callback object. Move the copying logic into a new function, a member of the assumption cache itself, so that we don't run into UB should the callback handle itself be removed mid-copy.

Diff Detail

Repository
rL LLVM

Event Timeline

hfinkel updated this revision to Diff 84505.Jan 15 2017, 2:38 PM
hfinkel retitled this revision from to Fix use-after-free bug in AffectedValueCallbackVH::allUsesReplacedWith.
hfinkel updated this object.
hfinkel added reviewers: dim, chandlerc.
hfinkel added a subscriber: llvm-commits.
sanjoy accepted this revision.Jan 15 2017, 3:32 PM
sanjoy added a reviewer: sanjoy.
sanjoy added a subscriber: sanjoy.

The code lgtm, but

  • Can you easily test this using a C++ unit test? I'll settle for something that normally runs "fine", but trips asan if the build is using asan.
  • getAffectedValues should be renamed to getOrInsertAffectedValues.
lib/Analysis/AssumptionCache.cpp
121 ↗(On Diff #84505)

Can you use llvm::find here (in a later change)?

132 ↗(On Diff #84505)

Please add why it may dangle.

This revision is now accepted and ready to land.Jan 15 2017, 3:32 PM
emaste added a subscriber: emaste.Jan 15 2017, 4:10 PM
hfinkel marked an inline comment as done.Jan 16 2017, 6:58 AM

The code lgtm, but

  • Can you easily test this using a C++ unit test? I'll settle for something that normally runs "fine", but trips asan if the build is using asan.

I'll experiment with this today and commit one in follow-up. I might also be able to create an IR test case for instcombine if things work out just right.

  • getAffectedValues should be renamed to getOrInsertAffectedValues.

Sure.

lib/Analysis/AssumptionCache.cpp
121 ↗(On Diff #84505)

Yes, I think so. I'd forgotten about that.

This revision was automatically updated to reflect the committed changes.