[clang] Fix WebKit static analyzers to support ref and deref methods to be defined on different classes. (PR #69985)

Ryosuke Niwa via cfe-commits cfe-commits at lists.llvm.org
Mon Oct 23 16:38:50 PDT 2023


https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/69985

None

>From d8236358e137967dbbd63606b7d43983709597e8 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Mon, 23 Oct 2023 16:38:10 -0700
Subject: [PATCH] Fix WebKit static analyzers to support ref and deref methods
 to be defined on different classes.

---
 .../Checkers/WebKit/PtrTypesSemantics.cpp     | 61 ++++++++++---------
 .../Checkers/WebKit/PtrTypesSemantics.h       |  8 +--
 .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 47 ++++++++++++--
 ...virtual-dtor-ref-deref-on-diff-classes.cpp | 22 +++++++
 ...nted-members-ref-deref-on-diff-classes.cpp | 23 +++++++
 5 files changed, 125 insertions(+), 36 deletions(-)
 create mode 100644 clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-ref-deref-on-diff-classes.cpp
 create mode 100644 clang/test/Analysis/Checkers/WebKit/uncounted-members-ref-deref-on-diff-classes.cpp

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index c1f180f31338cb3..8c7eaa3b9a5369d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -18,24 +18,14 @@ using namespace clang;
 
 namespace {
 
-bool hasPublicRefAndDeref(const CXXRecordDecl *R) {
+bool hasPublicMethodInBaseClass(const CXXRecordDecl *R, const char* NameToMatch) {
   assert(R);
   assert(R->hasDefinition());
 
-  bool hasRef = false;
-  bool hasDeref = false;
   for (const CXXMethodDecl *MD : R->methods()) {
     const auto MethodName = safeGetName(MD);
-
-    if (MethodName == "ref" && MD->getAccess() == AS_public) {
-      if (hasDeref)
-        return true;
-      hasRef = true;
-    } else if (MethodName == "deref" && MD->getAccess() == AS_public) {
-      if (hasRef)
-        return true;
-      hasDeref = true;
-    }
+    if (MethodName == NameToMatch && MD->getAccess() == AS_public)
+      return true;
   }
   return false;
 }
@@ -44,9 +34,8 @@ bool hasPublicRefAndDeref(const CXXRecordDecl *R) {
 
 namespace clang {
 
-std::optional<const clang::CXXRecordDecl*>
-isRefCountable(const CXXBaseSpecifier* Base)
-{
+std::optional<const clang::CXXRecordDecl *>
+hasPublicMethodInBase(const CXXBaseSpecifier *Base, const char *NameToMatch) {
   assert(Base);
 
   const Type *T = Base->getType().getTypePtrOrNull();
@@ -59,7 +48,7 @@ isRefCountable(const CXXBaseSpecifier* Base)
   if (!R->hasDefinition())
     return std::nullopt;
 
-  return hasPublicRefAndDeref(R) ? R : nullptr;
+  return hasPublicMethodInBaseClass(R, NameToMatch) ? R : nullptr;
 }
 
 std::optional<bool> isRefCountable(const CXXRecordDecl* R)
@@ -70,29 +59,45 @@ std::optional<bool> isRefCountable(const CXXRecordDecl* R)
   if (!R)
     return std::nullopt;
 
-  if (hasPublicRefAndDeref(R))
+  bool hasRef = hasPublicMethodInBaseClass(R, "ref");
+  bool hasDeref = hasPublicMethodInBaseClass(R, "deref");
+  if (hasRef && hasDeref)
     return true;
 
   CXXBasePaths Paths;
   Paths.setOrigin(const_cast<CXXRecordDecl *>(R));
 
   bool AnyInconclusiveBase = false;
-  const auto isRefCountableBase =
-      [&AnyInconclusiveBase](const CXXBaseSpecifier* Base, CXXBasePath&) {
-          std::optional<const clang::CXXRecordDecl*> IsRefCountable = clang::isRefCountable(Base);
-          if (!IsRefCountable) {
-              AnyInconclusiveBase = true;
-              return false;
-          }
-          return (*IsRefCountable) != nullptr;
+  const auto hasPublicRefInBase =
+      [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
+        auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
+        if (!hasRefInBase) {
+          AnyInconclusiveBase = true;
+          return false;
+        }
+        return (*hasRefInBase) != nullptr;
       };
 
-  bool BasesResult = R->lookupInBases(isRefCountableBase, Paths,
+  hasRef = hasRef || R->lookupInBases(hasPublicRefInBase, Paths,
                                       /*LookupInDependent =*/true);
   if (AnyInconclusiveBase)
     return std::nullopt;
 
-  return BasesResult;
+  const auto hasPublicDerefInBase =
+      [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
+        auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
+        if (!hasDerefInBase) {
+          AnyInconclusiveBase = true;
+          return false;
+        }
+        return (*hasDerefInBase) != nullptr;
+      };
+  hasDeref = hasDeref || R->lookupInBases(hasPublicDerefInBase, Paths,
+                                          /*LookupInDependent =*/true);
+  if (AnyInconclusiveBase)
+    return std::nullopt;
+
+  return hasRef && hasDeref;
 }
 
 bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
index 91e3ccf2ec3047e..45b21cc0918443b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -26,10 +26,10 @@ class Type;
 // In WebKit there are two ref-counted templated smart pointers: RefPtr<T> and
 // Ref<T>.
 
-/// \returns CXXRecordDecl of the base if the type is ref-countable, nullptr if
-/// not, std::nullopt if inconclusive.
-std::optional<const clang::CXXRecordDecl*>
-isRefCountable(const clang::CXXBaseSpecifier* Base);
+/// \returns CXXRecordDecl of the base if the type has ref as a public method,
+/// nullptr if not, std::nullopt if inconclusive.
+std::optional<const clang::CXXRecordDecl *>
+hasPublicMethodInBase(const CXXBaseSpecifier *Base, const char *NameToMatch);
 
 /// \returns true if \p Class is ref-countable, false if not, std::nullopt if
 /// inconclusive.
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index 48dcfc4a3c4645d..bf54bb40c23c01c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -77,14 +77,53 @@ class RefCntblBaseVirtualDtorChecker
               (AccSpec == AS_none && RD->isClass()))
             return false;
 
-          std::optional<const CXXRecordDecl*> RefCntblBaseRD = isRefCountable(Base);
-          if (!RefCntblBaseRD || !(*RefCntblBaseRD))
+          auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
+          auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
+
+          bool hasRef = hasRefInBase && *hasRefInBase != nullptr;
+          bool hasDeref = hasDerefInBase && *hasDerefInBase != nullptr;
+
+          const Type *T = Base->getType().getTypePtrOrNull();
+          if (!T)
+            return false;
+
+          const CXXRecordDecl *C = T->getAsCXXRecordDecl();
+          if (!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 &) {
+                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,
+                                              /*LookupInDependent =*/true);
+          hasDeref = hasDeref || C->lookupInBases(hasPublicDerefInBase, Paths,
+                                                  /*LookupInDependent =*/true);
+          if (AnyInconclusiveBase || !hasRef || !hasDeref)
             return false;
 
-          const auto *Dtor = (*RefCntblBaseRD)->getDestructor();
+          const auto *Dtor = C->getDestructor();
           if (!Dtor || !Dtor->isVirtual()) {
             ProblematicBaseSpecifier = Base;
-            ProblematicBaseClass = *RefCntblBaseRD;
+            ProblematicBaseClass = C;
             return true;
           }
 
diff --git a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-ref-deref-on-diff-classes.cpp b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-ref-deref-on-diff-classes.cpp
new file mode 100644
index 000000000000000..aac58c0c1dda68e
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-ref-deref-on-diff-classes.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.RefCntblBaseVirtualDtor -verify %s
+
+struct RefCountedBase {
+  void ref() {}
+};
+
+template<typename T> struct RefCounted : RefCountedBase {
+public:
+  void deref() const { }
+};
+
+struct Base : RefCounted<Base> {
+// expected-warning at -1{{Struct 'RefCounted<Base>' is used as a base of struct 'Base' but doesn't have virtual destructor}}
+  virtual void foo() { }
+};
+
+struct Derived : Base { };
+// expected-warning at -1{{Struct 'Base' is used as a base of struct 'Derived' but doesn't have virtual destructor}}
+
+void foo () {
+  Derived d;
+}
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-members-ref-deref-on-diff-classes.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-members-ref-deref-on-diff-classes.cpp
new file mode 100644
index 000000000000000..4198b2388fed8f5
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-members-ref-deref-on-diff-classes.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.NoUncountedMemberChecker -verify %s
+
+#include "mock-types.h"
+
+class RefCountedBase {
+public:
+  void ref() const { }
+};
+
+template<typename T> class RefCounted : public RefCountedBase {
+public:
+  virtual ~RefCounted() { }
+  void deref() const { }
+};
+
+class TreeNode : public RefCounted<TreeNode> {
+public:
+  void setParent(TreeNode& parent) { m_parent = &parent; }
+
+private:
+  TreeNode* m_parent;
+// expected-warning at -1{{Member variable 'm_parent' in 'TreeNode' is a raw pointer to ref-countable type 'TreeNode'}}
+};



More information about the cfe-commits mailing list