[webkit.UncountedLambdaCapturesChecker] Ignore trivial functions and [[clang::noescape]]. (llvm#114897)

This PR makes webkit.UncountedLambdaCapturesChecker ignore trivial
functions as well as the one being passed to an argument with
[[clang::noescape]] attribute. This dramatically reduces the false
positive rate for this checker.

To do this, this PR replaces VisitLambdaExpr in favor of checking
lambdas via VisitDeclRefExpr and VisitCallExpr. The idea is that if a
lambda is defined but never called or stored somewhere, then capturing
whatever variable in such a lambda is harmless.

VisitCallExpr explicitly looks for direct invocation of lambdas and
registers its DeclRefExpr to be ignored in VisitDeclRefExpr. If a lambda
is being passed to a function, it checks whether its argument is
annotated with [[clang::noescape]]. If it's not annotated such, it
checks captures for their safety.

Because WTF::switchOn could not be annotated with [[clang::noescape]] as
function type parameters are variadic template function so we hard-code
this function into the checker.

In order to check whether "this" pointer is ref-counted type or not, we
override TraverseDecl and record the most recent method's declaration.

In addition, this PR fixes a bug in isUnsafePtr that it was erroneously
checking whether std::nullopt was returned by isUncounted and
isUnchecked as opposed to the actual boolean value.

Finally, this PR also converts the accompanying test to use -verify and
adds a bunch of tests.

Author: rniwa

clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp

@@ -222,7 +222,13 @@ std::optional<bool> isUncheckedPtr(const QualType T) {
 std::optional<bool> isUnsafePtr(const QualType T) {
   if (T->isPointerType() || T->isReferenceType()) {
     if (auto *CXXRD = T->getPointeeCXXRecordDecl()) {
-      return isUncounted(CXXRD) || isUnchecked(CXXRD);
+      auto isUncountedPtr = isUncounted(CXXRD);
+      auto isUncheckedPtr = isUnchecked(CXXRD);
+      if (isUncountedPtr && isUncheckedPtr)
+        return *isUncountedPtr || *isUncheckedPtr;
+      if (isUncountedPtr)
+        return *isUncountedPtr;
+      return isUncheckedPtr;
     }
   }
   return false;

clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h

@@ -75,6 +75,10 @@ std::optional<bool> isUncountedPtr(const clang::QualType T);
 /// class, false if not, std::nullopt if inconclusive.
 std::optional<bool> isUncheckedPtr(const clang::QualType T);
 
+/// \returns true if \p T is either a raw pointer or reference to an uncounted
+/// or unchecked class, false if not, std::nullopt if inconclusive.
+std::optional<bool> isUnsafePtr(const QualType T);
+
 /// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or its
 /// variant, false if not.
 bool isSafePtrType(const clang::QualType T);

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp

@@ -6,6 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "ASTUtils.h"
 #include "DiagOutputUtils.h"
 #include "PtrTypesSemantics.h"
 #include "clang/AST/CXXInheritance.h"
@@ -26,6 +27,7 @@ class UncountedLambdaCapturesChecker
   BugType Bug{this, "Lambda capture of uncounted variable",
               "WebKit coding guidelines"};
   mutable BugReporter *BR = nullptr;
+  TrivialFunctionAnalysis TFA;
 
 public:
   void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
@@ -37,6 +39,11 @@ class UncountedLambdaCapturesChecker
     // want to visit those, so we make our own RecursiveASTVisitor.
     struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> {
       const UncountedLambdaCapturesChecker *Checker;
+      llvm::DenseSet<const DeclRefExpr *> DeclRefExprsToIgnore;
+      QualType ClsType;
+
+      using Base = RecursiveASTVisitor<LocalVisitor>;
+
       explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
           : Checker(Checker) {
         assert(Checker);
@@ -45,32 +52,118 @@ class UncountedLambdaCapturesChecker
       bool shouldVisitTemplateInstantiations() const { return true; }
       bool shouldVisitImplicitCode() const { return false; }
 
-      bool VisitLambdaExpr(LambdaExpr *L) {
-        Checker->visitLambdaExpr(L);
+      bool TraverseCXXMethodDecl(CXXMethodDecl *CXXMD) {
+        llvm::SaveAndRestore SavedDecl(ClsType);
+        ClsType = CXXMD->getThisType();
+        return Base::TraverseCXXMethodDecl(CXXMD);
+      }
+
+      bool shouldCheckThis() {
+        auto result = !ClsType.isNull() ? isUnsafePtr(ClsType) : std::nullopt;
+        return result && *result;
+      }
+
+      bool VisitDeclRefExpr(DeclRefExpr *DRE) {
+        if (DeclRefExprsToIgnore.contains(DRE))
+          return true;
+        auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl());
+        if (!VD)
+          return true;
+        auto *Init = VD->getInit();
+        if (!Init)
+          return true;
+        auto *L = dyn_cast_or_null<LambdaExpr>(Init->IgnoreParenCasts());
+        if (!L)
+          return true;
+        Checker->visitLambdaExpr(L, shouldCheckThis());
         return true;
       }
+
+      // WTF::switchOn(T, F... f) is a variadic template function and couldn't
+      // be annotated with NOESCAPE. We hard code it here to workaround that.
+      bool shouldTreatAllArgAsNoEscape(FunctionDecl *Decl) {
+        auto *NsDecl = Decl->getParent();
+        if (!NsDecl || !isa<NamespaceDecl>(NsDecl))
+          return false;
+        return safeGetName(NsDecl) == "WTF" && safeGetName(Decl) == "switchOn";
+      }
+
+      bool VisitCallExpr(CallExpr *CE) {
+        checkCalleeLambda(CE);
+        if (auto *Callee = CE->getDirectCallee()) {
+          bool TreatAllArgsAsNoEscape = shouldTreatAllArgAsNoEscape(Callee);
+          unsigned ArgIndex = 0;
+          for (auto *Param : Callee->parameters()) {
+            if (ArgIndex >= CE->getNumArgs())
+              return true;
+            auto *Arg = CE->getArg(ArgIndex)->IgnoreParenCasts();
+            if (!Param->hasAttr<NoEscapeAttr>() && !TreatAllArgsAsNoEscape) {
+              if (auto *L = dyn_cast_or_null<LambdaExpr>(Arg)) {
+                Checker->visitLambdaExpr(L, shouldCheckThis());
+              }
+            }
+            ++ArgIndex;
+          }
+        }
+        return true;
+      }
+
+      void checkCalleeLambda(CallExpr *CE) {
+        auto *Callee = CE->getCallee();
+        if (!Callee)
+          return;
+        auto *DRE = dyn_cast<DeclRefExpr>(Callee->IgnoreParenCasts());
+        if (!DRE)
+          return;
+        auto *MD = dyn_cast_or_null<CXXMethodDecl>(DRE->getDecl());
+        if (!MD || CE->getNumArgs() != 1)
+          return;
+        auto *Arg = CE->getArg(0)->IgnoreParenCasts();
+        auto *ArgRef = dyn_cast<DeclRefExpr>(Arg);
+        if (!ArgRef)
+          return;
+        auto *VD = dyn_cast_or_null<VarDecl>(ArgRef->getDecl());
+        if (!VD)
+          return;
+        auto *Init = VD->getInit();
+        if (!Init)
+          return;
+        auto *L = dyn_cast_or_null<LambdaExpr>(Init->IgnoreParenCasts());
+        if (!L)
+          return;
+        DeclRefExprsToIgnore.insert(ArgRef);
+        Checker->visitLambdaExpr(L, shouldCheckThis(),
+                                 /* ignoreParamVarDecl */ true);
+      }
     };
 
     LocalVisitor visitor(this);
     visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
   }
 
-  void visitLambdaExpr(LambdaExpr *L) const {
+  void visitLambdaExpr(LambdaExpr *L, bool shouldCheckThis,
+                       bool ignoreParamVarDecl = false) const {
+    if (TFA.isTrivial(L->getBody()))
+      return;
     for (const LambdaCapture &C : L->captures()) {
       if (C.capturesVariable()) {
         ValueDecl *CapturedVar = C.getCapturedVar();
+        if (ignoreParamVarDecl && isa<ParmVarDecl>(CapturedVar))
+          continue;
         QualType CapturedVarQualType = CapturedVar->getType();
-        if (auto *CapturedVarType = CapturedVarQualType.getTypePtrOrNull()) {
-          auto IsUncountedPtr = isUncountedPtr(CapturedVarQualType);
-          if (IsUncountedPtr && *IsUncountedPtr)
-            reportBug(C, CapturedVar, CapturedVarType);
-        }
+        auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType());
+        if (IsUncountedPtr && *IsUncountedPtr)
+          reportBug(C, CapturedVar, CapturedVarQualType);
+      } else if (C.capturesThis() && shouldCheckThis) {
+        if (ignoreParamVarDecl) // this is always a parameter to this function.
+          continue;
+        reportBugOnThisPtr(C);
       }
     }
   }
 
   void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
-                 const Type *T) const {
+                 const QualType T) const {
     assert(CapturedVar);
 
     SmallString<100> Buf;
@@ -89,7 +182,25 @@ class UncountedLambdaCapturesChecker
     }
 
     printQuotedQualifiedName(Os, Capture.getCapturedVar());
-    Os << " to uncounted type is unsafe.";
+    Os << " to ref-counted type or CheckedPtr-capable type is unsafe.";
+
+    PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
+    auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
+    BR->emitReport(std::move(Report));
+  }
+
+  void reportBugOnThisPtr(const LambdaCapture &Capture) const {
+    SmallString<100> Buf;
+    llvm::raw_svector_ostream Os(Buf);
+
+    if (Capture.isExplicit()) {
+      Os << "Captured ";
+    } else {
+      Os << "Implicitly captured ";
+    }
+
+    Os << "raw-pointer 'this' to ref-counted type or CheckedPtr-capable type "
+          "is unsafe.";
 
     PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
     auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);

clang/test/Analysis/Checkers/WebKit/mock-types.h

@@ -135,7 +135,9 @@ struct RefCountable {
   void ref() {}
   void deref() {}
   void method();
+  void constMethod() const;
   int trivial() { return 123; }
+  RefCountable* next();
 };
 
 template <typename T> T *downcast(T *t) { return t; }