[webkit.RefCntblBaseVirtualDtor] Allow CRTP classes without a virtual destructor.

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

@@ -11,16 +11,116 @@
 #include "PtrTypesSemantics.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/StmtVisitor.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/SetVector.h"
 #include <optional>
 
 using namespace clang;
 using namespace ento;
 
 namespace {
+
+class DerefFuncDeleteExprVisitor
+    : public ConstStmtVisitor<DerefFuncDeleteExprVisitor, bool> {
+  // Returns true if any of child statements return true.
+  bool VisitChildren(const Stmt *S) {
+    for (const Stmt *Child : S->children()) {
+      if (Child && Visit(Child))
+        return true;
+    }
+    return false;
+  }
+
+  bool VisitBody(const Stmt *Body) {
+    if (!Body)
+      return false;
+
+    auto [It, IsNew] = VisitedBody.insert(Body);
+    if (!IsNew) // This body is recursive
+      return false;
+
+    return Visit(Body);
+  }
+
+public:
+  DerefFuncDeleteExprVisitor(const TemplateArgumentList &ArgList,
+                             const CXXRecordDecl *ClassDecl)
+      : ArgList(&ArgList), ClassDecl(ClassDecl) {}
+
+  DerefFuncDeleteExprVisitor(const CXXRecordDecl *ClassDecl)
+      : ClassDecl(ClassDecl) {}
+
+  std::optional<bool> HasSpecializedDelete(CXXMethodDecl *Decl) {
+    if (auto *Body = Decl->getBody())
+      return VisitBody(Body);
+    if (auto *Tmpl = Decl->getTemplateInstantiationPattern())
+      return std::nullopt; // Indeterminate. There was no concrete instance.
+    return false;
+  }
+
+  bool VisitCallExpr(const CallExpr *CE) {
+    const Decl *D = CE->getCalleeDecl();
+    if (D && D->hasBody())
+      return VisitBody(D->getBody());
+    return false;
+  }
+
+  bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) {
+    auto *Arg = E->getArgument();
+    while (Arg) {
+      if (auto *Paren = dyn_cast<ParenExpr>(Arg))
+        Arg = Paren->getSubExpr();
+      else if (auto *Cast = dyn_cast<CastExpr>(Arg)) {
+        Arg = Cast->getSubExpr();
+        auto CastType = Cast->getType();
+        if (auto *PtrType = dyn_cast<PointerType>(CastType)) {
+          auto PointeeType = PtrType->getPointeeType();
+          while (auto *ET = dyn_cast<ElaboratedType>(PointeeType)) {
+            if (ET->isSugared())
+              PointeeType = ET->desugar();
+          }
+          if (auto *ParmType = dyn_cast<TemplateTypeParmType>(PointeeType)) {
+            if (ArgList) {
+              auto ParmIndex = ParmType->getIndex();
+              auto Type = ArgList->get(ParmIndex).getAsType();
+              if (Type->getAsCXXRecordDecl() == ClassDecl)
+                return true;
+            }
+          } else if (auto *RD = dyn_cast<RecordType>(PointeeType)) {
+            if (RD->getDecl() == ClassDecl)
+              return true;
+          } else if (auto *ST =
+                         dyn_cast<SubstTemplateTypeParmType>(PointeeType)) {
+            auto Type = ST->getReplacementType();
+            if (auto *RD = dyn_cast<RecordType>(Type)) {
+              if (RD->getDecl() == ClassDecl)
+                return true;
+            }
+          }
+        }
+      } else
+        break;
+    }
+    return false;
+  }
+
+  bool VisitStmt(const Stmt *S) { return VisitChildren(S); }
+
+  // Return false since the contents of lambda isn't necessarily executed.
+  // If it is executed, VisitCallExpr above will visit its body.
+  bool VisitLambdaExpr(const LambdaExpr *) { return false; }
+
+private:
+  const TemplateArgumentList *ArgList{nullptr};
+  const CXXRecordDecl *ClassDecl;
+  llvm::DenseSet<const Stmt *> VisitedBody;
+};
+
 class RefCntblBaseVirtualDtorChecker
     : public Checker<check::ASTDecl<TranslationUnitDecl>> {
 private:
@@ -51,92 +151,136 @@ class RefCntblBaseVirtualDtorChecker
       bool shouldVisitImplicitCode() const { return false; }
 
       bool VisitCXXRecordDecl(const CXXRecordDecl *RD) {
-        Checker->visitCXXRecordDecl(RD);
+        if (!RD->hasDefinition())
+          return true;
+
+        Decls.insert(RD);
+
+        for (auto &Base : RD->bases()) {
+          const auto AccSpec = Base.getAccessSpecifier();
+          if (AccSpec == AS_protected || AccSpec == AS_private ||
+              (AccSpec == AS_none && RD->isClass()))
+            continue;
+
+          QualType T = Base.getType();
+          if (T.isNull())
+            continue;
+
+          const CXXRecordDecl *C = T->getAsCXXRecordDecl();
+          if (!C)
+            continue;
+
+          if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(C)) {
+            for (auto &Arg : CTSD->getTemplateArgs().asArray()) {
+              if (Arg.getKind() != TemplateArgument::Type)
+                continue;
+              auto TemplT = Arg.getAsType();
+              if (TemplT.isNull())
+                continue;
+
+              bool IsCRTP = TemplT->getAsCXXRecordDecl() == RD;
+              if (!IsCRTP)
+                continue;
+              CRTPs.insert(C);
+            }
+          }
+        }
+
         return true;
       }
