[Mlir-commits] [mlir] 4f0262c - Fix use-after-free in SymbolTable::replaceAllSymbolUses

[Mehdi Amini]

Author: Mehdi Amini
Date: 2022-08-02T22:30:17Z
New Revision: 4f0262c1640531dd431cf205f4b802b1fabb6489

URL: https://github.com/llvm/llvm-project/commit/4f0262c1640531dd431cf205f4b802b1fabb6489
DIFF: https://github.com/llvm/llvm-project/commit/4f0262c1640531dd431cf205f4b802b1fabb6489.diff

LOG: Fix use-after-free in SymbolTable::replaceAllSymbolUses

In some cases the recursion will grow the `visited` hash table and
invalidate the cached iterator.
(caught with ASAN)

Differential Revision: https://reviews.llvm.org/D131027

Added: 
    

Modified: 
    mlir/lib/IR/SubElementInterfaces.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/lib/IR/SubElementInterfaces.cpp b/mlir/lib/IR/SubElementInterfaces.cpp
index f8526dc6d3869..a362479e8a2ce 100644
--- a/mlir/lib/IR/SubElementInterfaces.cpp
+++ b/mlir/lib/IR/SubElementInterfaces.cpp
@@ -117,31 +117,39 @@ static void updateSubElementImpl(
 
   // Check for an existing mapping for this element, and walk it if we haven't
   // yet.
-  T &mappedElement = visited[element];
-  if (!mappedElement) {
+  T *mappedElement = &visited[element];
+  if (!*mappedElement) {
     WalkResult result = WalkResult::advance();
-    std::tie(mappedElement, result) = walkFn(element);
+    std::tie(*mappedElement, result) = walkFn(element);
 
     // Try walking this element.
-    if (result.wasInterrupted() || !mappedElement) {
+    if (result.wasInterrupted() || !*mappedElement) {
       changed = failure();
       return;
     }
 
     // Handle replacing sub-elements if this element is also a container.
     if (!result.wasSkipped()) {
-      if (auto interface = mappedElement.template dyn_cast<InterfaceT>()) {
-        if (!(mappedElement = replaceSubElementFn(interface))) {
+      if (auto interface = mappedElement->template dyn_cast<InterfaceT>()) {
+        // Cache the size of the `visited` map since it may grow when calling
+        // `replaceSubElementFn` and we would need to fetch again the (now
+        // invalidated) reference to `mappedElement`.
+        size_t visitedSize = visited.size();
+        auto replacedElement = replaceSubElementFn(interface);
+        if (!replacedElement) {
           changed = failure();
           return;
         }
+        if (visitedSize != visited.size())
+          mappedElement = &visited[element];
+        *mappedElement = replacedElement;
       }
     }
   }
 
   // Update to the mapped element.
-  if (mappedElement != element) {
-    newElements.back() = mappedElement;
+  if (*mappedElement != element) {
+    newElements.back() = *mappedElement;
     changed = true;
   }
 }