Page MenuHomePhabricator

[SCCIterator] Fix another potential use-after-free
Needs ReviewPublic

Authored by loladiro on Jan 10 2020, 6:06 PM.



This is a follow up to Since this
code is a bit old, I decided to see if there were any similar issues
hiding and believe I have found one. The Inliner transform deletes
CallGraphNodes, but doesn't remove the deleted node from the scc
iterator. At first glance, one might think this is fine, because
we don't access the CallGraphNodes through that pointer and it
simply serves as an indication whether or not we have visited
a given CallGraphNode. However, there is a subtle problem:
The pointer of the CallGraphNode we have freed may be re-used
by a later allocation of a new CallGraphNode. This could now
present a problem, because the scc iterator will associate the
old state with the new CallGraphNode. Fix that by adding the
ability to inform the scc iterator of node deletion.

I suspect this situation never actually causes a problem in
practice because the sequence of events (node deleted -> node
reallocated with the same pointer -> node accessed in CFG
walk) is quite long and may not actually lead to a crash or
even incorrect code (I believe the symptom would be a missing
node in the SCC). I'd like to get it fixed anyway. As a bonus,
I believe we can remove an outstanding todo from the Inliner.

Event Timeline

loladiro created this revision.Jan 10 2020, 6:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2020, 6:06 PM
loladiro updated this revision to Diff 237769.Jan 13 2020, 1:39 PM

Turns out this doesn't quite yet address the TODO in Inliner.
Reverted that part of this change for a complete solution in a future

This is a follow up to

For reference, we independently encountered a similar issue to the one described in the above review, and we proposed a fix (at D72469) that has now been committed.

What's the status here?


I'm suspecting this might or might not be relevant for the
failures from before rG0c22cb0.
Is anything holding this patch up? Can we land this?

jdoerfert added inline comments.Apr 9 2020, 7:59 PM

This part actually landed by now.