https://github.com/ziqingluo-90 updated
https://github.com/llvm/llvm-project/pull/124477
>From 1f67a396d917428ff35ca54ecb2d9124c14342de Mon Sep 17 00:00:00 2001
From: Ziqing Luo <ziqing_...@apple.com>
Date: Thu, 30 Jan 2025 12:24:45 -0800
Subject: [PATCH 1/2] [StaticAnalyzer] Add reproducers for a bug in
VisitObjCForCollectionStmt
The bug (see #124477) in VisitObjCForCollectionStmt seems have been
there for a long time and can be very rarely triggered. Now adding
tests that reproduce it.
The integrated test is reduced from a crash observed downstream that
reproduces the bug.
The unit test is a more straightforward way of showing how was the bug
triggered.
(rdar://143280254)
---
clang/test/Analysis/bugfix-124477.m | 39 ++++++++++++
clang/unittests/StaticAnalyzer/CMakeLists.txt | 1 +
.../StaticAnalyzer/ObjcBug-124477.cpp | 63 +++++++++++++++++++
3 files changed, 103 insertions(+)
create mode 100644 clang/test/Analysis/bugfix-124477.m
create mode 100644 clang/unittests/StaticAnalyzer/ObjcBug-124477.cpp
diff --git a/clang/test/Analysis/bugfix-124477.m
b/clang/test/Analysis/bugfix-124477.m
new file mode 100644
index 000000000000000..80820f4c934443d
--- /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
+@class NSArray, NSObject;
+
+@interface Base
+@property (readonly, copy) NSArray *array;
+@end
+
+#pragma clang assume_nonnull end
+# 8 "this-file.m" 2
+
+
+@interface Test : Base
+
+@property (readwrite, copy, nullable) NSObject *label;
+@property (readwrite, strong, nullable) Test * field;
+
+- (void)f;
+
+@end
+
+@implementation Test
+- (void)f
+{
+ NSObject * X;
+
+ for (NSObject *ele in self.field.array) {}
+ self.label = X;
+}
+@end
+
+
diff --git a/clang/unittests/StaticAnalyzer/CMakeLists.txt
b/clang/unittests/StaticAnalyzer/CMakeLists.txt
index f5da86e5456030f..3b01a4e9e532766 100644
--- a/clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ b/clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -15,6 +15,7 @@ add_clang_unittest(StaticAnalysisTests
IsCLibraryFunctionTest.cpp
MemRegionDescriptiveNameTest.cpp
NoStateChangeFuncVisitorTest.cpp
+ ObjcBug-124477.cpp
ParamRegionTest.cpp
RangeSetTest.cpp
RegisterCustomCheckersTest.cpp
diff --git a/clang/unittests/StaticAnalyzer/ObjcBug-124477.cpp
b/clang/unittests/StaticAnalyzer/ObjcBug-124477.cpp
new file mode 100644
index 000000000000000..51bd33210032c70
--- /dev/null
+++ b/clang/unittests/StaticAnalyzer/ObjcBug-124477.cpp
@@ -0,0 +1,63 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "CheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace ento;
+
+// Some dummy trait that we can mutate back and forth to force a new State.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(Flag, bool)
+
+namespace {
+class FlipFlagOnCheckLocation : public Checker<check::Location> {
+public:
+ // We make sure we alter the State every time we model a checkLocation event.
+ void checkLocation(SVal l, bool isLoad, const Stmt *S,
+ CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+ State = State->set<Flag>(!State->get<Flag>());
+ C.addTransition(State);
+ }
+};
+
+void addFlagFlipperChecker(AnalysisASTConsumer &AnalysisConsumer,
+ AnalyzerOptions &AnOpts) {
+ AnOpts.CheckersAndPackages = {{"test.FlipFlagOnCheckLocation", true}};
+ AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+
Registry.addChecker<FlipFlagOnCheckLocation>("test.FlipFlagOnCheckLocation",
+ "Description", "");
+ });
+}
+
+TEST(ObjCTest, CheckLocationEventsShouldMaterializeInObjCForCollectionStmts) {
+ // Previously, the `ExprEngine::hasMoreIteration` may fired an assertion
+ // because we forgot to handle correctly the resulting nodes of the
+ // check::Location callback for the ObjCForCollectionStmts.
+ // This caused inconsistencies in the graph and triggering the assertion.
+ // See #124477 for more details.
+ std::string Diags;
+ EXPECT_TRUE(runCheckerOnCodeWithArgs<addFlagFlipperChecker>(
+ R"(
+ @class NSArray, NSDictionary, NSString;
+ extern void NSLog(NSString *format, ...)
__attribute__((format(__NSString__, 1, 2)));
+ void entrypoint(NSArray *bowl) {
+ for (NSString *fruit in bowl) { // no-crash
+ NSLog(@"Fruit: %@", fruit);
+ }
+ })",
+ {"-x", "objective-c"}, Diags));
+}
+
+} // namespace
>From 730082e31af715196f4e0b1358489c665aa3a207 Mon Sep 17 00:00:00 2001
From: Ziqing Luo <ziqing_...@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 f075df3ab5e4d60..9426e0afd65a046 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,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
