[webkit.UncountedLambdaCapturesChecker] Recognize nested protectedThis pattern (llvm#126443)

rniwa · rniwa · commit 3926bc271159 · 2025-02-14T17:33:27.000-08:00
In WebKit, it's pretty common to capture "this" and "protectedThis"
where "protectedThis" is a guardian variable of type Ref or RefPtr for
"this". Furthermore, it's common for this "protectedThis" variable from
being passed to an inner lambda by std::move. Recognize this pattern so
that we don't emit warnings for nested inner lambdas.

To recognize this pattern, we introduce a new DenseSet,
ProtectedThisDecls, which contains every "protectedThis" we've
recognized to our subclass of DynamicRecursiveASTVisitor. This set is
now populated in "hasProtectedThis" and "declProtectsThis" uses this
DenseSet to determine a given value declaration constitutes a
"protectedThis" pattern or not.

Because hasProtectedThis and declProtectsThis had to be moved from the
checker class to the visitor class, it's now a responsibility of each
caller of visitLambdaExpr to check whether a given lambda captures
"this" without a "protectedThis" or not.

Finally, this PR improves the code to recognize "protectedThis" pattern
by allowing more nested CXXBindTemporaryExpr, CXXOperatorCallExpr, and
UnaryOperator expressions.
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
@@ -41,7 +41,9 @@ class UncountedLambdaCapturesChecker
       const UncountedLambdaCapturesChecker *Checker;
       llvm::DenseSet<const DeclRefExpr *> DeclRefExprsToIgnore;
       llvm::DenseSet<const LambdaExpr *> LambdasToIgnore;
+      llvm::DenseSet<const ValueDecl *> ProtectedThisDecls;
       llvm::DenseSet<const CXXConstructExpr *> ConstructToIgnore;
+
       QualType ClsType;
 
       using Base = RecursiveASTVisitor<LocalVisitor>;
@@ -69,7 +71,7 @@ class UncountedLambdaCapturesChecker
       bool VisitLambdaExpr(LambdaExpr *L) {
         if (LambdasToIgnore.contains(L))
           return true;
-        Checker->visitLambdaExpr(L, shouldCheckThis());
+        Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L));
         return true;
       }
 
@@ -97,7 +99,7 @@ class UncountedLambdaCapturesChecker
         if (!L)
           return true;
         LambdasToIgnore.insert(L);
-        Checker->visitLambdaExpr(L, shouldCheckThis());
+        Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L));
         return true;
       }
 
@@ -122,7 +124,8 @@ class UncountedLambdaCapturesChecker
             if (auto *L = findLambdaInArg(Arg)) {
               LambdasToIgnore.insert(L);
               if (!Param->hasAttr<NoEscapeAttr>())
-                Checker->visitLambdaExpr(L, shouldCheckThis());
+                Checker->visitLambdaExpr(L, shouldCheckThis() &&
+                                                !hasProtectedThis(L));
             }
             ++ArgIndex;
           }
@@ -142,7 +145,8 @@ class UncountedLambdaCapturesChecker
             if (auto *L = findLambdaInArg(Arg)) {
               LambdasToIgnore.insert(L);
               if (!Param->hasAttr<NoEscapeAttr>() && !TreatAllArgsAsNoEscape)
-                Checker->visitLambdaExpr(L, shouldCheckThis());
+                Checker->visitLambdaExpr(L, shouldCheckThis() &&
+                                                !hasProtectedThis(L));
             }
             ++ArgIndex;
           }
@@ -171,6 +175,13 @@ class UncountedLambdaCapturesChecker
           ConstructToIgnore.insert(CE);
           return Lambda;
         }
+        if (auto *TempExpr = dyn_cast<CXXBindTemporaryExpr>(CtorArg)) {
+          E = TempExpr->getSubExpr()->IgnoreParenCasts();
+          if (auto *Lambda = dyn_cast<LambdaExpr>(E)) {
+            ConstructToIgnore.insert(CE);
+            return Lambda;
+          }
+        }
         auto *DRE = dyn_cast<DeclRefExpr>(CtorArg);
         if (!DRE)
           return nullptr;
@@ -216,9 +227,68 @@ class UncountedLambdaCapturesChecker
           return;
         DeclRefExprsToIgnore.insert(ArgRef);
         LambdasToIgnore.insert(L);
-        Checker->visitLambdaExpr(L, shouldCheckThis(),
+        Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L),
                                  /* ignoreParamVarDecl */ true);
       }
