From 21dd8e6fed15e95aee6b4b512d64d796a36336ec Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa 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 IsUnsafe = isUnsafePtr(E->getReceiverType()); if (IsUnsafe && *IsUnsafe && !isPtrOriginSafe(Receiver)) { - if (auto *InnerMsg = dyn_cast(Receiver)) { + if (auto *InnerMsg = + dyn_cast(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(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(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(CFCopyArray(cf1))); @@ -28,6 +29,8 @@ void basic_wrong() { // expected-warning@-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@-1{{Incorrect use of adoptNS. The argument is +0 and results in an use-after-free [alpha.webkit.RetainPtrCtorAdoptChecker]}} + RetainPtr ns3 = [[SomeObj allocWithZone:nullptr] init]; + // expected-warning@-1{{Incorrect use of RetainPtr constructor. The argument is +1 and results in a memory leak [alpha.webkit.RetainPtrCtorAdoptChecker]}} RetainPtr cf1 = CFArrayCreateMutable(kCFAllocatorDefault, 10); // expected-warning@-1{{Incorrect use of RetainPtr constructor. The argument is +1 and results in a memory leak [alpha.webkit.RetainPtrCtorAdoptChecker]}} RetainPtr 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(provide()).autorelease(); } ++ (SomeObj *)sharedObj +{ + return adoptNS([[SomeObj alloc] init]).autorelease(); +} + - (void)doWorkOnSomeObj { [[self getSomeObj] doWork]; + // expected-warning@-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@-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 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(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@-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 {