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

[Ziqing Luo via cfe-commits]

https://github.com/ziqingluo-90 created https://github.com/llvm/llvm-project/pull/124477

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)

>From afdf1445e95e25503505d3c3dd20cdba99742ad3 Mon Sep 17 00:00:00 2001
From: Ziqing Luo <ziqing_luo at apple.com>
Date: Sun, 26 Jan 2025 10:54:49 -0800
Subject: [PATCH] [StaticAnalyzer] Fix state update in
 VisitObjCForCollectionStmt

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)
---
 .../StaticAnalyzer/Core/ExprEngineObjC.cpp    | 30 ++++++++++---------
 1 file changed, 16 insertions(+), 14 deletions(-)

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,