[alpha.webkit.UnretainedLambdaCapturesChecker] Add the support for protectedSelf

[rniwa]

This PR adds the support for treating capturing of "self" as safe if the lambda simultaneously captures "protectedSelf", which is a RetainPtr of "self".

This PR also fixes a bug that the checker wasn't generating a warning when "self" is implicitly captured. Note when "self" is implicitly captured, we use the lambda's getBeginLoc as a fallback source location.

[llvmbot]

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

@llvm/pr-subscribers-clang

Author: Ryosuke Niwa (rniwa)

Changes

This PR adds the support for treating capturing of "self" as safe if the lambda simultaneously captures "protectedSelf", which is a RetainPtr of "self".

This PR also fixes a bug that the checker wasn't generating a warning when "self" is implicitly captured. Note when "self" is implicitly captured, we use the lambda's getBeginLoc as a fallback source location.

---

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

3 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp (+1-1)
• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp (+50-11)
• (modified) clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures.mm (+16)

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index b4d2353a03cd2..a161cf644b7d9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -147,7 +147,7 @@ bool isCtorOfCheckedPtr(const clang::FunctionDecl *F) {
 bool isCtorOfRetainPtr(const clang::FunctionDecl *F) {
   const std::string &FunctionName = safeGetName(F);
   return FunctionName == "RetainPtr" || FunctionName == "adoptNS" ||
-         FunctionName == "adoptCF";
+         FunctionName == "adoptCF" || FunctionName == "retainPtr";
 }
 
 bool isCtorOfSafePtr(const clang::FunctionDecl *F) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp
index 8496a75c1b84f..da403d591a2e2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp
@@ -36,6 +36,7 @@ class RawPtrRefLambdaCapturesChecker
       : Bug(this, description, "WebKit coding guidelines") {}
 
   virtual std::optional<bool> isUnsafePtr(QualType) const = 0;
+  virtual bool isPtrType(const std::string &) const = 0;
   virtual const char *ptrKind(QualType QT) const = 0;
 
   void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
@@ -68,6 +69,15 @@ class RawPtrRefLambdaCapturesChecker
         return DynamicRecursiveASTVisitor::TraverseCXXMethodDecl(CXXMD);
       }
 
+      bool TraverseObjCMethodDecl(ObjCMethodDecl *OCMD) override {
+        llvm::SaveAndRestore SavedDecl(ClsType);
+        if (OCMD && OCMD->isInstanceMethod()) {
+          if (auto *ImplParamDecl = OCMD->getSelfDecl())
+            ClsType = ImplParamDecl->getType();
+        }
+        return DynamicRecursiveASTVisitor::TraverseObjCMethodDecl(OCMD);
+      }
+
       bool VisitTypedefDecl(TypedefDecl *TD) override {
         if (Checker->RTC)
           Checker->RTC->visitTypedef(TD);
@@ -275,10 +285,10 @@ class RawPtrRefLambdaCapturesChecker
         auto *VD = dyn_cast<VarDecl>(ValueDecl);
         if (!VD)
           return false;
-        auto *Init = VD->getInit()->IgnoreParenCasts();
+        auto *Init = VD->getInit();
         if (!Init)
           return false;
-        const Expr *Arg = Init;
+        const Expr *Arg = Init->IgnoreParenCasts();
         do {
           if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Arg))
             Arg = BTE->getSubExpr()->IgnoreParenCasts();
@@ -287,7 +297,7 @@ class RawPtrRefLambdaCapturesChecker
             if (!Ctor)
               return false;
             auto clsName = safeGetName(Ctor->getParent());
-            if (isRefType(clsName) && CE->getNumArgs()) {
+            if (Checker->isPtrType(clsName) && CE->getNumArgs()) {
               Arg = CE->getArg(0)->IgnoreParenCasts();
               continue;
             }
@@ -307,6 +317,12 @@ class RawPtrRefLambdaCapturesChecker
               Arg = CE->getArg(0)->IgnoreParenCasts();
               continue;
             }
+            if (auto *Callee = CE->getDirectCallee()) {
+              if (isCtorOfSafePtr(Callee) && CE->getNumArgs() == 1) {
+                Arg = CE->getArg(0)->IgnoreParenCasts();
+                continue;
+              }
+            }
           }
           if (auto *OpCE = dyn_cast<CXXOperatorCallExpr>(Arg)) {
             auto OpCode = OpCE->getOperator();
@@ -315,7 +331,7 @@ class RawPtrRefLambdaCapturesChecker
               if (!Callee)
                 return false;
               auto clsName = safeGetName(Callee->getParent());
-              if (!isRefType(clsName) || !OpCE->getNumArgs())
+              if (!Checker->isPtrType(clsName) || !OpCE->getNumArgs())
                 return false;
               Arg = OpCE->getArg(0)->IgnoreParenCasts();
               continue;
@@ -330,8 +346,15 @@ class RawPtrRefLambdaCapturesChecker
           }
           break;
         } while (Arg);
