diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index c1f180f31338c..d2b6634105800 100644 --- 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 -isRefCountable(const CXXBaseSpecifier* Base) -{ +std::optional +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 isRefCountable(const CXXRecordDecl* R) @@ -70,29 +60,45 @@ std::optional 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(R)); bool AnyInconclusiveBase = false; - const auto isRefCountableBase = - [&AnyInconclusiveBase](const CXXBaseSpecifier* Base, CXXBasePath&) { - std::optional 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 91e3ccf2ec304..45b21cc091844 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 and // Ref. -/// \returns CXXRecordDecl of the base if the type is ref-countable, nullptr if -/// not, std::nullopt if inconclusive. -std::optional -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 +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 bd7c50ccfa9c4..d879c110b75d3 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 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 new file mode 100644 index 0000000000000..aac58c0c1dda6 --- /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 struct RefCounted : RefCountedBase { +public: + void deref() const { } +}; + +struct Base : RefCounted { +// expected-warning@-1{{Struct 'RefCounted' 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 new file mode 100644 index 0000000000000..4198b2388fed8 --- /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 class RefCounted : public RefCountedBase { +public: + virtual ~RefCounted() { } + void deref() const { } +}; + +class TreeNode : public RefCounted { +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'}} +};