[clang] [StaticAnalyzer] Fix state update in VisitObjCForCollectionStmt (PR #124477)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Ziqing Luo (ziqingluo-90)

<details>
<summary>Changes</summary>

In `VisitObjCForCollectionStmt`, the function does `evalLocation` for the current element at the original source state `Pred`. The evaluation may result in a new state, say `PredNew`. I.e., there is a transition: `Pred -> PredNew`, though it is a very rare case that `Pred` is NOT identical to `PredNew`. (This explains why the bug exists for many years but no one noticed until recently a crash observed downstream.)  Later, the original code does NOT use `PredNew` as the new source state in `StmtNodeBuilder` for next transitions.  In cases `Pred != PredNew`, the program ill behaves.

(rdar://143280254)

---
Full diff: https://github.com/llvm/llvm-project/pull/124477.diff


1 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp (+16-14) 


``````````diff
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
index f075df3ab5e4d6..9aa5cee71c1374 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
@@ -124,24 +124,26 @@ void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S,
 
   bool isContainerNull = state->isNull(collectionV).isConstrainedTrue();
 
-  ExplodedNodeSet dstLocation;
-  evalLocation(dstLocation, S, elem, Pred, state, elementV, false);
+  ExplodedNodeSet NewPreds; // evalLocation may alter `Pred`
+  evalLocation(NewPreds, S, elem, Pred, state, elementV, false);
 
-  ExplodedNodeSet Tmp;
-  StmtNodeBuilder Bldr(Pred, Tmp, *currBldrCtx);
+  for (ExplodedNode *Pred : NewPreds) {
+    ExplodedNodeSet PredSingleton{Pred}, Tmp;
+    StmtNodeBuilder Bldr(Pred, Tmp, *currBldrCtx);
 
-  if (!isContainerNull)
-    populateObjCForDestinationSet(dstLocation, svalBuilder, S, elem, elementV,
-                                  SymMgr, currBldrCtx, Bldr,
-                                  /*hasElements=*/true);
+    if (!isContainerNull)
+      populateObjCForDestinationSet(PredSingleton, svalBuilder, S, elem,
+                                    elementV, SymMgr, currBldrCtx, Bldr,
+                                    /*hasElements=*/true);
 
-  populateObjCForDestinationSet(dstLocation, svalBuilder, S, elem, elementV,
-                                SymMgr, currBldrCtx, Bldr,
-                                /*hasElements=*/false);
+    populateObjCForDestinationSet(PredSingleton, svalBuilder, S, elem, elementV,
+                                  SymMgr, currBldrCtx, Bldr,
+                                  /*hasElements=*/false);
 
-  // Finally, run any custom checkers.
-  // FIXME: Eventually all pre- and post-checks should live in VisitStmt.
-  getCheckerManager().runCheckersForPostStmt(Dst, Tmp, S, *this);
+    // Finally, run any custom checkers.
+    // FIXME: Eventually all pre- and post-checks should live in VisitStmt.
+    getCheckerManager().runCheckersForPostStmt(Dst, Tmp, S, *this);
+  }
 }
 
 void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME,

``````````

</details>


https://github.com/llvm/llvm-project/pull/124477