-        if (auto *DRE = dyn_cast<DeclRefExpr>(Arg))
-          return ProtectedThisDecls.contains(DRE->getDecl());
+        if (auto *DRE = dyn_cast<DeclRefExpr>(Arg)) {
+          auto *Decl = DRE->getDecl();
+          if (auto *ImplicitParam = dyn_cast<ImplicitParamDecl>(Decl)) {
+            auto kind = ImplicitParam->getParameterKind();
+            return kind == ImplicitParamKind::ObjCSelf ||
+                   kind == ImplicitParamKind::CXXThis;
+          }
+          return ProtectedThisDecls.contains(Decl);
+        }
         return isa<CXXThisExpr>(Arg);
       }
     };
@@ -351,10 +374,17 @@ class RawPtrRefLambdaCapturesChecker
         ValueDecl *CapturedVar = C.getCapturedVar();
         if (ignoreParamVarDecl && isa<ParmVarDecl>(CapturedVar))
           continue;
+        if (auto *ImplicitParam = dyn_cast<ImplicitParamDecl>(CapturedVar)) {
+          auto kind = ImplicitParam->getParameterKind();
+          if ((kind == ImplicitParamKind::ObjCSelf ||
+               kind == ImplicitParamKind::CXXThis) &&
+              !shouldCheckThis)
+            continue;
+        }
         QualType CapturedVarQualType = CapturedVar->getType();
         auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType());
         if (IsUncountedPtr && *IsUncountedPtr)
-          reportBug(C, CapturedVar, CapturedVarQualType);
+          reportBug(C, CapturedVar, CapturedVarQualType, L);
       } else if (C.capturesThis() && shouldCheckThis) {
         if (ignoreParamVarDecl) // this is always a parameter to this function.
           continue;
@@ -364,11 +394,12 @@ class RawPtrRefLambdaCapturesChecker
   }
 
   void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
-                 const QualType T) const {
+                 const QualType T, LambdaExpr *L) const {
     assert(CapturedVar);
 
-    if (isa<ImplicitParamDecl>(CapturedVar) && !Capture.getLocation().isValid())
-      return; // Ignore implicit captruing of self.
+    auto Location = Capture.getLocation();
+    if (isa<ImplicitParamDecl>(CapturedVar) && !Location.isValid())
+      Location = L->getBeginLoc();
 
     SmallString<100> Buf;
     llvm::raw_svector_ostream Os(Buf);
@@ -387,7 +418,7 @@ class RawPtrRefLambdaCapturesChecker
     printQuotedQualifiedName(Os, CapturedVar);
     Os << " to " << ptrKind(T) << " type is unsafe.";
 
-    PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
+    PathDiagnosticLocation BSLoc(Location, BR->getSourceManager());
     auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
     BR->emitReport(std::move(Report));
   }
@@ -429,6 +460,10 @@ class UncountedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker {
     return result2;
   }
 
+  virtual bool isPtrType(const std::string &Name) const final {
+    return isRefType(Name) || isCheckedPtr(Name);
+  }
+
   const char *ptrKind(QualType QT) const final {
     if (isUncounted(QT))
       return "uncounted";
@@ -448,6 +483,10 @@ class UnretainedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker {
     return RTC->isUnretained(QT);
   }
 
+  virtual bool isPtrType(const std::string &Name) const final {
+    return isRetainPtr(Name);
+  }
+
   const char *ptrKind(QualType QT) const final { return "unretained"; }
 };
 
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures.mm b/clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures.mm
index 073eff9386baa..33be0eaaae914 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures.mm
@@ -279,17 +279,33 @@ @interface ObjWithSelf : NSObject {
   RetainPtr<id> delegate;
 }
 -(void)doWork;
+-(void)doMoreWork;
 -(void)run;
 @end
 
 @implementation ObjWithSelf
 -(void)doWork {
   auto doWork = [&] {
+    // expected-warning@-1{{Implicitly captured raw-pointer 'self' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
+    someFunction();
+    [delegate doWork];
+  };
+  auto doExtraWork = [&, protectedSelf = retainPtr(self)] {
     someFunction();
     [delegate doWork];
   };
   callFunctionOpaque(doWork);
+  callFunctionOpaque(doExtraWork);
 }
+
+-(void)doMoreWork {
+  auto doWork = [self, protectedSelf = retainPtr(self)] {
+    someFunction();
+    [self doWork];
+  };
+  callFunctionOpaque(doWork);
+}
+
 -(void)run {
   someFunction();
 }

[t-rasmud]

Also adda test for capture by value? auto doWork = [=]

[rniwa]

Sure, done.

[t-rasmud]

Are there any actual callers to this function?

[rniwa]

Yes, line 290 / 300 and line 318 / 334.

[t-rasmud]

LGTM!

[rniwa]

Thanks for the review!