Author: Ryosuke Niwa Date: 2025-04-10T15:26:10-07:00 New Revision: c26d097d0c3e0e3924e273c0ec2d1e57192e66c8
URL: https://github.com/llvm/llvm-project/commit/c26d097d0c3e0e3924e273c0ec2d1e57192e66c8 DIFF: https://github.com/llvm/llvm-project/commit/c26d097d0c3e0e3924e273c0ec2d1e57192e66c8.diff LOG: [alpha.webkit.RetainPtrCtorAdoptChecker] Support adopt(cast(copy(~)) (#132316) This PR adds the support for recognizing calling adoptCF/adoptNS on the result of a cast operation on the return value of a function which creates NS or CF types. It also fixes a bug that we weren't reporting memory leaks when CF types are created without ever calling RetainPtr's constructor, adoptCF, or adoptNS. To do this, this PR adds a new mechanism to report a memory leak whenever create or copy CF functions are invoked unless this CallExpr has already been visited while validating a call to adoptCF. Also added an early exit when isOwned returns IsOwnedResult::Skip due to an unresolved template argument. Added: Modified: clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp clang/lib/StaticAnalyzer/Checkers/WebKit/RetainPtrCtorAdoptChecker.cpp clang/test/Analysis/Checkers/WebKit/objc-mock-types.h clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use-arc.mm clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use.mm Removed: ################################################################################ diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 430f1c6db50b4..781b0de5abd2f 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -125,6 +125,16 @@ bool isCheckedPtr(const std::string &Name) { return Name == "CheckedPtr" || Name == "CheckedRef"; } +bool isSmartPtrClass(const std::string &Name) { + return isRefType(Name) || isCheckedPtr(Name) || isRetainPtr(Name) || + Name == "WeakPtr" || Name == "WeakPtr" || Name == "WeakPtrFactory" || + Name == "WeakPtrFactoryWithBitField" || Name == "WeakPtrImplBase" || + Name == "WeakPtrImplBaseSingleThread" || Name == "ThreadSafeWeakPtr" || + Name == "ThreadSafeWeakOrStrongPtr" || + Name == "ThreadSafeWeakPtrControlBlock" || + Name == "ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr"; +} + bool isCtorOfRefCounted(const clang::FunctionDecl *F) { assert(F); const std::string &FunctionName = safeGetName(F); @@ -222,8 +232,13 @@ void RetainTypeChecker::visitTypedef(const TypedefDecl *TD) { auto PointeeQT = QT->getPointeeType(); const RecordType *RT = PointeeQT->getAs<RecordType>(); - if (!RT) + if (!RT) { + if (TD->hasAttr<ObjCBridgeAttr>() || TD->hasAttr<ObjCBridgeMutableAttr>()) { + if (auto *Type = TD->getTypeForDecl()) + RecordlessTypes.insert(Type); + } return; + } for (auto *Redecl : RT->getDecl()->getMostRecentDecl()->redecls()) { if (Redecl->getAttr<ObjCBridgeAttr>() || @@ -240,6 +255,17 @@ bool RetainTypeChecker::isUnretained(const QualType QT, bool ignoreARC) { auto CanonicalType = QT.getCanonicalType(); auto PointeeType = CanonicalType->getPointeeType(); auto *RT = dyn_cast_or_null<RecordType>(PointeeType.getTypePtrOrNull()); + if (!RT) { + auto *Type = QT.getTypePtrOrNull(); + while (Type) { + if (RecordlessTypes.contains(Type)) + return true; + auto *ET = dyn_cast_or_null<ElaboratedType>(Type); + if (!ET) + break; + Type = ET->desugar().getTypePtrOrNull(); + } + } return RT && CFPointees.contains(RT); } diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h index 096675fb912f2..97c9d0510e67d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -70,6 +70,7 @@ std::optional<bool> isUnchecked(const clang::QualType T); /// underlying pointer type. class RetainTypeChecker { llvm::DenseSet<const RecordType *> CFPointees; + llvm::DenseSet<const Type *> RecordlessTypes; bool IsARCEnabled{false}; public: @@ -135,6 +136,9 @@ bool isCheckedPtr(const std::string &Name); /// \returns true if \p Name is RetainPtr or its variant, false if not. bool isRetainPtr(const std::string &Name); +/// \returns true if \p Name is a smart pointer type name, false if not. +bool isSmartPtrClass(const std::string &Name); + /// \returns true if \p M is getter of a ref-counted class, false if not. std::optional<bool> isGetterOfSafePtr(const clang::CXXMethodDecl *Method); diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp index 13088920cfa19..20f7e9703c67c 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp @@ -69,7 +69,7 @@ class RawPtrRefCallArgsChecker } bool TraverseClassTemplateDecl(ClassTemplateDecl *Decl) override { - if (isRefType(safeGetName(Decl))) + if (isSmartPtrClass(safeGetName(Decl))) return true; return DynamicRecursiveASTVisitor::TraverseClassTemplateDecl(Decl); } diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp index 9975d1a91b681..a23f3aa356cb0 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp @@ -261,6 +261,12 @@ class RawPtrRefLocalVarsChecker return DynamicRecursiveASTVisitor::TraverseCompoundStmt(CS); return true; } + + bool TraverseClassTemplateDecl(ClassTemplateDecl *Decl) override { + if (isSmartPtrClass(safeGetName(Decl))) + return true; + return DynamicRecursiveASTVisitor::TraverseClassTemplateDecl(Decl); + } }; LocalVisitor visitor(this); diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RetainPtrCtorAdoptChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RetainPtrCtorAdoptChecker.cpp index fc3f510d1d499..d372c5d1ba626 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RetainPtrCtorAdoptChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RetainPtrCtorAdoptChecker.cpp @@ -34,6 +34,7 @@ class RetainPtrCtorAdoptChecker mutable BugReporter *BR = nullptr; mutable std::unique_ptr<RetainSummaryManager> Summaries; mutable llvm::DenseSet<const ValueDecl *> CreateOrCopyOutArguments; + mutable llvm::DenseSet<const Expr *> CreateOrCopyFnCall; mutable RetainTypeChecker RTC; public: @@ -120,7 +121,7 @@ class RetainPtrCtorAdoptChecker return; if (!isAdoptFn(F) || !CE->getNumArgs()) { - rememberOutArguments(CE, F); + checkCreateOrCopyFunction(CE, F, DeclWithIssue); return; } @@ -129,24 +130,29 @@ class RetainPtrCtorAdoptChecker auto Name = safeGetName(F); if (Result == IsOwnedResult::Unknown) Result = IsOwnedResult::NotOwned; - if (Result == IsOwnedResult::NotOwned && !isAllocInit(Arg) && - !isCreateOrCopy(Arg)) { - if (auto *DRE = dyn_cast<DeclRefExpr>(Arg)) { - if (CreateOrCopyOutArguments.contains(DRE->getDecl())) - return; - } - if (RTC.isARCEnabled() && isAdoptNS(F)) - reportUseAfterFree(Name, CE, DeclWithIssue, "when ARC is disabled"); - else - reportUseAfterFree(Name, CE, DeclWithIssue); + if (isAllocInit(Arg) || isCreateOrCopy(Arg)) { + CreateOrCopyFnCall.insert(Arg); // Avoid double reporting. + return; } + if (Result == IsOwnedResult::Owned || Result == IsOwnedResult::Skip) + return; + + if (auto *DRE = dyn_cast<DeclRefExpr>(Arg)) { + if (CreateOrCopyOutArguments.contains(DRE->getDecl())) + return; + } + if (RTC.isARCEnabled() && isAdoptNS(F)) + reportUseAfterFree(Name, CE, DeclWithIssue, "when ARC is disabled"); + else + reportUseAfterFree(Name, CE, DeclWithIssue); } - void rememberOutArguments(const CallExpr *CE, - const FunctionDecl *Callee) const { + void checkCreateOrCopyFunction(const CallExpr *CE, const FunctionDecl *Callee, + const Decl *DeclWithIssue) const { if (!isCreateOrCopyFunction(Callee)) return; + bool hasOutArgument = false; unsigned ArgCount = CE->getNumArgs(); for (unsigned ArgIndex = 0; ArgIndex < ArgCount; ++ArgIndex) { auto *Arg = CE->getArg(ArgIndex)->IgnoreParenCasts(); @@ -165,7 +171,12 @@ class RetainPtrCtorAdoptChecker if (!Decl) continue; CreateOrCopyOutArguments.insert(Decl); + hasOutArgument = true; } + if (!RTC.isUnretained(Callee->getReturnType())) + return; + if (!hasOutArgument && !CreateOrCopyFnCall.contains(CE)) + reportLeak(CE, DeclWithIssue); } void visitConstructExpr(const CXXConstructExpr *CE, @@ -192,6 +203,13 @@ class RetainPtrCtorAdoptChecker std::string Name = "RetainPtr constructor"; auto *Arg = CE->getArg(0)->IgnoreParenCasts(); auto Result = isOwned(Arg); + + if (isCreateOrCopy(Arg)) + CreateOrCopyFnCall.insert(Arg); // Avoid double reporting. + + if (Result == IsOwnedResult::Skip) + return; + if (Result == IsOwnedResult::Unknown) Result = IsOwnedResult::NotOwned; if (Result == IsOwnedResult::Owned) @@ -317,11 +335,22 @@ class RetainPtrCtorAdoptChecker if (auto *Callee = CE->getDirectCallee()) { if (isAdoptFn(Callee)) return IsOwnedResult::NotOwned; - if (safeGetName(Callee) == "__builtin___CFStringMakeConstantString") + auto Name = safeGetName(Callee); + if (Name == "__builtin___CFStringMakeConstantString") return IsOwnedResult::NotOwned; + if ((Name == "checked_cf_cast" || Name == "dynamic_cf_cast" || + Name == "checked_objc_cast" || Name == "dynamic_objc_cast") && + CE->getNumArgs() == 1) { + E = CE->getArg(0)->IgnoreParenCasts(); + continue; + } auto RetType = Callee->getReturnType(); if (isRetainPtrType(RetType)) return IsOwnedResult::NotOwned; + if (isCreateOrCopyFunction(Callee)) { + CreateOrCopyFnCall.insert(CE); + return IsOwnedResult::Owned; + } } else if (auto *CalleeExpr = CE->getCallee()) { if (isa<CXXDependentScopeMemberExpr>(CalleeExpr)) return IsOwnedResult::Skip; // Wait for instantiation. @@ -387,6 +416,20 @@ class RetainPtrCtorAdoptChecker Report->setDeclWithIssue(DeclWithIssue); BR->emitReport(std::move(Report)); } + + void reportLeak(const CallExpr *CE, const Decl *DeclWithIssue) const { + SmallString<100> Buf; + llvm::raw_svector_ostream Os(Buf); + + Os << "The return value is +1 and results in a memory leak."; + + PathDiagnosticLocation BSLoc(CE->getSourceRange().getBegin(), + BR->getSourceManager()); + auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc); + Report->addRange(CE->getSourceRange()); + Report->setDeclWithIssue(DeclWithIssue); + BR->emitReport(std::move(Report)); + } }; } // namespace diff --git a/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h index 9eb63b4745b21..3f075ca0a6e5b 100644 --- a/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h +++ b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h @@ -24,6 +24,9 @@ typedef struct CF_BRIDGED_MUTABLE_TYPE(CFRunLoopRef) __CFRunLoop * CFRunLoopRef; extern const CFAllocatorRef kCFAllocatorDefault; typedef struct _NSZone NSZone; + +CFTypeID CFGetTypeID(CFTypeRef cf); + CFTypeID CFGetTypeID(CFTypeRef cf); CFTypeID CFArrayGetTypeID(); CFMutableArrayRef CFArrayCreateMutable(CFAllocatorRef allocator, CFIndex capacity); @@ -281,12 +284,12 @@ template<typename T> inline RetainPtr<T> retainPtr(T* ptr) inline NSObject *bridge_cast(CFTypeRef object) { - return (__bridge NSObject *)object; + return (__bridge NSObject *)object; } inline CFTypeRef bridge_cast(NSObject *object) { - return (__bridge CFTypeRef)object; + return (__bridge CFTypeRef)object; } inline id bridge_id_cast(CFTypeRef object) @@ -386,35 +389,35 @@ template <typename> struct CFTypeTrait; template<typename T> T dynamic_cf_cast(CFTypeRef object) { - if (!object) - return nullptr; + if (!object) + return nullptr; - if (CFGetTypeID(object) != CFTypeTrait<T>::typeID()) - return nullptr; + if (CFGetTypeID(object) != CFTypeTrait<T>::typeID()) + return nullptr; - return static_cast<T>(const_cast<CF_BRIDGED_TYPE(id) void*>(object)); + return static_cast<T>(const_cast<CF_BRIDGED_TYPE(id) void*>(object)); } template<typename T> T checked_cf_cast(CFTypeRef object) { - if (!object) - return nullptr; + if (!object) + return nullptr; - if (CFGetTypeID(object) != CFTypeTrait<T>::typeID()) - WTFCrash(); + if (CFGetTypeID(object) != CFTypeTrait<T>::typeID()) + WTFCrash(); - return static_cast<T>(const_cast<CF_BRIDGED_TYPE(id) void*>(object)); + return static_cast<T>(const_cast<CF_BRIDGED_TYPE(id) void*>(object)); } template<typename T, typename U> RetainPtr<T> dynamic_cf_cast(RetainPtr<U>&& object) { - if (!object) - return nullptr; + if (!object) + return nullptr; - if (CFGetTypeID(object.get()) != CFTypeTrait<T>::typeID()) - return nullptr; + if (CFGetTypeID(object.get()) != CFTypeTrait<T>::typeID()) + return nullptr; - return adoptCF(static_cast<T>(const_cast<CF_BRIDGED_TYPE(id) void*>(object.leakRef()))); + return adoptCF(static_cast<T>(const_cast<CF_BRIDGED_TYPE(id) void*>(object.leakRef()))); } } // namespace WTF @@ -448,4 +451,4 @@ using WTF::is_objc; using WTF::checked_objc_cast; using WTF::dynamic_objc_cast; using WTF::checked_cf_cast; -using WTF::dynamic_cf_cast; \ No newline at end of file +using WTF::dynamic_cf_cast; diff --git a/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use-arc.mm b/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use-arc.mm index 63383d4f49642..45db0506ae5f2 100644 --- a/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use-arc.mm +++ b/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use-arc.mm @@ -3,6 +3,8 @@ #include "objc-mock-types.h" +CFTypeRef CFCopyArray(CFArrayRef); + void basic_correct() { auto ns1 = adoptNS([SomeObj alloc]); auto ns2 = adoptNS([[SomeObj alloc] init]); @@ -12,6 +14,7 @@ void basic_correct() { auto ns6 = retainPtr([ns3 next]); CFMutableArrayRef cf1 = adoptCF(CFArrayCreateMutable(kCFAllocatorDefault, 10)); auto cf2 = adoptCF(SecTaskCreateFromSelf(kCFAllocatorDefault)); + auto cf3 = adoptCF(checked_cf_cast<CFArrayRef>(CFCopyArray(cf1))); } CFMutableArrayRef provide_cf(); @@ -27,6 +30,8 @@ void basic_wrong() { // expected-warning@-1{{Incorrect use of adoptCF. The argument is +0 and results in an use-after-free [alpha.webkit.RetainPtrCtorAdoptChecker]}} RetainPtr<CFTypeRef> cf3 = SecTaskCreateFromSelf(kCFAllocatorDefault); // expected-warning@-1{{Incorrect use of RetainPtr constructor. The argument is +1 and results in a memory leak [alpha.webkit.RetainPtrCtorAdoptChecker]}} + CFCopyArray(cf1); + // expected-warning@-1{{The return value is +1 and results in a memory leak [alpha.webkit.RetainPtrCtorAdoptChecker]}} } RetainPtr<CVPixelBufferRef> cf_out_argument() { @@ -68,7 +73,7 @@ void adopt_retainptr() { class MemberInit { public: - MemberInit(CFMutableArrayRef array, NSString *str, CFRunLoopRef runLoop) + MemberInit(RetainPtr<CFMutableArrayRef>&& array, NSString *str, CFRunLoopRef runLoop) : m_array(array) , m_str(str) , m_runLoop(runLoop) @@ -80,7 +85,7 @@ void adopt_retainptr() { RetainPtr<CFRunLoopRef> m_runLoop; }; void create_member_init() { - MemberInit init { CFArrayCreateMutable(kCFAllocatorDefault, 10), @"hello", CFRunLoopGetCurrent() }; + MemberInit init { adoptCF(CFArrayCreateMutable(kCFAllocatorDefault, 10)), @"hello", CFRunLoopGetCurrent() }; } RetainPtr<CFStringRef> cfstr() { diff --git a/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use.mm b/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use.mm index e94b7511dee14..c07f79c9f9650 100644 --- a/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use.mm +++ b/clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use.mm @@ -3,6 +3,9 @@ #include "objc-mock-types.h" +CFTypeRef CFCopyArray(CFArrayRef); +void* CreateCopy(); + void basic_correct() { auto ns1 = adoptNS([SomeObj alloc]); auto ns2 = adoptNS([[SomeObj alloc] init]); @@ -12,6 +15,8 @@ void basic_correct() { auto ns6 = retainPtr([ns3 next]); CFMutableArrayRef cf1 = adoptCF(CFArrayCreateMutable(kCFAllocatorDefault, 10)); auto cf2 = adoptCF(SecTaskCreateFromSelf(kCFAllocatorDefault)); + auto cf3 = adoptCF(checked_cf_cast<CFArrayRef>(CFCopyArray(cf1))); + CreateCopy(); } CFMutableArrayRef provide_cf(); @@ -27,6 +32,8 @@ void basic_wrong() { // expected-warning@-1{{Incorrect use of adoptCF. The argument is +0 and results in an use-after-free [alpha.webkit.RetainPtrCtorAdoptChecker]}} RetainPtr<CFTypeRef> cf3 = SecTaskCreateFromSelf(kCFAllocatorDefault); // expected-warning@-1{{Incorrect use of RetainPtr constructor. The argument is +1 and results in a memory leak [alpha.webkit.RetainPtrCtorAdoptChecker]}} + CFCopyArray(cf1); + // expected-warning@-1{{The return value is +1 and results in a memory leak [alpha.webkit.RetainPtrCtorAdoptChecker]}} } RetainPtr<CVPixelBufferRef> cf_out_argument() { @@ -68,7 +75,7 @@ void adopt_retainptr() { class MemberInit { public: - MemberInit(CFMutableArrayRef array, NSString *str, CFRunLoopRef runLoop) + MemberInit(RetainPtr<CFMutableArrayRef>&& array, NSString *str, CFRunLoopRef runLoop) : m_array(array) , m_str(str) , m_runLoop(runLoop) @@ -80,7 +87,7 @@ void adopt_retainptr() { RetainPtr<CFRunLoopRef> m_runLoop; }; void create_member_init() { - MemberInit init { CFArrayCreateMutable(kCFAllocatorDefault, 10), @"hello", CFRunLoopGetCurrent() }; + MemberInit init { adoptCF(CFArrayCreateMutable(kCFAllocatorDefault, 10)), @"hello", CFRunLoopGetCurrent() }; } RetainPtr<CFStringRef> cfstr() { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
