[webkit.RefCntblBaseVirtualDtor] Ignore a base class which has a specialized deref function for a given derived class.

[rniwa]

When a base class B has a deref function which calls delete operator on a derived class D, don't emit a warning for B even if it did not have a virtual destructor.

[llvmbot]

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

@llvm/pr-subscribers-clang

Author: Ryosuke Niwa (rniwa)

Changes

When a base class B has a deref function which calls delete operator on a derived class D, don't emit a warning for B even if it did not have a virtual destructor.

---

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

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp (+138-2)
• (modified) clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp (+192-4)

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index 7f4c3a7b787e8..36ab581f1edbd 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -11,16 +11,128 @@
 #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 <optional>
 
 using namespace clang;
 using namespace ento;
 
 namespace {
+
+class DerefAnalysisVisitor
+    : public ConstStmtVisitor<DerefAnalysisVisitor, 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:
+  DerefAnalysisVisitor(const TemplateArgumentList &ArgList,
+                       const CXXRecordDecl *ClassDecl)
+    : ArgList(&ArgList)
+    , ClassDecl(ClassDecl)
+  { }
+
+  DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl)
+    : ClassDecl(ClassDecl)
+  { }
+
+  bool HasSpecializedDelete(CXXMethodDecl *Decl) {
+    if (auto *Tmpl = Decl->getTemplateInstantiationPattern())
+      return VisitBody(Tmpl->getBody());
+    return VisitBody(Decl->getBody());
+  }
+
+  bool VisitCallExpr(const CallExpr *CE) {
+    auto *Callee = CE->getCallee();
+    while (auto *Expr = dyn_cast<CastExpr>(Callee))
+      Callee = Expr->getSubExpr();
+    if (auto *DeclRef = dyn_cast<DeclRefExpr>(Callee)) {
+      auto *Decl = DeclRef->getDecl();
+      if (auto *VD = dyn_cast<VarDecl>(Decl)) {
+        if (auto *Init = VD->getInit()) {
+          if (auto *Lambda = dyn_cast<LambdaExpr>(Init))
+            return VisitBody(Lambda->getBody());
+        }
+      } else if (auto *FD = dyn_cast<FunctionDecl>(Decl))
+        return VisitBody(FD->getBody());
+    }
+    return false;
+  }
+
+  bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) {
+    auto *Callee = MCE->getMethodDecl();
+    if (!Callee)
+      return false;
+    return VisitBody(Callee->getBody());
+  }
+
+  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<ExplicitCastExpr>(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 (auto *RD = dyn_cast<RecordType>(Type)) {
+                if (RD->getDecl() == ClassDecl)
+                  return true;
+              }
+            }
+          } else if (auto *RD = dyn_cast<RecordType>(PointeeType)) {
+            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:
@@ -91,8 +203,6 @@ class RefCntblBaseVirtualDtorChecker
           const CXXRecordDecl *C = T->getAsCXXRecordDecl();
           if (!C)
             return false;
-          if (isRefCountedClass(C))
-            return false;
 
           bool AnyInconclusiveBase = false;
           const auto hasPublicRefInBase =
@@ -124,6 +234,9 @@ class RefCntblBaseVirtualDtorChecker
           if (AnyInconclusiveBase || !hasRef || !hasDeref)
             return false;
 
+          if (isClassWithSpecializedDelete(C, RD))
+            return false;
+
           const auto *Dtor = C->getDestructor();
           if (!Dtor || !Dtor->isVirtual()) {
             ProblematicBaseSpecifier = Base;
@@ -182,6 +295,29 @@ class RefCntblBaseVirtualDtorChecker
             ClsName == "ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr");
   }
 
+  static bool isClassWithSpecializedDelete(const CXXRecordDecl *C,
+                                           const CXXRecordDecl *DerivedClass) {
+    if (auto *ClsTmplSpDecl = dyn_cast<ClassTemplateSpecializationDecl>(C)) {
+      for (auto *MethodDecl : C->methods()) {
+        if (safeGetName(MethodDecl) == "deref") {
+          DerefAnalysisVisitor DerefAnalysis(ClsTmplSpDecl->getTemplateArgs(),
+                                             DerivedClass);
+          if (DerefAnalysis.HasSpecializedDelete(MethodDecl))
+            return true;
+        }
+      }
+      return false;
+    }
+    for (auto *MethodDecl : C->methods()) {
+        if (safeGetName(MethodDecl) == "deref") {
+          DerefAnalysisVisitor DerefAnalysis(DerivedClass);
+          if (DerefAnalysis.HasSpecializedDelete(MethodDecl))
+            return true;
+        }
+    }
+    return false;
+  }
+
   void reportBug(const CXXRecordDecl *DerivedClass,
                  const CXXBaseSpecifier *BaseSpec,
                  const CXXRecordDecl *ProblematicBaseClass) const {
diff --git a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
index eeb62d5d89ec4..37f5d46110896 100644
--- a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
@@ -58,24 +58,101 @@ class RefCounted : public RefCountedBase {
   RefCounted() { }
 };
 
+template <typename X, typename T>
+class ExoticRefCounted : RefCountedBase {
+public:
+  void deref() const {
+    if (derefBase())
+      delete (const_cast<T*>(static_cast<const T*>(this)));
+  }
+};
+
+template <typename X, typename T>
+class BadBase : RefCountedBase {
+public:
+  void deref() const {
+    if (derefBase())
+      delete (const_cast<X*>(static_cast<const X*>(this)));
+  }
+};
+
+template <typename T>
+class FancyDeref {
+public:
+  void ref() const
+  {
+    ++refCount;
+  }
+
+  void deref() const
+  {
+    --refCount;
+    if (refCount)
+      return;
+    auto deleteThis = [this] {
+      delete static_cast<const T*>(this);
+    };
+    deleteThis();
+  }
+private:
+  mutable unsigned refCount { 0 };
+};
+
+template <typename T>
+class BadFancyDeref {
+public:
+  void ref() const
+  {
+    ++refCount;
+  }
+
+  void deref() const
+  {
+    --refCount;
+    if (refCount)
+      return;
+    auto deleteThis = [this] {
+      delete static_cast<const T*>(this);
+    };
+    delete this;
+  }
+private:
+  mutable unsigned refCount { 0 };
+};
+
 template <typename T>
 class ThreadSafeRefCounted {
 public:
-  void ref() const;
-  bool deref() const;
+  void ref() const { ++refCount; }
+  void deref() const {
+    if (!--refCount)
+      delete const_cast<T*>(static_cast<const T*>(this));
+  }
+private:
+  mutable unsigned refCount { 0 };
 };
 
 template <typename T>
 class ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr {
 public:
-  void ref() const;
-  bool deref() const;
+  void ref() const { ++refCount; }
+  void deref() const {
+    if (!--refCount)
+      delete const_cast<T*>(static_cast<const T*>(this));
+  }
+private:
+  mutable unsigned refCount { 0 };
 };
 
 } // namespace WTF
 
 class DerivedClass4 : public WTF::RefCounted<DerivedClass4> { };
 