+
+      llvm::SetVector<const CXXRecordDecl *> Decls;
+      llvm::DenseSet<const CXXRecordDecl *> CRTPs;
     };
 
     LocalVisitor visitor(this);
     visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
+    for (auto *RD : visitor.Decls) {
+      if (visitor.CRTPs.contains(RD))
+        continue;
+      visitCXXRecordDecl(RD);
+    }
   }
 
   void visitCXXRecordDecl(const CXXRecordDecl *RD) const {
     if (shouldSkipDecl(RD))
       return;
 
-    CXXBasePaths Paths;
-    Paths.setOrigin(RD);
+    for (auto &Base : RD->bases()) {
+      const auto AccSpec = Base.getAccessSpecifier();
+      if (AccSpec == AS_protected || AccSpec == AS_private ||
+          (AccSpec == AS_none && RD->isClass()))
+        continue;
 
-    const CXXBaseSpecifier *ProblematicBaseSpecifier = nullptr;
-    const CXXRecordDecl *ProblematicBaseClass = nullptr;
+      auto hasRefInBase = clang::hasPublicMethodInBase(&Base, "ref");
+      auto hasDerefInBase = clang::hasPublicMethodInBase(&Base, "deref");
 
-    const auto IsPublicBaseRefCntblWOVirtualDtor =
-        [RD, &ProblematicBaseSpecifier,
-         &ProblematicBaseClass](const CXXBaseSpecifier *Base, CXXBasePath &) {
-          const auto AccSpec = Base->getAccessSpecifier();
-          if (AccSpec == AS_protected || AccSpec == AS_private ||
-              (AccSpec == AS_none && RD->isClass()))
-            return false;
+      bool hasRef = hasRefInBase && *hasRefInBase != nullptr;
+      bool hasDeref = hasDerefInBase && *hasDerefInBase != nullptr;
 
-          auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
-          auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
+      QualType T = Base.getType();
+      if (T.isNull())
+        continue;
 
-          bool hasRef = hasRefInBase && *hasRefInBase != nullptr;
-          bool hasDeref = hasDerefInBase && *hasDerefInBase != nullptr;
+      const CXXRecordDecl *C = T->getAsCXXRecordDecl();
+      if (!C)
+        continue;
 
-          QualType T = Base->getType();
-          if (T.isNull())
-            return false;
-
-          const CXXRecordDecl *C = T->getAsCXXRecordDecl();
-          if (!C)
-            return false;
-          if (isRefCountedClass(C))
-            return false;
-
-          bool AnyInconclusiveBase = false;
-          const auto hasPublicRefInBase =
-              [&AnyInconclusiveBase](const CXXBaseSpecifier *Base,
-                                     CXXBasePath &) {
-                auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
-                if (!hasRefInBase) {
-                  AnyInconclusiveBase = true;
-                  return false;
-                }
-                return (*hasRefInBase) != nullptr;
-              };
-          const auto hasPublicDerefInBase = [&AnyInconclusiveBase](
-                                                const CXXBaseSpecifier *Base,
-                                                CXXBasePath &) {
+      bool AnyInconclusiveBase = false;
+      const auto hasPublicRefInBase =
+          [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
+            auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
+            if (!hasRefInBase) {
+              AnyInconclusiveBase = true;
+              return false;
+            }
+            return (*hasRefInBase) != nullptr;
+          };
+      const auto hasPublicDerefInBase =
+          [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
             auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
             if (!hasDerefInBase) {
               AnyInconclusiveBase = true;
               return false;
             }
             return (*hasDerefInBase) != nullptr;
           };
-          CXXBasePaths Paths;
-          Paths.setOrigin(C);
-          hasRef = hasRef || C->lookupInBases(hasPublicRefInBase, Paths,
+      CXXBasePaths Paths;
+      Paths.setOrigin(C);
+      hasRef = hasRef || C->lookupInBases(hasPublicRefInBase, Paths,
+                                          /*LookupInDependent =*/true);
+      hasDeref = hasDeref || C->lookupInBases(hasPublicDerefInBase, Paths,
                                               /*LookupInDependent =*/true);
-          hasDeref = hasDeref || C->lookupInBases(hasPublicDerefInBase, Paths,
-                                                  /*LookupInDependent =*/true);
-          if (AnyInconclusiveBase || !hasRef || !hasDeref)
-            return false;
-
-          const auto *Dtor = C->getDestructor();
-          if (!Dtor || !Dtor->isVirtual()) {
-            ProblematicBaseSpecifier = Base;
-            ProblematicBaseClass = C;
-            return true;
-          }
-
-          return false;
-        };
-
-    if (RD->lookupInBases(IsPublicBaseRefCntblWOVirtualDtor, Paths,
-                          /*LookupInDependent =*/true)) {
-      reportBug(RD, ProblematicBaseSpecifier, ProblematicBaseClass);
+      if (AnyInconclusiveBase || !hasRef || !hasDeref)
+        continue;
+
+      auto HasSpecializedDelete = isClassWithSpecializedDelete(C, RD);
+      if (!HasSpecializedDelete || *HasSpecializedDelete)
+        continue;
+      if (C->lookupInBases(
+              [&](const CXXBaseSpecifier *Base, CXXBasePath &) {
+                auto *T = Base->getType().getTypePtrOrNull();
+                if (!T)
+                  return false;
+                auto *R = T->getAsCXXRecordDecl();
+                if (!R)
+                  return false;
+                auto Result = isClassWithSpecializedDelete(R, RD);
+                if (!Result)
+                  AnyInconclusiveBase = true;
+                return Result && *Result;
+              },
+              Paths, /*LookupInDependent =*/true))
+        continue;
+      if (AnyInconclusiveBase)
+        continue;
+
+      const auto *Dtor = C->getDestructor();
+      if (!Dtor || !Dtor->isVirtual()) {
+        auto *ProblematicBaseSpecifier = &Base;
+        auto *ProblematicBaseClass = C;
+        reportBug(RD, ProblematicBaseSpecifier, ProblematicBaseClass);
+      }
     }
   }
 
@@ -182,6 +326,32 @@ class RefCntblBaseVirtualDtorChecker
             ClsName == "ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr");
   }
 
+  static std::optional<bool>
+  isClassWithSpecializedDelete(const CXXRecordDecl *C,
+                               const CXXRecordDecl *DerivedClass) {
+    if (auto *ClsTmplSpDecl = dyn_cast<ClassTemplateSpecializationDecl>(C)) {
+      for (auto *MethodDecl : C->methods()) {
+        if (safeGetName(MethodDecl) == "deref") {
+          DerefFuncDeleteExprVisitor Visitor(ClsTmplSpDecl->getTemplateArgs(),
+                                             DerivedClass);
+          auto Result = Visitor.HasSpecializedDelete(MethodDecl);
+          if (!Result || *Result)
+            return Result;
+        }
+      }
+      return false;
+    }
+    for (auto *MethodDecl : C->methods()) {
+      if (safeGetName(MethodDecl) == "deref") {
+        DerefFuncDeleteExprVisitor Visitor(DerivedClass);
+        auto Result = Visitor.HasSpecializedDelete(MethodDecl);
+        if (!Result || *Result)
+          return Result;
+      }
+    }
+    return false;
+  }
+
   void reportBug(const CXXRecordDecl *DerivedClass,
                  const CXXBaseSpecifier *BaseSpec,
                  const CXXRecordDecl *ProblematicBaseClass) const {

clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-ref-deref-on-diff-classes.cpp

@@ -19,4 +19,5 @@ struct Derived : Base { };
 
 void foo () {
   Derived d;
+  d.deref();
 }