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

[Ziqing Luo via cfe-commits]

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

>From 635cba186fb9cda4718263caa0d349729279390d Mon Sep 17 00:00:00 2001
From: Ziqing Luo <ziqing_luo at apple.com>
Date: Thu, 30 Jan 2025 12:24:45 -0800
Subject: [PATCH 1/2] [StaticAnalyzer] Add a reproducer for a bug in
 VisitObjCForCollectionStmt

The bug in VisitObjCForCollectionStmt seems have been there for a long
time and can be very rarely triggered.  The adding test is reduced
from a crash observed downstream that reproduces the bug.

(#124477)
(rdar://143280254)
---
 clang/test/Analysis/bugfix-124477.m | 39 +++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 clang/test/Analysis/bugfix-124477.m

diff --git a/clang/test/Analysis/bugfix-124477.m b/clang/test/Analysis/bugfix-124477.m
new file mode 100644
index 00000000000000..80820f4c934443
--- /dev/null
+++ b/clang/test/Analysis/bugfix-124477.m
@@ -0,0 +1,39 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,apiModeling,nullability.NullableDereferenced,nullability.NullabilityBase -x objective-c %s
+/*
+  This test is reduced from a static analyzer crash. The bug causing
+  the crash is explained in #124477.  It can only be triggered in some
+  rare cases so please do not modify this reproducer.
+*/
+
+#pragma clang assume_nonnull begin
+# 15 "some-sys-header.h" 1 3
+ at class NSArray, NSObject;
+
+ at interface Base
+ at property (readonly, copy) NSArray *array;
+ at end
+
+#pragma clang assume_nonnull end
+# 8 "this-file.m" 2
+
+
+ at interface Test : Base
+
+ at property (readwrite, copy, nullable) NSObject *label;
+ at property (readwrite, strong, nullable) Test * field;
+
+- (void)f;
+
+ at end
+
+ at implementation Test
+- (void)f
+{
+  NSObject * X;
+
+  for (NSObject *ele in self.field.array) {}
+  self.label = X;  
+}
+ at end
+
+

>From 941ea48895ba7d7467981b55804a03852d88ab8d 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 2/2] [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.

(#124477)
(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..9426e0afd65a04 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 DstLocation; // states in `DstLocation` may differ from `Pred`
+  evalLocation(DstLocation, S, elem, Pred, state, elementV, false);
 
-  ExplodedNodeSet Tmp;
-  StmtNodeBuilder Bldr(Pred, Tmp, *currBldrCtx);
+  for (ExplodedNode *dstLocation : DstLocation) {
+    ExplodedNodeSet DstLocationSingleton{dstLocation}, Tmp;
+    StmtNodeBuilder Bldr(dstLocation, Tmp, *currBldrCtx);
 
-  if (!isContainerNull)
-    populateObjCForDestinationSet(dstLocation, svalBuilder, S, elem, elementV,
-                                  SymMgr, currBldrCtx, Bldr,
-                                  /*hasElements=*/true);
+    if (!isContainerNull)
+      populateObjCForDestinationSet(DstLocationSingleton, svalBuilder, S, elem,
+                                    elementV, SymMgr, currBldrCtx, Bldr,
+                                    /*hasElements=*/true);
 
-  populateObjCForDestinationSet(dstLocation, svalBuilder, S, elem, elementV,
-                                SymMgr, currBldrCtx, Bldr,
-                                /*hasElements=*/false);
+    populateObjCForDestinationSet(DstLocationSingleton, 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,