Skip to content

[analyzer] WebKit checkers: support ref and deref defined on different classes. #68170

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[analyzer] WebKit checkers: support ref and deref defined on different classes. #68170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 62 additions & 28 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,26 @@ using namespace clang;

namespace {

bool hasPublicRefAndDeref(const CXXRecordDecl *R) {
bool hasPublicRefMethod(const CXXRecordDecl *R) {
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)
return true;
}
return false;
}

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;
}
bool hasPublicDerefMethod(const CXXRecordDecl *R) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably a good idea to deduplicate some code here, eg. have a function hasPublicMethod[InBase]WithName(string) and pass "Ref" and "Deref" as parameters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done that.

assert(R);
assert(R->hasDefinition());

for (const CXXMethodDecl *MD : R->methods()) {
const auto MethodName = safeGetName(MD);
if (MethodName == "deref" && MD->getAccess() == AS_public)
return true;
}
return false;
}
Expand All @@ -44,9 +46,25 @@ bool hasPublicRefAndDeref(const CXXRecordDecl *R) {

namespace clang {

std::optional<const clang::CXXRecordDecl*>
isRefCountable(const CXXBaseSpecifier* Base)
{
std::optional<const clang::CXXRecordDecl *>
hasPublicRefInBase(const CXXBaseSpecifier *Base) {
assert(Base);

const Type *T = Base->getType().getTypePtrOrNull();
if (!T)
return std::nullopt;
Comment on lines +54 to +55
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these nullopt cases are so frustrating to spell out! It looks like the only reason we have them is that we wanted to analyze not-fully-instantiated templates. (If they're ever instantiated, we'd still reanalyze each instantiation. So it only fights false negatives on templates that literally nobody instantiates.) In particular, this alternative approach doesn't seem to cause any test failures:

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index a66fa38315f4..042682487710 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -46,47 +46,31 @@ bool hasPublicDerefMethod(const CXXRecordDecl *R) {
 
 namespace clang {
 
-std::optional<const clang::CXXRecordDecl *>
+const CXXRecordDecl *
 hasPublicRefInBase(const CXXBaseSpecifier *Base) {
   assert(Base);
 
-  const Type *T = Base->getType().getTypePtrOrNull();
-  if (!T)
-    return std::nullopt;
-
+  QualType T = Base->getType();
   const CXXRecordDecl *R = T->getAsCXXRecordDecl();
-  if (!R)
-    return std::nullopt;
-  if (!R->hasDefinition())
-    return std::nullopt;
-
+  assert(R && R->hasDefinition());
   return hasPublicRefMethod(R) ? R : nullptr;
 }
 
-std::optional<const clang::CXXRecordDecl *>
+const CXXRecordDecl *
 hasPublicDerefInBase(const CXXBaseSpecifier *Base) {
   assert(Base);
 
-  const Type *T = Base->getType().getTypePtrOrNull();
-  if (!T)
-    return std::nullopt;
-
+  QualType T = Base->getType();
   const CXXRecordDecl *R = T->getAsCXXRecordDecl();
-  if (!R)
-    return std::nullopt;
-  if (!R->hasDefinition())
-    return std::nullopt;
-
+  assert(R && R->hasDefinition());
   return hasPublicDerefMethod(R) ? R : nullptr;
 }
 
 std::optional<bool> isRefCountable(const CXXRecordDecl* R)
 {
   assert(R);
-
   R = R->getDefinition();
-  if (!R)
-    return std::nullopt;
+  assert(R);
 
   bool hasRef = hasPublicRefMethod(R);
   bool hasDeref = hasPublicDerefMethod(R);
@@ -96,37 +80,19 @@ std::optional<bool> isRefCountable(const CXXRecordDecl* R)
   CXXBasePaths Paths;
   Paths.setOrigin(const_cast<CXXRecordDecl *>(R));
 
-  bool AnyInconclusiveBase = false;
-  const auto hasPublicRefInBase =
-      [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
-        auto hasRefInBase = clang::hasPublicRefInBase(Base);
-        if (!hasRefInBase) {
-          AnyInconclusiveBase = true;
-          return false;
-        }
-        return (*hasRefInBase) != nullptr;
-      };
-
-  hasRef =
-      R->lookupInBases(hasPublicRefInBase, Paths, /*LookupInDependent =*/true);
-  if (AnyInconclusiveBase)
-    return std::nullopt;
-
-  const auto hasPublicDerefInBase =
-      [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
-        auto hasDerefInBase = clang::hasPublicDerefInBase(Base);
-        if (!hasDerefInBase) {
-          AnyInconclusiveBase = true;
-          return false;
-        }
-        return (*hasDerefInBase) != nullptr;
-      };
-  hasDeref = R->lookupInBases(hasPublicDerefInBase, Paths,
-                              /*LookupInDependent =*/true);
-  if (AnyInconclusiveBase)
-    return std::nullopt;
-
-  return hasRef && hasDeref;
+  bool hasDeepRef = R->lookupInBases(
+      [](const CXXBaseSpecifier *B, CXXBasePath &) {
+        return (bool)hasPublicRefInBase(B);
+      },
+      Paths,
+      /*LookupInDependent =*/true);
+  bool hasDeepDeref = R->lookupInBases(
+      [](const CXXBaseSpecifier *B, CXXBasePath &) {
+        return (bool)hasPublicDerefInBase(B);
+      },
+      Paths,
+      /*LookupInDependent =*/true);
+  return (hasRef || hasDeepRef) && (hasDeref || hasDeepDeref);
 }
 
 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 9c7a933b9ee9..ec80870d58e9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -27,13 +27,13 @@ class Type;
 // Ref<T>.
 
 /// \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 *>
+/// nullptr if not.
+const clang::CXXRecordDecl *
 hasPublicRefInBase(const CXXBaseSpecifier *Base);
 
 /// \returns CXXRecordDecl of the base if the type has deref as a public
-/// method, nullptr if not, std::nullopt if inconclusive.
-std::optional<const clang::CXXRecordDecl *>
+/// method, nullptr if not.
+const clang::CXXRecordDecl *
 hasPublicDerefInBase(const CXXBaseSpecifier *Base);
 
 /// \returns true if \p Class is ref-countable, false if not, std::nullopt if
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index 6b8b952c8274..9421208adc4a 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -77,53 +77,35 @@ public:
               (AccSpec == AS_none && RD->isClass()))
             return false;
 
-          auto hasRefInBase = clang::hasPublicRefInBase(Base);
-          auto hasDerefInBase = clang::hasPublicDerefInBase(Base);
+          bool hasRef = hasPublicRefInBase(Base);
+          bool hasDeref = hasPublicDerefInBase(Base);
 
-          bool hasRef = hasRefInBase && *hasRefInBase != nullptr;
-          bool hasDeref = hasDerefInBase && *hasDerefInBase != nullptr;
+          QualType T = Base->getType();
+          const CXXRecordDecl *R = T->getAsCXXRecordDecl();
 
-          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::hasPublicRefInBase(Base);
-                if (!hasRefInBase) {
-                  AnyInconclusiveBase = true;
-                  return false;
-                }
-                return (*hasRefInBase) != nullptr;
-              };
-          const auto hasPublicDerefInBase =
-              [&AnyInconclusiveBase](const CXXBaseSpecifier *Base,
-                                     CXXBasePath &) {
-                auto hasDerefInBase = clang::hasPublicDerefInBase(Base);
-                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)
+          Paths.setOrigin(R);
+          bool hasDeepRef = R->lookupInBases(
+              [](const CXXBaseSpecifier *B, CXXBasePath &) {
+                return (bool)hasPublicRefInBase(B);
+              },
+              Paths,
+              /*LookupInDependent =*/true);
+          bool hasDeepDeref = R->lookupInBases(
+              [](const CXXBaseSpecifier *B, CXXBasePath &) {
+                return (bool)hasPublicDerefInBase(B);
+              },
+              Paths,
+              /*LookupInDependent =*/true);
+          hasRef = hasRef || hasDeepRef;
+          hasDeref = hasDeref || hasDeepDeref;
+          if (!hasRef || !hasDeref)
             return false;
 
-          const auto *Dtor = C->getDestructor();
+          const auto *Dtor = R->getDestructor();
           if (!Dtor || !Dtor->isVirtual()) {
             ProblematicBaseSpecifier = Base;
-            ProblematicBaseClass = C;
+            ProblematicBaseClass = R;
             return true;
           }
 
@@ -137,6 +119,16 @@ public:
   }
 
   bool shouldSkipDecl(const CXXRecordDecl *RD) const {
+    // Do not visit templates. We'll visit instantiations anyway
+    // and they're much easier to analyze.
+    if (const ClassTemplateDecl *TD = RD->getDescribedClassTemplate()) {
+      if (TD->getTemplatedDecl() == RD)
+        return true;
+
+      if (RD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
+        return true;
+    }
+
     if (!RD->isThisDeclarationADefinition())
       return true;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... this seems like a much more involved refactoring so perhaps we should do it in a separate pass? WDYT?


const CXXRecordDecl *R = T->getAsCXXRecordDecl();
if (!R)
return std::nullopt;
if (!R->hasDefinition())
return std::nullopt;

return hasPublicRefMethod(R) ? R : nullptr;
}

std::optional<const clang::CXXRecordDecl *>
hasPublicDerefInBase(const CXXBaseSpecifier *Base) {
assert(Base);

const Type *T = Base->getType().getTypePtrOrNull();
Expand All @@ -59,7 +77,7 @@ isRefCountable(const CXXBaseSpecifier* Base)
if (!R->hasDefinition())
return std::nullopt;

return hasPublicRefAndDeref(R) ? R : nullptr;
return hasPublicDerefMethod(R) ? R : nullptr;
}

std::optional<bool> isRefCountable(const CXXRecordDecl* R)
Expand All @@ -70,29 +88,45 @@ std::optional<bool> isRefCountable(const CXXRecordDecl* R)
if (!R)
return std::nullopt;

if (hasPublicRefAndDeref(R))
bool hasRef = hasPublicRefMethod(R);
bool hasDeref = hasPublicDerefMethod(R);
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::hasPublicRefInBase(Base);
if (!hasRefInBase) {
AnyInconclusiveBase = true;
return false;
}
return (*hasRefInBase) != nullptr;
};

bool BasesResult = R->lookupInBases(isRefCountableBase, Paths,
/*LookupInDependent =*/true);
hasRef =
R->lookupInBases(hasPublicRefInBase, Paths, /*LookupInDependent =*/true);
if (AnyInconclusiveBase)
return std::nullopt;

const auto hasPublicDerefInBase =
[&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
auto hasDerefInBase = clang::hasPublicDerefInBase(Base);
if (!hasDerefInBase) {
AnyInconclusiveBase = true;
return false;
}
return (*hasDerefInBase) != nullptr;
};
hasDeref = R->lookupInBases(hasPublicDerefInBase, Paths,
/*LookupInDependent =*/true);
Comment on lines +124 to +125
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overwrites the old hasDeref value, so it may give an incorrect answer if eg. Deref is in the class itself but Ref is in a base class.

In the other copy of similar code we see hasDeref = hasDeref || lookupInBases(...) which is probably more correct.

(Which makes me wish there weren't two copies of this code in the first place.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

if (AnyInconclusiveBase)
return std::nullopt;

return BasesResult;
return hasRef && hasDeref;
}

bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
Expand Down
13 changes: 9 additions & 4 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,15 @@ 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 *>
hasPublicRefInBase(const CXXBaseSpecifier *Base);

/// \returns CXXRecordDecl of the base if the type has deref as a public
/// method, nullptr if not, std::nullopt if inconclusive.
std::optional<const clang::CXXRecordDecl *>
hasPublicDerefInBase(const CXXBaseSpecifier *Base);

/// \returns true if \p Class is ref-countable, false if not, std::nullopt if
/// inconclusive.
Expand Down
47 changes: 43 additions & 4 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::hasPublicRefInBase(Base);
auto hasDerefInBase = clang::hasPublicDerefInBase(Base);

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::hasPublicRefInBase(Base);
if (!hasRefInBase) {
AnyInconclusiveBase = true;
return false;
}
return (*hasRefInBase) != nullptr;
};
const auto hasPublicDerefInBase =
[&AnyInconclusiveBase](const CXXBaseSpecifier *Base,
CXXBasePath &) {
auto hasDerefInBase = clang::hasPublicDerefInBase(Base);
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;
}

Expand Down
22 changes: 22 additions & 0 deletions ...g/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-ref-deref-on-diff-classes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
}
23 changes: 23 additions & 0 deletions clang/test/Analysis/Checkers/WebKit/uncounted-members-ref-deref-on-diff-classes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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'}}
};