[PATCH] D72529: [SCCIterator] Fix use-after-free

[Keno Fischer via Phabricator via llvm-commits]

loladiro created this revision.
loladiro added a reviewer: lebedev.ri.
Herald added subscribers: llvm-commits, dexonsmith.
Herald added a project: LLVM.

The line at issue is the following:

  nodeVisitNumbers[New] = nodeVisitNumbers[Old];

If we write this as:

  unsigned &OldRef = nodeVisitNumbers[Old];
  unsigned &NewRef = nodeVisitNumbers[New];
  unsigned OldVal = OldRef;
  NewRef = OldVal;

the issue becomes obvious: The call to `nodeVisitNumbers[New]` may
invalidate the reference from the `nodeVisitNumbers[Old]`, causing
the subsequent reference to value conversion to read free'ed memory.
The rewritten evaluation order is valid, because there are no sequence
points among the LHS and RHS value expressions, so either evaluation
order is acceptable. However, as described one of them results in a
use-after-free. However, the resulting crash is highly compiler and
runtime dependent (since the use happens immediately after the free,
there's only an issue if the memory gets freed or re-used by a
different thread).

An obvious fix is just to manually sequence the operations. However,
I would be concerned that this issue could be re-introduced later
by somebody assuming that removing the intermediate variable is NFC.
Instead, switch the access to use the iterator interface. This has
two advantages:

1. When LLVM is build with debug checks, iterator access is validated, to catch these kind of issues.
2. We save ourselves a bucket lookup, because we can re-use the iterator for erasure, increasing performance.

We believe this issue to be the root cause behind https://bugs.llvm.org/show_bug.cgi?id=34480.

Co-authored-by: Valentin Churavy <vchuravy at mit.edu>


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72529

Files:
  llvm/include/llvm/ADT/SCCIterator.h


Index: llvm/include/llvm/ADT/SCCIterator.h
===================================================================
--- llvm/include/llvm/ADT/SCCIterator.h
+++ llvm/include/llvm/ADT/SCCIterator.h
@@ -133,9 +133,11 @@
   /// This informs the \c scc_iterator that the specified \c Old node
   /// has been deleted, and \c New is to be used in its place.
   void ReplaceNode(NodeRef Old, NodeRef New) {
-    assert(nodeVisitNumbers.count(Old) && "Old not in scc_iterator?");
-    nodeVisitNumbers[New] = nodeVisitNumbers[Old];
-    nodeVisitNumbers.erase(Old);
+    auto it = nodeVisitNumbers.find(Old);
+    assert(it != nodeVisitNumbers.end() && "Old not in scc_iterator?");
+    unsigned OldVisitNumber = it->second;
+    nodeVisitNumbers.erase(it);
+    nodeVisitNumbers[New] = OldVisitNumber;
   }
 };
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D72529.237403.patch
Type: text/x-patch
Size: 801 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200110/2083c101/attachment.bin>