https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/128641
Add a new WebKit checker for member variables and instance variables of NS and CF types. A member variable or instance variable to a CF type should be RetainPtr regardless of whether ARC is enabled or disabled, and that of a NS type should be RetainPtr when ARC is disabled. >From a4cd301d6176a8ee3441d0c74f6d2c4e32a50cdd Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa <rn...@webkit.org> Date: Mon, 24 Feb 2025 23:01:31 -0800 Subject: [PATCH] [alpha.webkit.NoUnretainedMemberChecker] Add a new WebKit checker for unretained member variables and ivars. Add a new WebKit checker for member variables and instance variables of NS and CF types. A member variable or instance variable to a CF type should be RetainPtr regardless of whether ARC is enabled or disabled, and that of a NS type should be RetainPtr when ARC is disabled. --- clang/docs/analyzer/checkers.rst | 13 ++ .../clang/StaticAnalyzer/Checkers/Checkers.td | 4 + .../WebKit/RawPtrRefMemberChecker.cpp | 117 +++++++++++++++--- .../Checkers/WebKit/unretained-members-arc.mm | 39 ++++++ .../Checkers/WebKit/unretained-members.mm | 57 +++++++++ 5 files changed, 210 insertions(+), 20 deletions(-) create mode 100644 clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm create mode 100644 clang/test/Analysis/Checkers/WebKit/unretained-members.mm diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index c1eedb33e74d2..78372c3f1ca66 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -3487,6 +3487,19 @@ Raw pointers and references to an object which supports CheckedPtr or CheckedRef See `WebKit Guidelines for Safer C++ Programming <https://github.com/WebKit/WebKit/wiki/Safer-CPP-Guidelines>`_ for details. +alpha.webkit.NoUnretainedMemberChecker +"""""""""""""""""""""""""""""""""""""""" +Raw pointers and references to a NS or CF object can't be used as class members or ivars. Only RetainPtr is allowed. + +.. code-block:: cpp + + struct Foo { + NSObject *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 410f841630660..578b377cb0849 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1766,6 +1766,10 @@ def NoUncheckedPtrMemberChecker : Checker<"NoUncheckedPtrMemberChecker">, HelpText<"Check for no unchecked member variables.">, Documentation<HasDocumentation>; +def NoUnretainedMemberChecker : Checker<"NoUnretainedMemberChecker">, + HelpText<"Check for no unretained member variables.">, + Documentation<HasDocumentation>; + def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">, HelpText<"Check uncounted call arguments.">, Documentation<HasDocumentation>; diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp index 963f59831c8ed..0e0fd95e1d79d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp @@ -31,12 +31,16 @@ class RawPtrRefMemberChecker BugType Bug; mutable BugReporter *BR; +protected: + mutable std::optional<RetainTypeChecker> RTC; + public: RawPtrRefMemberChecker(const char *description) : Bug(this, description, "WebKit coding guidelines") {} virtual std::optional<bool> - isPtrCompatible(const clang::CXXRecordDecl *) const = 0; + isPtrCompatible(const clang::QualType, + const clang::CXXRecordDecl *R) const = 0; virtual bool isPtrCls(const clang::CXXRecordDecl *) const = 0; virtual const char *typeName() const = 0; virtual const char *invariant() const = 0; @@ -57,6 +61,12 @@ class RawPtrRefMemberChecker ShouldVisitImplicitCode = false; } + bool VisitTypedefDecl(const TypedefDecl *TD) override { + if (Checker->RTC) + Checker->RTC->visitTypedef(TD); + return true; + } + bool VisitRecordDecl(const RecordDecl *RD) override { Checker->visitRecordDecl(RD); return true; @@ -69,6 +79,8 @@ class RawPtrRefMemberChecker }; LocalVisitor visitor(this); + if (RTC) + RTC->visitTranslationUnitDecl(TUD); visitor.TraverseDecl(TUD); } @@ -77,16 +89,22 @@ class RawPtrRefMemberChecker return; for (auto *Member : RD->fields()) { - const Type *MemberType = Member->getType().getTypePtrOrNull(); + auto QT = Member->getType(); + const Type *MemberType = QT.getTypePtrOrNull(); if (!MemberType) continue; if (auto *MemberCXXRD = MemberType->getPointeeCXXRecordDecl()) { - // If we don't see the definition we just don't know. - if (MemberCXXRD->hasDefinition()) { - std::optional<bool> isRCAble = isPtrCompatible(MemberCXXRD); - if (isRCAble && *isRCAble) - reportBug(Member, MemberType, MemberCXXRD, RD); + std::optional<bool> IsCompatible = isPtrCompatible(QT, MemberCXXRD); + if (IsCompatible && *IsCompatible) + reportBug(Member, MemberType, MemberCXXRD, RD); + } else { + std::optional<bool> IsCompatible = isPtrCompatible(QT, nullptr); + auto* PointeeType = MemberType->getPointeeType().getTypePtrOrNull(); + if (IsCompatible && *IsCompatible) { + auto *Desugared = PointeeType->getUnqualifiedDesugaredType(); + if (auto *ObjCType = dyn_cast_or_null<ObjCInterfaceType>(Desugared)) + reportBug(Member, MemberType, ObjCType->getDecl(), RD); } } } @@ -107,11 +125,12 @@ class RawPtrRefMemberChecker void visitIvarDecl(const ObjCContainerDecl *CD, const ObjCIvarDecl *Ivar) const { - const Type *IvarType = Ivar->getType().getTypePtrOrNull(); + auto QT = Ivar->getType(); + const Type *IvarType = QT.getTypePtrOrNull(); if (!IvarType) return; if (auto *IvarCXXRD = IvarType->getPointeeCXXRecordDecl()) { - std::optional<bool> IsCompatible = isPtrCompatible(IvarCXXRD); + std::optional<bool> IsCompatible = isPtrCompatible(QT, IvarCXXRD); if (IsCompatible && *IsCompatible) reportBug(Ivar, IvarType, IvarCXXRD, CD); } @@ -151,13 +170,13 @@ class RawPtrRefMemberChecker return false; } - template <typename DeclType, typename ParentDeclType> + template <typename DeclType, typename PointeeType, typename ParentDeclType> void reportBug(const DeclType *Member, const Type *MemberType, - const CXXRecordDecl *MemberCXXRD, + const PointeeType *Pointee, const ParentDeclType *ClassCXXRD) const { assert(Member); assert(MemberType); - assert(MemberCXXRD); + assert(Pointee); SmallString<100> Buf; llvm::raw_svector_ostream Os(Buf); @@ -169,10 +188,13 @@ class RawPtrRefMemberChecker printQuotedName(Os, Member); Os << " in "; printQuotedQualifiedName(Os, ClassCXXRD); - Os << " is a " - << (isa<PointerType>(MemberType) ? "raw pointer" : "reference") << " to " - << typeName() << " "; - printQuotedQualifiedName(Os, MemberCXXRD); + Os << " is a "; + if (printPointer(Os, MemberType) == PrintDeclKind::Pointer) { + auto Typedef = MemberType->getAs<TypedefType>(); + assert(Typedef); + printQuotedQualifiedName(Os, Typedef->getDecl()); + } else + printQuotedQualifiedName(Os, Pointee); Os << "; " << invariant() << "."; PathDiagnosticLocation BSLoc(Member->getSourceRange().getBegin(), @@ -181,6 +203,16 @@ class RawPtrRefMemberChecker Report->addRange(Member->getSourceRange()); BR->emitReport(std::move(Report)); } + + enum class PrintDeclKind { Pointee, Pointer }; + virtual PrintDeclKind printPointer(llvm::raw_svector_ostream& Os, + const Type *T) const { + T = T->getUnqualifiedDesugaredType(); + bool IsPtr = isa<PointerType>(T) || isa<ObjCObjectPointerType>(T); + Os << (IsPtr ? "raw pointer" : "reference") << " to " + << typeName() << " "; + return PrintDeclKind::Pointee; + } }; class NoUncountedMemberChecker final : public RawPtrRefMemberChecker { @@ -190,8 +222,9 @@ class NoUncountedMemberChecker final : public RawPtrRefMemberChecker { "reference-countable type") {} std::optional<bool> - isPtrCompatible(const clang::CXXRecordDecl *R) const final { - return isRefCountable(R); + isPtrCompatible(const clang::QualType, + const clang::CXXRecordDecl *R) const final { + return R && isRefCountable(R); } bool isPtrCls(const clang::CXXRecordDecl *R) const final { @@ -212,8 +245,9 @@ class NoUncheckedPtrMemberChecker final : public RawPtrRefMemberChecker { "checked-pointer capable type") {} std::optional<bool> - isPtrCompatible(const clang::CXXRecordDecl *R) const final { - return isCheckedPtrCapable(R); + isPtrCompatible(const clang::QualType, + const clang::CXXRecordDecl *R) const final { + return R && isCheckedPtrCapable(R); } bool isPtrCls(const clang::CXXRecordDecl *R) const final { @@ -228,6 +262,40 @@ class NoUncheckedPtrMemberChecker final : public RawPtrRefMemberChecker { } }; +class NoUnretainedMemberChecker final : public RawPtrRefMemberChecker { +public: + NoUnretainedMemberChecker() + : RawPtrRefMemberChecker("Member variable is a raw-pointer/reference to " + "retainable type") { + RTC = RetainTypeChecker(); + } + + std::optional<bool> + isPtrCompatible(const clang::QualType QT, + const clang::CXXRecordDecl *) const final { + return RTC->isUnretained(QT); + } + + bool isPtrCls(const clang::CXXRecordDecl *R) const final { + return isRetainPtr(R); + } + + const char *typeName() const final { return "retainable type"; } + + const char *invariant() const final { + return "member variables must be a RetainPtr"; + } + + PrintDeclKind printPointer(llvm::raw_svector_ostream& Os, + const Type *T) const final { + if (!isa<ObjCObjectPointerType>(T) && T->getAs<TypedefType>()) { + Os << typeName() << " "; + return PrintDeclKind::Pointer; + } + return RawPtrRefMemberChecker::printPointer(Os, T); + } +}; + } // namespace void ento::registerNoUncountedMemberChecker(CheckerManager &Mgr) { @@ -246,3 +314,12 @@ bool ento::shouldRegisterNoUncheckedPtrMemberChecker( const CheckerManager &Mgr) { return true; } + +void ento::registerNoUnretainedMemberChecker(CheckerManager &Mgr) { + Mgr.registerChecker<NoUnretainedMemberChecker>(); +} + +bool ento::shouldRegisterNoUnretainedMemberChecker( + const CheckerManager &Mgr) { + return true; +} diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm b/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm new file mode 100644 index 0000000000000..9820c875b87c0 --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm @@ -0,0 +1,39 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.NoUnretainedMemberChecker -fobjc-arc -verify %s + +#include "objc-mock-types.h" + +namespace members { + + struct Foo { + private: + SomeObj* a = nullptr; + + [[clang::suppress]] + SomeObj* a_suppressed = nullptr; + + protected: + RetainPtr<SomeObj> b; + + public: + SomeObj* c = nullptr; + RetainPtr<SomeObj> d; + + CFMutableArrayRef e = nullptr; +// expected-warning@-1{{Member variable 'e' in 'members::Foo' is a retainable type 'CFMutableArrayRef'}} + }; + + template<class T, class S> + struct FooTmpl { + T* x; + S y; +// expected-warning@-1{{Member variable 'y' in 'members::FooTmpl<SomeObj, __CFArray *>' is a raw pointer to retainable type}} + }; + + void forceTmplToInstantiate(FooTmpl<SomeObj, CFMutableArrayRef>) {} + + struct [[clang::suppress]] FooSuppressed { + private: + SomeObj* a = nullptr; + }; + +} diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm new file mode 100644 index 0000000000000..fc607bf52b176 --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm @@ -0,0 +1,57 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.NoUnretainedMemberChecker -verify %s + +#include "objc-mock-types.h" + +namespace members { + + struct Foo { + private: + SomeObj* a = nullptr; +// expected-warning@-1{{Member variable 'a' in 'members::Foo' is a raw pointer to retainable type}} + + [[clang::suppress]] + SomeObj* a_suppressed = nullptr; + + protected: + RetainPtr<SomeObj> b; + + public: + SomeObj* c = nullptr; +// expected-warning@-1{{Member variable 'c' in 'members::Foo' is a raw pointer to retainable type}} + RetainPtr<SomeObj> d; + + CFMutableArrayRef e = nullptr; +// expected-warning@-1{{Member variable 'e' in 'members::Foo' is a retainable type 'CFMutableArrayRef'}} + }; + + template<class T, class S> + struct FooTmpl { + T* a; +// expected-warning@-1{{Member variable 'a' in 'members::FooTmpl<SomeObj, __CFArray *>' is a raw pointer to retainable type}} + S b; +// expected-warning@-1{{Member variable 'b' in 'members::FooTmpl<SomeObj, __CFArray *>' is a raw pointer to retainable type}} + }; + + void forceTmplToInstantiate(FooTmpl<SomeObj, CFMutableArrayRef>) {} + + struct [[clang::suppress]] FooSuppressed { + private: + SomeObj* a = nullptr; + }; + +} + +namespace ignore_unions { + union Foo { + SomeObj* a; + RetainPtr<SomeObj> b; + CFMutableArrayRef c; + }; + + template<class T> + union RefPtr { + T* a; + }; + + void forceTmplToInstantiate(RefPtr<SomeObj>) {} +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
