[WebKit checkers] Treat the return value of an instance method as an unsafe pointer origin

[rniwa]

In bf1d278, 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.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Ryosuke Niwa (rniwa)

Changes

In bf1d278, 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.

---

Full diff: https://github.com/llvm/llvm-project/pull/160569.diff

7 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp (+2)
• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp (+4-8)
• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp (-4)
• (modified) clang/test/Analysis/Checkers/WebKit/objc-mock-types.h (+2)
• (modified) clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use.mm (+7)
• (modified) clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm (+17)
• (modified) clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm (+3)

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@-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<SomeObj> 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<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());
@@ -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@-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@-1{{Local variable 'obj' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}}
   [obj doWork];
+  auto *obj2 = [SomeObj sharedInstance];
+  [obj2 doWork];
 }
 @end

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[t-rasmud]

LGTM!