Skip to content

Commit 941ea48

Browse files
committed
[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. (llvm#124477) (rdar://143280254)
1 parent 635cba1 commit 941ea48

File tree

1 file changed

+16
-14
lines changed

1 file changed

+16
-14
lines changed

clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -124,24 +124,26 @@ void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S,
124124

125125
bool isContainerNull = state->isNull(collectionV).isConstrainedTrue();
126126

127-
ExplodedNodeSet dstLocation;
128-
evalLocation(dstLocation, S, elem, Pred, state, elementV, false);
127+
ExplodedNodeSet DstLocation; // states in `DstLocation` may differ from `Pred`
128+
evalLocation(DstLocation, S, elem, Pred, state, elementV, false);
129129

130-
ExplodedNodeSet Tmp;
131-
StmtNodeBuilder Bldr(Pred, Tmp, *currBldrCtx);
130+
for (ExplodedNode *dstLocation : DstLocation) {
131+
ExplodedNodeSet DstLocationSingleton{dstLocation}, Tmp;
132+
StmtNodeBuilder Bldr(dstLocation, Tmp, *currBldrCtx);
132133

133-
if (!isContainerNull)
134-
populateObjCForDestinationSet(dstLocation, svalBuilder, S, elem, elementV,
135-
SymMgr, currBldrCtx, Bldr,
136-
/*hasElements=*/true);
134+
if (!isContainerNull)
135+
populateObjCForDestinationSet(DstLocationSingleton, svalBuilder, S, elem,
136+
elementV, SymMgr, currBldrCtx, Bldr,
137+
/*hasElements=*/true);
137138

138-
populateObjCForDestinationSet(dstLocation, svalBuilder, S, elem, elementV,
139-
SymMgr, currBldrCtx, Bldr,
140-
/*hasElements=*/false);
139+
populateObjCForDestinationSet(DstLocationSingleton, svalBuilder, S, elem,
140+
elementV, SymMgr, currBldrCtx, Bldr,
141+
/*hasElements=*/false);
141142

142-
// Finally, run any custom checkers.
143-
// FIXME: Eventually all pre- and post-checks should live in VisitStmt.
144-
getCheckerManager().runCheckersForPostStmt(Dst, Tmp, S, *this);
143+
// Finally, run any custom checkers.
144+
// FIXME: Eventually all pre- and post-checks should live in VisitStmt.
145+
getCheckerManager().runCheckersForPostStmt(Dst, Tmp, S, *this);
146+
}
145147
}
146148

147149
void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME,

0 commit comments

Comments
 (0)