https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/160569
>From 9065efe18a85cb7f33359b9f3c0f8dd3819fc495 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa <[email protected]> Date: Wed, 24 Sep 2025 00:41:55 -0700 Subject: [PATCH] [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. Also treat invocations of copy & isEqual selectors as safe. --- .../Checkers/WebKit/ASTUtils.cpp | 2 ++ .../WebKit/RawPtrRefCallArgsChecker.cpp | 8 +++---- .../WebKit/RawPtrRefLocalVarsChecker.cpp | 4 ---- .../Checkers/WebKit/objc-mock-types.h | 4 ++++ .../WebKit/retain-ptr-ctor-adopt-use-arc.mm | 8 +++++++ .../WebKit/retain-ptr-ctor-adopt-use.mm | 11 +++++++++ .../Checkers/WebKit/unretained-call-args.mm | 23 +++++++++++++++++++ .../Checkers/WebKit/unretained-local-vars.mm | 3 +++ 8 files changed, 54 insertions(+), 9 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp index 7b4d231620314..0094c06476d77 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp @@ -211,6 +211,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 8ea058a32524c..7fee003f6f4d0 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp @@ -185,6 +185,9 @@ class RawPtrRefCallArgsChecker if (IsUnsafe && *IsUnsafe && !isPtrOriginSafe(Receiver)) { if (isAllocInit(E)) return; + auto SelectorName = E->getSelector().getNameForSlot(0); + if (SelectorName == "isEqual" || SelectorName == "isEqualToString") + return; reportBugOnReceiver(Receiver, D); } } @@ -515,11 +518,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 97404298a956c..a0bb75ff879d2 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp @@ -501,10 +501,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 ff6c133940ca9..ee9ac937eb0bf 100644 --- a/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h +++ b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h @@ -174,6 +174,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 @@ -209,8 +211,10 @@ extern NSApplication * NSApp; @end @interface SomeObj : NSObject ++ (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 135fe651a9f16..4f27bb6e41557 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 @@ -51,6 +51,10 @@ @implementation SomeObj { SomeObj *_other; } ++ (SomeObj *)sharedInstance { + return nil; +} + - (instancetype)_init { self = [super init]; _number = nil; @@ -67,6 +71,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 c337752d6bd21..d84f0c12a57d2 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 @@ -16,6 +16,7 @@ void basic_correct() { auto ns7 = retainPtr((SomeObj *)0); auto ns8 = adoptNS(nil); auto ns9 = adoptNSNullable([[SomeObj alloc] init]); + auto ns10 = 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))); @@ -32,6 +33,8 @@ void basic_wrong() { // expected-warning@-1{{Incorrect use of adoptNS. The argument is +0 and results in an use-after-free [alpha.webkit.RetainPtrCtorAdoptChecker]}} auto ns3 = adoptNSNullable([ns1.get() next]); // expected-warning@-1{{Incorrect use of adoptNSNullable. The argument is +0 and results in an use-after-free [alpha.webkit.RetainPtrCtorAdoptChecker]}} + RetainPtr<SomeObj> ns4 = [[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<CFMutableArrayRef> 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<CFMutableArrayRef> cf2 = adoptCF(provide_cf()); @@ -56,6 +59,10 @@ @implementation SomeObj { SomeObj *_other; } ++ (SomeObj *)sharedInstance { + return nil; +} + - (instancetype)_init { self = [super init]; _number = nil; @@ -73,6 +80,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 add9144744db4..6bc1c7c089ac6 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); @@ -447,6 +449,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]; } } @@ -633,6 +644,7 @@ @interface TestObject : NSObject - (void)doWork:(NSString *)msg, ...; - (void)doWorkOnSelf; - (SomeObj *)getSomeObj; ++ (SomeObj *)sharedObj; @end @implementation TestObject @@ -652,14 +664,25 @@ - (void)doWorkOnSelf { [self doWork:nil]; [NSApp run]; adoptNS([allocObj() init]); + [provide() isEqual:provide()]; + [provide_str() isEqualToString:@"foo"]; + [provide_str() copyWithZone:nullptr]; + [provide_str() mutableCopy]; } - (SomeObj *)getSomeObj { return RetainPtr<SomeObj *>(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 04f403eda206f..260e9ce7db83a 100644 --- a/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm +++ b/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm @@ -592,7 +592,10 @@ - (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]; } - (void)assignToGuardianArg:(RetainPtr<SomeObj>&)obj { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
