[clang] [WebKit checkers] Treat the return value of an instance method as an unsafe pointer origin (PR #160569)
Ryosuke Niwa via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 15 12:27:59 PDT 2025
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/160569
>From 21dd8e6fed15e95aee6b4b512d64d796a36336ec Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Wed, 24 Sep 2025 00:41:55 -0700
Subject: [PATCH 1/2] [WebKit checkers] Treat the return value of an instance
method as an unsafe pointer origin
In bf1d27889b583, we started treating the return value of any selector invocation as safe.
This isn't quite right since not every return value is autorelease'd. So start treating
these as unsafe again for messages sent to an instance but keep treating safe for messages
sent to a class since those should always be autorelease'd.
This PR also moves the check from isSafeExpr in local variable and call arguments checkers
to tryToFindPtrOrigin so that this semantic change could be more easily implemented. This
PR also fixes RawPtrRefCallArgsChecker so that it recognizes alloc-init pattern even if
allocWithZone or _init instead of alloc and init was used.
---
.../Checkers/WebKit/ASTUtils.cpp | 2 ++
.../WebKit/RawPtrRefCallArgsChecker.cpp | 19 +++++++++++--------
.../WebKit/RawPtrRefLocalVarsChecker.cpp | 4 ----
.../Checkers/WebKit/objc-mock-types.h | 1 +
.../WebKit/retain-ptr-ctor-adopt-use.mm | 7 +++++++
.../Checkers/WebKit/unretained-call-args.mm | 17 +++++++++++++++++
.../Checkers/WebKit/unretained-local-vars.mm | 3 +++
7 files changed, 41 insertions(+), 12 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index c1a5000f8b647..73956711c4068 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -196,6 +196,8 @@ bool tryToFindPtrOrigin(
if (isSafePtrType(Method->getReturnType()))
return callback(E, true);
}
+ if (ObjCMsgExpr->isClassMessage())
+ return callback(E, true);
auto Selector = ObjCMsgExpr->getSelector();
auto NameForFirstSlot = Selector.getNameForSlot(0);
if ((NameForFirstSlot == "class" || NameForFirstSlot == "superclass") &&
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
index 791e70998477f..26af0c3b724ec 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
@@ -178,13 +178,21 @@ class RawPtrRefCallArgsChecker
return;
auto Selector = E->getSelector();
+ auto SelName = Selector.getNameForSlot(0);
+ bool IsSafeSel = SelName.starts_with("copy") || SelName.contains("Copy") ||
+ SelName == "isEqual" || SelName == "isEqualToString";
+ if (Selector.getNumArgs() <= 1 && IsSafeSel)
+ return; // These selectors are assumed to be readonly.
+
if (auto *Receiver = E->getInstanceReceiver()) {
std::optional<bool> IsUnsafe = isUnsafePtr(E->getReceiverType());
if (IsUnsafe && *IsUnsafe && !isPtrOriginSafe(Receiver)) {
- if (auto *InnerMsg = dyn_cast<ObjCMessageExpr>(Receiver)) {
+ if (auto *InnerMsg =
+ dyn_cast<ObjCMessageExpr>(Receiver->IgnoreParenCasts())) {
auto InnerSelector = InnerMsg->getSelector();
- if (InnerSelector.getNameForSlot(0) == "alloc" &&
- Selector.getNameForSlot(0).starts_with("init"))
+ auto SelName = Selector.getNameForSlot(0);
+ if (InnerSelector.getNameForSlot(0).starts_with("alloc") &&
+ (SelName.starts_with("init") || SelName.starts_with("_init")))
return;
}
reportBugOnReceiver(Receiver, D);
@@ -476,11 +484,6 @@ class UnretainedCallArgsChecker final : public RawPtrRefCallArgsChecker {
return isRetainPtrOrOSPtrType(type);
}
- bool isSafeExpr(const Expr *E) const final {
- return ento::cocoa::isCocoaObjectRef(E->getType()) &&
- isa<ObjCMessageExpr>(E);
- }
-
bool isSafeDecl(const Decl *D) const final {
// Treat NS/CF globals in system header as immortal.
return BR->getSourceManager().isInSystemHeader(D->getLocation());
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
index c13df47920f72..c950dc91088b9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
@@ -441,10 +441,6 @@ class UnretainedLocalVarsChecker final : public RawPtrRefLocalVarsChecker {
bool isSafePtrType(const QualType type) const final {
return isRetainPtrOrOSPtrType(type);
}
- bool isSafeExpr(const Expr *E) const final {
- return ento::cocoa::isCocoaObjectRef(E->getType()) &&
- isa<ObjCMessageExpr>(E);
- }
bool isSafeDecl(const Decl *D) const final {
// Treat NS/CF globals in system header as immortal.
return BR->getSourceManager().isInSystemHeader(D->getLocation());
diff --git a/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h
index a5fc3d7f9a932..53e3ddedd9e45 100644
--- a/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h
@@ -195,6 +195,7 @@ extern NSApplication * NSApp;
@end
@interface SomeObj : NSObject
++ (SomeObj *)sharedInstance;
- (instancetype)_init;
- (SomeObj *)mutableCopy;
- (SomeObj *)copyWithValue:(int)value;
diff --git a/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use.mm b/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use.mm
index 45705615f3196..a58f0cd805bb9 100644
--- a/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use.mm
+++ b/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use.mm
@@ -15,6 +15,7 @@ void basic_correct() {
auto ns6 = retainPtr([ns3 next]);
auto ns7 = retainPtr((SomeObj *)0);
auto ns8 = adoptNS(nil);
+ auto ns9 = adoptNS([[SomeObj allocWithZone:nullptr] _init]);
CFMutableArrayRef cf1 = adoptCF(CFArrayCreateMutable(kCFAllocatorDefault, 10));
auto cf2 = adoptCF(SecTaskCreateFromSelf(kCFAllocatorDefault));
auto cf3 = adoptCF(checked_cf_cast<CFArrayRef>(CFCopyArray(cf1)));
@@ -28,6 +29,8 @@ void basic_wrong() {
// expected-warning at -1{{Incorrect use of RetainPtr constructor. The argument is +1 and results in a memory leak [alpha.webkit.RetainPtrCtorAdoptChecker]}}
auto ns2 = adoptNS([ns1.get() next]);
// expected-warning at -1{{Incorrect use of adoptNS. The argument is +0 and results in an use-after-free [alpha.webkit.RetainPtrCtorAdoptChecker]}}
+ RetainPtr<SomeObj> ns3 = [[SomeObj allocWithZone:nullptr] init];
+ // expected-warning at -1{{Incorrect use of RetainPtr constructor. The argument is +1 and results in a memory leak [alpha.webkit.RetainPtrCtorAdoptChecker]}}
RetainPtr<CFMutableArrayRef> cf1 = CFArrayCreateMutable(kCFAllocatorDefault, 10);
// expected-warning at -1{{Incorrect use of RetainPtr constructor. The argument is +1 and results in a memory leak [alpha.webkit.RetainPtrCtorAdoptChecker]}}
RetainPtr<CFMutableArrayRef> cf2 = adoptCF(provide_cf());
@@ -50,6 +53,10 @@ @implementation SomeObj {
SomeObj *_other;
}
++ (SomeObj *)sharedInstance {
+ return nil;
+}
+
- (instancetype)_init {
self = [super init];
_number = nil;
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm b/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm
index 5dc3b38ccb61c..855658f931082 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm
@@ -445,6 +445,15 @@ void foo() {
void foo() {
auto obj = adoptNS([[SomeObj alloc] init]);
[obj doWork];
+ auto obj2 = adoptNS([[SomeObj alloc] _init]);
+ [obj2 doWork];
+ }
+
+ void bar(NSZone *zone) {
+ auto obj = adoptNS([[SomeObj allocWithZone:zone] init]);
+ [obj doWork];
+ auto obj2 = adoptNS([(SomeObj *)[SomeObj allocWithZone:zone] _init]);
+ [obj2 doWork];
}
}
@@ -582,6 +591,7 @@ @interface TestObject : NSObject
- (void)doWork:(NSString *)msg, ...;
- (void)doWorkOnSelf;
- (SomeObj *)getSomeObj;
++ (SomeObj *)sharedObj;
@end
@implementation TestObject
@@ -606,8 +616,15 @@ - (SomeObj *)getSomeObj {
return RetainPtr<SomeObj *>(provide()).autorelease();
}
++ (SomeObj *)sharedObj
+{
+ return adoptNS([[SomeObj alloc] init]).autorelease();
+}
+
- (void)doWorkOnSomeObj {
[[self getSomeObj] doWork];
+ // expected-warning at -1{{Receiver is unretained and unsafe}}
+ [[TestObject sharedObj] doWork];
}
- (CGImageRef)createImage {
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm b/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm
index f49e7bdb3e79c..2b4a388f344a2 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm
@@ -592,6 +592,9 @@ - (SomeObj*)getSomeObj {
- (void)storeSomeObj {
auto *obj = [self getSomeObj];
+ // expected-warning at -1{{Local variable 'obj' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}}
[obj doWork];
+ auto *obj2 = [SomeObj sharedInstance];
+ [obj2 doWork];
}
@end
>From e31e59beb9116cb7a35b6490e63f4e61f33ebc27 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Wed, 24 Sep 2025 15:54:07 -0700
Subject: [PATCH 2/2] Treat invocations of copy & isEqual selectors as safe.
---
.../Checkers/WebKit/RawPtrRefCallArgsChecker.cpp | 1 -
clang/test/Analysis/Checkers/WebKit/objc-mock-types.h | 3 +++
.../Checkers/WebKit/retain-ptr-ctor-adopt-use-arc.mm | 8 ++++++++
.../Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use.mm | 4 ++++
.../test/Analysis/Checkers/WebKit/unretained-call-args.mm | 6 ++++++
5 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
index 26af0c3b724ec..5be1742913776 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
@@ -190,7 +190,6 @@ class RawPtrRefCallArgsChecker
if (auto *InnerMsg =
dyn_cast<ObjCMessageExpr>(Receiver->IgnoreParenCasts())) {
auto InnerSelector = InnerMsg->getSelector();
- auto SelName = Selector.getNameForSlot(0);
if (InnerSelector.getNameForSlot(0).starts_with("alloc") &&
(SelName.starts_with("init") || SelName.starts_with("_init")))
return;
diff --git a/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h
index 53e3ddedd9e45..1fcfe6e06f721 100644
--- a/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h
@@ -160,6 +160,8 @@ __attribute__((objc_root_class))
- ( const char *)UTF8String;
- (id)initWithUTF8String:(const char *)nullTerminatedCString;
- (NSString *)copy;
+- (NSString *)mutableCopy;
+- (BOOL)isEqualToString:(NSString *)aString;
+ (id)stringWithUTF8String:(const char *)nullTerminatedCString;
@end
@@ -198,6 +200,7 @@ extern NSApplication * NSApp;
+ (SomeObj *)sharedInstance;
- (instancetype)_init;
- (SomeObj *)mutableCopy;
+- (BOOL)isEqual:(SomeObj *)other;
- (SomeObj *)copyWithValue:(int)value;
- (void)doWork;
- (SomeObj *)other;
diff --git a/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use-arc.mm b/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use-arc.mm
index 47203cbd27355..18100168fd6eb 100644
--- a/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use-arc.mm
+++ b/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use-arc.mm
@@ -45,6 +45,10 @@ @implementation SomeObj {
SomeObj *_other;
}
++ (SomeObj *)sharedInstance {
+ return nil;
+}
+
- (instancetype)_init {
self = [super init];
_number = nil;
@@ -61,6 +65,10 @@ - (SomeObj *)mutableCopy {
return copy;
}
+- (BOOL)isEqual:(SomeObj *)other {
+ return self.value == other.value && self.next == other.next && _other == other.other;
+}
+
- (SomeObj *)copyWithValue:(int)value {
auto *copy = [[SomeObj alloc] init];
[copy setValue:_number];
diff --git a/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use.mm b/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use.mm
index a58f0cd805bb9..06a308f4e4cbc 100644
--- a/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use.mm
+++ b/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use.mm
@@ -74,6 +74,10 @@ - (SomeObj *)mutableCopy {
return copy;
}
+- (BOOL)isEqual:(SomeObj *)other {
+ return self.value == other.value && self.next == other.next && _other == other.other;
+}
+
- (SomeObj *)copyWithValue:(int)value {
auto *copy = [[SomeObj alloc] init];
// expected-warning at -1{{The return value is +1 and results in a memory leak [alpha.webkit.RetainPtrCtorAdoptChecker]}}
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm b/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm
index 855658f931082..d6c21e3c0c9b8 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm
@@ -6,6 +6,8 @@
SomeObj *provide();
void consume_obj(SomeObj*);
+NSString *provide_str();
+
CFMutableArrayRef provide_cf();
void consume_cf(CFMutableArrayRef);
@@ -610,6 +612,10 @@ - (void)doWorkOnSelf {
[self doWork:__null];
[self doWork:nil];
[NSApp run];
+ [provide() isEqual:provide()];
+ [provide_str() isEqualToString:@"foo"];
+ [provide_str() copyWithZone:nullptr];
+ [provide_str() mutableCopy];
}
- (SomeObj *)getSomeObj {
More information about the cfe-commits
mailing list