+
+      bool hasProtectedThis(LambdaExpr *L) {
+        for (const LambdaCapture &OtherCapture : L->captures()) {
+          if (!OtherCapture.capturesVariable())
+            continue;
+          if (auto *ValueDecl = OtherCapture.getCapturedVar()) {
+            if (declProtectsThis(ValueDecl)) {
+              ProtectedThisDecls.insert(ValueDecl);
+              return true;
+            }
+          }
+        }
+        return false;
+      }
+
+      bool declProtectsThis(const ValueDecl *ValueDecl) const {
+        auto *VD = dyn_cast<VarDecl>(ValueDecl);
+        if (!VD)
+          return false;
+        auto *Init = VD->getInit()->IgnoreParenCasts();
+        if (!Init)
+          return false;
+        const Expr *Arg = Init;
+        do {
+          if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Arg))
+            Arg = BTE->getSubExpr()->IgnoreParenCasts();
+          if (auto *CE = dyn_cast_or_null<CXXConstructExpr>(Arg)) {
+            auto *Ctor = CE->getConstructor();
+            if (!Ctor)
+              return false;
+            auto clsName = safeGetName(Ctor->getParent());
+            if (!isRefType(clsName) || !CE->getNumArgs())
+              return false;
+            Arg = CE->getArg(0)->IgnoreParenCasts();
+            continue;
+          }
+          if (auto *OpCE = dyn_cast<CXXOperatorCallExpr>(Arg)) {
+            auto OpCode = OpCE->getOperator();
+            if (OpCode == OO_Star || OpCode == OO_Amp) {
+              auto *Callee = OpCE->getDirectCallee();
+              auto clsName = safeGetName(Callee->getParent());
+              if (!isRefType(clsName) || !OpCE->getNumArgs())
+                return false;
+              Arg = OpCE->getArg(0)->IgnoreParenCasts();
+              continue;
+            }
+          }
+          if (auto *UO = dyn_cast<UnaryOperator>(Arg)) {
+            auto OpCode = UO->getOpcode();
+            if (OpCode == UO_Deref || OpCode == UO_AddrOf)
+              Arg = UO->getSubExpr()->IgnoreParenCasts();
+            continue;
+          }
+          break;
+        } while (Arg);
+        if (auto *DRE = dyn_cast<DeclRefExpr>(Arg))
+          return ProtectedThisDecls.contains(DRE->getDecl());
+        return isa<CXXThisExpr>(Arg);
+      }
     };
 
     LocalVisitor visitor(this);
@@ -241,53 +311,11 @@ class UncountedLambdaCapturesChecker
       } else if (C.capturesThis() && shouldCheckThis) {
         if (ignoreParamVarDecl) // this is always a parameter to this function.
           continue;
-        bool hasProtectThis = false;
-        for (const LambdaCapture &OtherCapture : L->captures()) {
-          if (!OtherCapture.capturesVariable())
-            continue;
-          if (auto *ValueDecl = OtherCapture.getCapturedVar()) {
-            if (protectThis(ValueDecl)) {
-              hasProtectThis = true;
-              break;
-            }
-          }
-        }
-        if (!hasProtectThis)
-          reportBugOnThisPtr(C);
+        reportBugOnThisPtr(C);
       }
     }
   }
 
-  bool protectThis(const ValueDecl *ValueDecl) const {
-    auto *VD = dyn_cast<VarDecl>(ValueDecl);
-    if (!VD)
-      return false;
-    auto *Init = VD->getInit()->IgnoreParenCasts();
-    if (!Init)
-      return false;
-    auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Init);
-    if (!BTE)
-      return false;
-    auto *CE = dyn_cast_or_null<CXXConstructExpr>(BTE->getSubExpr());
-    if (!CE)
-      return false;
-    auto *Ctor = CE->getConstructor();
-    if (!Ctor)
-      return false;
-    auto clsName = safeGetName(Ctor->getParent());
-    if (!isRefType(clsName) || !CE->getNumArgs())
-      return false;
-    auto *Arg = CE->getArg(0)->IgnoreParenCasts();
-    while (auto *UO = dyn_cast<UnaryOperator>(Arg)) {
-      auto OpCode = UO->getOpcode();
-      if (OpCode == UO_Deref || OpCode == UO_AddrOf)
-        Arg = UO->getSubExpr();
-      else
-        break;
-    }
-    return isa<CXXThisExpr>(Arg);
-  }
-
   void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
                  const QualType T) const {
     assert(CapturedVar);
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
@@ -89,6 +89,7 @@ template <typename Callback> void call(Callback callback) {
   someFunction();
   callback();
 }
+void callAsync(const WTF::Function<void()>&);
 
 void raw_ptr() {
   RefCountable* ref_countable = make_obj();
@@ -303,6 +304,37 @@ struct RefCountableWithLambdaCapturingThis {
     });
   }
 
+  void callAsyncNoescape([[clang::noescape]] WTF::Function<bool(RefCountable&)>&&);
+  void method_temp_lambda(RefCountable* obj) {
+    callAsyncNoescape([this, otherObj = RefPtr { obj }](auto& obj) {
+      return otherObj == &obj;
+    });
+  }
+
+  void method_nested_lambda() {
+    callAsync([this, protectedThis = Ref { *this }] {
+      callAsync([this, protectedThis = static_cast<const Ref<RefCountableWithLambdaCapturingThis>&&>(protectedThis)] {
+        nonTrivial();
+      });
+    });
+  }
+
+  void method_nested_lambda2() {
+    callAsync([this, protectedThis = RefPtr { this }] {
+      callAsync([this, protectedThis = static_cast<const Ref<RefCountableWithLambdaCapturingThis>&&>(*protectedThis)] {
+        nonTrivial();
+      });
+    });
+  }
+
+  void method_nested_lambda3() {
+    callAsync([this, protectedThis = RefPtr { this }] {
+      callAsync([this] {
+        // expected-warning@-1{{Captured raw-pointer 'this' to ref-counted type or CheckedPtr-capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+        nonTrivial();
+      });
+    });
+  }
 };
 
 struct NonRefCountableWithLambdaCapturingThis {