+class DerivedClass4b : public WTF::ExoticRefCounted<int, DerivedClass4b> { };
+
+class DerivedClass4c : public WTF::BadBase<int, DerivedClass4c> { };
+// expected-warning@-1{{Class 'WTF::BadBase<int, DerivedClass4c>' is used as a base of class 'DerivedClass4c' but doesn't have virtual destructor}}
+
 class DerivedClass5 : public DerivedClass4 { };
 // expected-warning@-1{{Class 'DerivedClass4' is used as a base of class 'DerivedClass5' but doesn't have virtual destructor}}
 
@@ -88,3 +165,114 @@ class DerivedClass8 : public WTF::ThreadSafeRefCountedAndCanMakeThreadSafeWeakPt
 
 class DerivedClass9 : public DerivedClass8 { };
 // expected-warning@-1{{Class 'DerivedClass8' is used as a base of class 'DerivedClass9' but doesn't have virtual destructor}}
+
+class DerivedClass10 : public WTF::FancyDeref<DerivedClass10> { };
+
+class DerivedClass10b : public WTF::BadFancyDeref<DerivedClass10b> { };
+// expected-warning@-1{{Class 'WTF::BadFancyDeref<DerivedClass10b>' is used as a base of class 'DerivedClass10b' but doesn't have virtual destructor}}
+
+class BaseClass1 {
+public:
+  void ref() const { ++refCount; }
+  void deref() const;
+private:
+  enum class Type { Base, Derived } type { Type::Base };
+  mutable unsigned refCount { 0 };
+};
+
+class DerivedClass11 : public BaseClass1 { };
+
+void BaseClass1::deref() const
+{
+  --refCount;
+  if (refCount)
+    return;
+  switch (type) {
+  case Type::Base:
+    delete const_cast<BaseClass1*>(this);
+    break;
+  case Type::Derived:
+    delete const_cast<DerivedClass11*>(static_cast<const DerivedClass11*>(this));
+    break;
+  }
+}
+
+class BaseClass2;
+static void deleteBase2(BaseClass2*);
+
+class BaseClass2 {
+public:
+  void ref() const { ++refCount; }
+  void deref() const
+  {
+    if (!--refCount)
+      deleteBase2(const_cast<BaseClass2*>(this));
+  }
+  virtual bool isDerived() { return false; }
+private:
+  mutable unsigned refCount { 0 };
+};
+
+class DerivedClass12 : public BaseClass2 {
+  bool isDerived() final { return true; }
+};
+
+void deleteBase2(BaseClass2* obj) {
+  if (obj->isDerived())
+    delete static_cast<DerivedClass12*>(obj);
+  else
+    delete obj;
+}
+
+class BaseClass3 {
+public:
+  void ref() const { ++refCount; }
+  void deref() const
+  {
+    if (!--refCount)
+      const_cast<BaseClass3*>(this)->destory();
+  }
+  virtual bool isDerived() { return false; }
+
+private:
+  void destory();
+
+  mutable unsigned refCount { 0 };
+};
+
+class DerivedClass13 : public BaseClass3 {
+  bool isDerived() final { return true; }
+};
+
+void BaseClass3::destory() {
+  if (isDerived())
+    delete static_cast<DerivedClass13*>(this);
+  else
+    delete this;
+}
+
+class RecursiveBaseClass {
+public:
+  void ref() const {
+    if (otherObject)
+      otherObject->ref();
+    else
+      ++refCount;
+  }
+  void deref() const {
+    if (otherObject)
+      otherObject->deref();
+    else {
+      --refCount;
+      if (refCount)
+        return;
+      delete this;
+    }
+  }
+private:
+  RecursiveBaseClass* otherObject { nullptr };
+  mutable unsigned refCount { 0 };
+};
+
+class RecursiveDerivedClass : public RecursiveBaseClass { };
+// expected-warning@-1{{Class 'RecursiveBaseClass' is used as a base of class 'RecursiveDerivedClass' but doesn't have virtual destructor}}

[github-actions]

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

[rniwa]

Closing this PR in favor of #92837, which has a single consolidated commit.