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

rniwa · web-flow · commit 09273d408da9 · 2023-11-10T12:21:36.000-08:00
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -18,24 +18,15 @@ 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 +35,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 +49,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 +60,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
@@ -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
@@ -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;
+
+          QualType T = Base->getType();
+          if (T.isNull())
+            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
@@ -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@-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@-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
@@ -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@-1{{Member variable 'm_parent' in 'TreeNode' is a raw pointer to ref-countable type 'TreeNode'}}
+};
