[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 Sep 24 15:54:23 PDT 2025
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/160569
>From b43b17baebe66fe4714106061e56aac2ca8cae2a 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/3] [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.
---
.../StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp | 2 ++
.../WebKit/RawPtrRefCallArgsChecker.cpp | 12 ++++--------
.../WebKit/RawPtrRefLocalVarsChecker.cpp | 4 ----
.../Analysis/Checkers/WebKit/objc-mock-types.h | 2 ++
.../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, 35 insertions(+), 12 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index 00a1b8b6e7e89..013705ab36f60 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -190,6 +190,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 9585ceb40f95e..ce35cf330d3e4 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
@@ -180,10 +180,11 @@ class RawPtrRefCallArgsChecker
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);
@@ -474,11 +475,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);
- }
-
const char *ptrKind() const final { return "unretained"; }
};
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
index dd9701fbbb017..75e06c7351d53 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
@@ -439,10 +439,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);
- }
const char *ptrKind() const final { return "unretained"; }
};
diff --git a/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h
index 39dee1746158b..a32f9f8e5191e 100644
--- a/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h
@@ -100,6 +100,7 @@ id CFBridgingRelease(CFTypeRef X) {
__attribute__((objc_root_class))
@interface NSObject
+ (instancetype) alloc;
++ (instancetype) allocWithZone:(NSZone *)zone;
+ (Class) class;
+ (Class) superclass;
- (instancetype) init;
@@ -164,6 +165,7 @@ __attribute__((objc_root_class))
@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 769901778cdf0..62c0e5b5b4da9 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 c9d2fe861bb49..cdbfab1f50445 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm
@@ -439,6 +439,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];
}
}
@@ -565,6 +574,7 @@ @interface TestObject : NSObject
- (void)doWork:(NSString *)msg, ...;
- (void)doWorkOnSelf;
- (SomeObj *)getSomeObj;
++ (SomeObj *)sharedObj;
@end
@implementation TestObject
@@ -588,8 +598,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 307a4d03fe101..2a41a302252c3 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm
@@ -554,6 +554,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 13b0a3e958059fc0eed43290852418065dcb6668 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Wed, 24 Sep 2025 10:39:11 -0700
Subject: [PATCH 2/3] Fix formatting
---
.../Checkers/WebKit/RawPtrRefCallArgsChecker.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
index ce35cf330d3e4..eedc1465c7796 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
@@ -180,7 +180,8 @@ class RawPtrRefCallArgsChecker
if (auto *Receiver = E->getInstanceReceiver()) {
std::optional<bool> IsUnsafe = isUnsafePtr(E->getReceiverType());
if (IsUnsafe && *IsUnsafe && !isPtrOriginSafe(Receiver)) {
- if (auto *InnerMsg = dyn_cast<ObjCMessageExpr>(Receiver->IgnoreParenCasts())) {
+ if (auto *InnerMsg =
+ dyn_cast<ObjCMessageExpr>(Receiver->IgnoreParenCasts())) {
auto InnerSelector = InnerMsg->getSelector();
auto SelName = Selector.getNameForSlot(0);
if (InnerSelector.getNameForSlot(0).starts_with("alloc") &&
>From 596eb8e952d646d795ac13e94a33c17e1ce28def 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 3/3] Treat invocations of copy & isEqual selectors as safe.
---
.../Checkers/WebKit/RawPtrRefCallArgsChecker.cpp | 7 ++++++-
clang/test/Analysis/Checkers/WebKit/objc-mock-types.h | 3 +++
.../test/Analysis/Checkers/WebKit/unretained-call-args.mm | 6 ++++++
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
index eedc1465c7796..e34140ba7eee3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
@@ -177,13 +177,18 @@ 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->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 a32f9f8e5191e..e7d69fa6b811d 100644
--- a/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h
@@ -146,6 +146,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
@@ -168,6 +170,7 @@ __attribute__((objc_root_class))
+ (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/unretained-call-args.mm b/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm
index cdbfab1f50445..6751000219b8a 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);
@@ -592,6 +594,10 @@ - (void)doWorkOnSelf {
[self doWork:@"hello", RetainPtr<SomeObj> { provide() }.get(), RetainPtr<CFMutableArrayRef> { provide_cf() }.get(), OSObjectPtr { provide_dispatch() }.get()];
[self doWork:__null];
[self doWork:nil];
+ [provide() isEqual:provide()];
+ [provide_str() isEqualToString:@"foo"];
+ [provide_str() copyWithZone:nullptr];
+ [provide_str() mutableCopy];
}
- (SomeObj *)getSomeObj {
More information about the cfe-commits
mailing list