Author: Ryosuke Niwa Date: 2024-09-27T00:42:18-07:00 New Revision: 3c0984309ed338560f902a918d6f99959b4c7c33
URL: https://github.com/llvm/llvm-project/commit/3c0984309ed338560f902a918d6f99959b4c7c33 DIFF: https://github.com/llvm/llvm-project/commit/3c0984309ed338560f902a918d6f99959b4c7c33.diff LOG: [alpha.webkit.NoUncheckedPtrMemberChecker] Introduce member variable checker for CheckedPtr/CheckedRef (#108352) This PR introduces new WebKit checker to warn a member variable that is a raw reference or a raw pointer to an object, which is capable of creating a CheckedRef/CheckedPtr. Added: clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp Modified: clang/docs/analyzer/checkers.rst clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h clang/test/Analysis/Checkers/WebKit/mock-types.h llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Checkers/BUILD.gn Removed: clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp ################################################################################ diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index 47c6fc680deb1b..9847d449d76d04 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -3442,6 +3442,27 @@ Check for non-determinism caused by sorting of pointers. alpha.WebKit ^^^^^^^^^^^^ +.. _alpha-webkit-NoUncheckedPtrMemberChecker: + +alpha.webkit.NoUncheckedPtrMemberChecker +"""""""""""""""""""""""""""""""""""""""" +Raw pointers and references to an object which supports CheckedPtr or CheckedRef can't be used as class members. Only CheckedPtr, CheckedRef, RefPtr, or Ref are allowed. + +.. code-block:: cpp + + struct CheckableObj { + void incrementPtrCount() {} + void decrementPtrCount() {} + }; + + struct Foo { + CheckableObj* ptr; // warn + CheckableObj& ptr; // warn + // ... + }; + +See `WebKit Guidelines for Safer C++ Programming <https://github.com/WebKit/WebKit/wiki/Safer-CPP-Guidelines>`_ for details. + .. _alpha-webkit-UncountedCallArgsChecker: alpha.webkit.UncountedCallArgsChecker diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 7da0d0a87e8c0c..747ebd8c2e4dee 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1764,6 +1764,10 @@ def UncountedLambdaCapturesChecker : Checker<"UncountedLambdaCapturesChecker">, let ParentPackage = WebKitAlpha in { +def NoUncheckedPtrMemberChecker : Checker<"NoUncheckedPtrMemberChecker">, + HelpText<"Check for no unchecked member variables.">, + Documentation<HasDocumentation>; + def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">, HelpText<"Check uncounted call arguments.">, Documentation<HasDocumentation>; diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 414282d58f779f..6da3665ab9a4df 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -132,7 +132,7 @@ add_clang_library(clangStaticAnalyzerCheckers VLASizeChecker.cpp ValistChecker.cpp VirtualCallChecker.cpp - WebKit/NoUncountedMembersChecker.cpp + WebKit/RawPtrRefMemberChecker.cpp WebKit/ASTUtils.cpp WebKit/PtrTypesSemantics.cpp WebKit/RefCntblBaseVirtualDtorChecker.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 54c99c3c1b37f9..4d145be808f6d8 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -19,8 +19,7 @@ using namespace clang; namespace { -bool hasPublicMethodInBaseClass(const CXXRecordDecl *R, - const char *NameToMatch) { +bool hasPublicMethodInBaseClass(const CXXRecordDecl *R, StringRef NameToMatch) { assert(R); assert(R->hasDefinition()); @@ -37,7 +36,7 @@ bool hasPublicMethodInBaseClass(const CXXRecordDecl *R, namespace clang { std::optional<const clang::CXXRecordDecl *> -hasPublicMethodInBase(const CXXBaseSpecifier *Base, const char *NameToMatch) { +hasPublicMethodInBase(const CXXBaseSpecifier *Base, StringRef NameToMatch) { assert(Base); const Type *T = Base->getType().getTypePtrOrNull(); @@ -53,16 +52,17 @@ hasPublicMethodInBase(const CXXBaseSpecifier *Base, const char *NameToMatch) { return hasPublicMethodInBaseClass(R, NameToMatch) ? R : nullptr; } -std::optional<bool> isRefCountable(const CXXRecordDecl* R) -{ +std::optional<bool> isSmartPtrCompatible(const CXXRecordDecl *R, + StringRef IncMethodName, + StringRef DecMethodName) { assert(R); R = R->getDefinition(); if (!R) return std::nullopt; - bool hasRef = hasPublicMethodInBaseClass(R, "ref"); - bool hasDeref = hasPublicMethodInBaseClass(R, "deref"); + bool hasRef = hasPublicMethodInBaseClass(R, IncMethodName); + bool hasDeref = hasPublicMethodInBaseClass(R, DecMethodName); if (hasRef && hasDeref) return true; @@ -70,15 +70,15 @@ std::optional<bool> isRefCountable(const CXXRecordDecl* R) Paths.setOrigin(const_cast<CXXRecordDecl *>(R)); 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 hasPublicRefInBase = [&](const CXXBaseSpecifier *Base, + CXXBasePath &) { + auto hasRefInBase = clang::hasPublicMethodInBase(Base, IncMethodName); + if (!hasRefInBase) { + AnyInconclusiveBase = true; + return false; + } + return (*hasRefInBase) != nullptr; + }; hasRef = hasRef || R->lookupInBases(hasPublicRefInBase, Paths, /*LookupInDependent =*/true); @@ -86,15 +86,15 @@ std::optional<bool> isRefCountable(const CXXRecordDecl* R) return std::nullopt; Paths.clear(); - const auto hasPublicDerefInBase = - [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) { - auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref"); - if (!hasDerefInBase) { - AnyInconclusiveBase = true; - return false; - } - return (*hasDerefInBase) != nullptr; - }; + const auto hasPublicDerefInBase = [&](const CXXBaseSpecifier *Base, + CXXBasePath &) { + auto hasDerefInBase = clang::hasPublicMethodInBase(Base, DecMethodName); + if (!hasDerefInBase) { + AnyInconclusiveBase = true; + return false; + } + return (*hasDerefInBase) != nullptr; + }; hasDeref = hasDeref || R->lookupInBases(hasPublicDerefInBase, Paths, /*LookupInDependent =*/true); if (AnyInconclusiveBase) @@ -103,11 +103,23 @@ std::optional<bool> isRefCountable(const CXXRecordDecl* R) return hasRef && hasDeref; } +std::optional<bool> isRefCountable(const clang::CXXRecordDecl *R) { + return isSmartPtrCompatible(R, "ref", "deref"); +} + +std::optional<bool> isCheckedPtrCapable(const clang::CXXRecordDecl *R) { + return isSmartPtrCompatible(R, "incrementPtrCount", "decrementPtrCount"); +} + bool isRefType(const std::string &Name) { return Name == "Ref" || Name == "RefAllowingPartiallyDestroyed" || Name == "RefPtr" || Name == "RefPtrAllowingPartiallyDestroyed"; } +bool isCheckedPtr(const std::string &Name) { + return Name == "CheckedPtr" || Name == "CheckedRef"; +} + bool isCtorOfRefCounted(const clang::FunctionDecl *F) { assert(F); const std::string &FunctionName = safeGetName(F); @@ -217,6 +229,15 @@ bool isRefCounted(const CXXRecordDecl *R) { return false; } +bool isCheckedPtr(const CXXRecordDecl *R) { + assert(R); + if (auto *TmplR = R->getTemplateInstantiationPattern()) { + const auto &ClassName = safeGetName(TmplR); + return isCheckedPtr(ClassName); + } + return false; +} + bool isPtrConversion(const FunctionDecl *F) { assert(F); if (isCtorOfRefCounted(F)) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h index e2d0342bebd52c..3528c52a7d659d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -34,15 +34,23 @@ class Type; /// \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); +hasPublicMethodInBase(const CXXBaseSpecifier *Base, + llvm::StringRef NameToMatch); /// \returns true if \p Class is ref-countable, false if not, std::nullopt if /// inconclusive. -std::optional<bool> isRefCountable(const clang::CXXRecordDecl* Class); +std::optional<bool> isRefCountable(const clang::CXXRecordDecl *Class); + +/// \returns true if \p Class is checked-pointer compatible, false if not, +/// std::nullopt if inconclusive. +std::optional<bool> isCheckedPtrCapable(const clang::CXXRecordDecl *Class); /// \returns true if \p Class is ref-counted, false if not. bool isRefCounted(const clang::CXXRecordDecl *Class); +/// \returns true if \p Class is a CheckedPtr / CheckedRef, false if not. +bool isCheckedPtr(const clang::CXXRecordDecl *Class); + /// \returns true if \p Class is ref-countable AND not ref-counted, false if /// not, std::nullopt if inconclusive. std::optional<bool> isUncounted(const clang::QualType T); diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp similarity index 63% rename from clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp rename to clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp index 69a0eb3086ab72..2ce6bc330e0ca1 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp @@ -1,4 +1,4 @@ -//=======- NoUncountedMembersChecker.cpp -------------------------*- C++ -*-==// +//=======- RawPtrRefMemberChecker.cpp ----------------------------*- C++ -*-==// // // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // See https://llvm.org/LICENSE.txt for license information. @@ -25,18 +25,21 @@ using namespace ento; namespace { -class NoUncountedMemberChecker +class RawPtrRefMemberChecker : public Checker<check::ASTDecl<TranslationUnitDecl>> { private: BugType Bug; mutable BugReporter *BR; public: - NoUncountedMemberChecker() - : Bug(this, - "Member variable is a raw-pointer/reference to reference-countable " - "type", - "WebKit coding guidelines") {} + RawPtrRefMemberChecker(const char *description) + : Bug(this, description, "WebKit coding guidelines") {} + + virtual std::optional<bool> + isPtrCompatible(const clang::CXXRecordDecl *) const = 0; + virtual bool isPtrCls(const clang::CXXRecordDecl *) const = 0; + virtual const char *typeName() const = 0; + virtual const char *invariant() const = 0; void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR, BugReporter &BRArg) const { @@ -46,8 +49,8 @@ class NoUncountedMemberChecker // visit template instantiations or lambda classes. We // want to visit those, so we make our own RecursiveASTVisitor. struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> { - const NoUncountedMemberChecker *Checker; - explicit LocalVisitor(const NoUncountedMemberChecker *Checker) + const RawPtrRefMemberChecker *Checker; + explicit LocalVisitor(const RawPtrRefMemberChecker *Checker) : Checker(Checker) { assert(Checker); } @@ -77,9 +80,9 @@ class NoUncountedMemberChecker if (auto *MemberCXXRD = MemberType->getPointeeCXXRecordDecl()) { // If we don't see the definition we just don't know. if (MemberCXXRD->hasDefinition()) { - std::optional<bool> isRCAble = isRefCountable(MemberCXXRD); - if (isRCAble && *isRCAble) - reportBug(Member, MemberType, MemberCXXRD, RD); + std::optional<bool> isRCAble = isPtrCompatible(MemberCXXRD); + if (isRCAble && *isRCAble) + reportBug(Member, MemberType, MemberCXXRD, RD); } } } @@ -114,7 +117,7 @@ class NoUncountedMemberChecker // a member but we trust them to handle it correctly. auto CXXRD = llvm::dyn_cast_or_null<CXXRecordDecl>(RD); if (CXXRD) - return isRefCounted(CXXRD); + return isPtrCls(CXXRD); return false; } @@ -134,10 +137,10 @@ class NoUncountedMemberChecker Os << " in "; printQuotedQualifiedName(Os, ClassCXXRD); Os << " is a " - << (isa<PointerType>(MemberType) ? "raw pointer" : "reference") - << " to ref-countable type "; + << (isa<PointerType>(MemberType) ? "raw pointer" : "reference") << " to " + << typeName() << " "; printQuotedQualifiedName(Os, MemberCXXRD); - Os << "; member variables must be ref-counted."; + Os << "; " << invariant() << "."; PathDiagnosticLocation BSLoc(Member->getSourceRange().getBegin(), BR->getSourceManager()); @@ -146,13 +149,67 @@ class NoUncountedMemberChecker BR->emitReport(std::move(Report)); } }; + +class NoUncountedMemberChecker final : public RawPtrRefMemberChecker { +public: + NoUncountedMemberChecker() + : RawPtrRefMemberChecker("Member variable is a raw-pointer/reference to " + "reference-countable type") {} + + std::optional<bool> + isPtrCompatible(const clang::CXXRecordDecl *R) const final { + return isRefCountable(R); + } + + bool isPtrCls(const clang::CXXRecordDecl *R) const final { + return isRefCounted(R); + } + + const char *typeName() const final { return "ref-countable type"; } + + const char *invariant() const final { + return "member variables must be Ref, RefPtr, WeakRef, or WeakPtr"; + } +}; + +class NoUncheckedPtrMemberChecker final : public RawPtrRefMemberChecker { +public: + NoUncheckedPtrMemberChecker() + : RawPtrRefMemberChecker("Member variable is a raw-pointer/reference to " + "checked-pointer capable type") {} + + std::optional<bool> + isPtrCompatible(const clang::CXXRecordDecl *R) const final { + return isCheckedPtrCapable(R); + } + + bool isPtrCls(const clang::CXXRecordDecl *R) const final { + return isCheckedPtr(R); + } + + const char *typeName() const final { return "CheckedPtr capable type"; } + + const char *invariant() const final { + return "member variables must be a CheckedPtr, CheckedRef, WeakRef, or " + "WeakPtr"; + } +}; + } // namespace void ento::registerNoUncountedMemberChecker(CheckerManager &Mgr) { Mgr.registerChecker<NoUncountedMemberChecker>(); } -bool ento::shouldRegisterNoUncountedMemberChecker( +bool ento::shouldRegisterNoUncountedMemberChecker(const CheckerManager &Mgr) { + return true; +} + +void ento::registerNoUncheckedPtrMemberChecker(CheckerManager &Mgr) { + Mgr.registerChecker<NoUncheckedPtrMemberChecker>(); +} + +bool ento::shouldRegisterNoUncheckedPtrMemberChecker( const CheckerManager &Mgr) { return true; } diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h index c427b22fd683e5..933b4c5e62a79c 100644 --- a/clang/test/Analysis/Checkers/WebKit/mock-types.h +++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h @@ -108,4 +108,52 @@ struct RefCountable { template <typename T> T *downcast(T *t) { return t; } +template <typename T> struct CheckedRef { +private: + T *t; + +public: + CheckedRef() : t{} {}; + CheckedRef(T &t) : t(t) { t->incrementPtrCount(); } + CheckedRef(const CheckedRef& o) : t(o.t) { if (t) t->incrementPtrCount(); } + ~CheckedRef() { if (t) t->decrementPtrCount(); } + T &get() { return *t; } + T *ptr() { return t; } + T *operator->() { return t; } + operator const T &() const { return *t; } + operator T &() { return *t; } +}; + +template <typename T> struct CheckedPtr { +private: + T *t; + +public: + CheckedPtr() : t(nullptr) {} + CheckedPtr(T *t) + : t(t) { + if (t) + t->incrementPtrCount(); + } + CheckedPtr(Ref<T>&& o) + : t(o.leakRef()) + { } + ~CheckedPtr() { + if (t) + t->decrementPtrCount(); + } + T *get() { return t; } + T *operator->() { return t; } + const T *operator->() const { return t; } + T &operator*() { return *t; } + CheckedPtr &operator=(T *) { return *this; } + operator bool() const { return t; } +}; + +class CheckedObj { +public: + void incrementPtrCount(); + void decrementPtrCount(); +}; + #endif diff --git a/clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp b/clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp new file mode 100644 index 00000000000000..0189b0cd50fcc9 --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp @@ -0,0 +1,52 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.NoUncheckedPtrMemberChecker -verify %s + +#include "mock-types.h" + +namespace members { + + struct Foo { + private: + CheckedObj* a = nullptr; +// expected-warning@-1{{Member variable 'a' in 'members::Foo' is a raw pointer to CheckedPtr capable type 'CheckedObj'}} + CheckedObj& b; +// expected-warning@-1{{Member variable 'b' in 'members::Foo' is a reference to CheckedPtr capable type 'CheckedObj'}} + + [[clang::suppress]] + CheckedObj* a_suppressed = nullptr; + + [[clang::suppress]] + CheckedObj& b_suppressed; + + CheckedPtr<CheckedObj> c; + CheckedRef<CheckedObj> d; + + public: + Foo(); + }; + + template <typename S> + struct FooTmpl { + S* e; +// expected-warning@-1{{Member variable 'e' in 'members::FooTmpl<CheckedObj>' is a raw pointer to CheckedPtr capable type 'CheckedObj'}} + }; + + void forceTmplToInstantiate(FooTmpl<CheckedObj>) { } + +} // namespace members + +namespace ignore_unions { + + union Foo { + CheckedObj* a; + CheckedPtr<CheckedObj> c; + CheckedRef<CheckedObj> d; + }; + + template<class T> + union FooTmpl { + T* a; + }; + + void forceTmplToInstantiate(FooTmpl<CheckedObj>) { } + +} // namespace ignore_unions diff --git a/llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Checkers/BUILD.gn b/llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Checkers/BUILD.gn index 3b640ae41b9f62..7a6c360e88c14e 100644 --- a/llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Checkers/BUILD.gn +++ b/llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Checkers/BUILD.gn @@ -141,7 +141,7 @@ static_library("Checkers") { "VforkChecker.cpp", "VirtualCallChecker.cpp", "WebKit/ASTUtils.cpp", - "WebKit/NoUncountedMembersChecker.cpp", + "WebKit/RawPtrRefMemberChecker.cpp", "WebKit/PtrTypesSemantics.cpp", "WebKit/RefCntblBaseVirtualDtorChecker.cpp", "WebKit/UncountedCallArgsChecker.cpp", _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
