llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Ryosuke Niwa (rniwa) <details> <summary>Changes</summary> This PR fixes a bug in UncountedCallArgsChecker that it would not emit a warning when a function is called with a WeakPtr local variable as an argument. We normally don't generate a warning for a local variable passed to a function argument in UncountedCallArgsChecker as the variable may have a guardian in an outer scope but only UncountedLocalVarsChecker is capable of locating one. So rather than generating a warning in UncountedCallArgsChecker directly, we rely on UncountedLocalVarsChecker to generate a warning for the local variable. This all falls apart in the case of a WeakPtr local variable because a WeakPtr is explicitly allowed as a local variable by UncountedLocalVarsChecker. So, this PR fixes the bug by detecting this exact scenario (a WeakPtr local variable used as a function argument), and generate a warning directly in UncountedCallArgsChecker. Also added the support for recognizing more weak smart pointer types in WebKit. --- Full diff: https://github.com/llvm/llvm-project/pull/184563.diff 5 Files Affected: - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp (+14-4) - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp (+15-2) - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h (+5-1) - (modified) clang/test/Analysis/Checkers/WebKit/call-args.cpp (+18) - (modified) clang/test/Analysis/Checkers/WebKit/mock-types.h (+3) ``````````diff diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp index 4a9a3be2b6614..f2ee9c742f4b8 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp @@ -239,10 +239,19 @@ bool tryToFindPtrOrigin( bool isASafeCallArg(const Expr *E) { assert(E); + auto IsCheckedLocalVarOrParam = [](const VarDecl *Decl) { + if (auto *Type = Decl->getType().getTypePtrOrNull()) { + if (auto *CXXRD = Type->getAsCXXRecordDecl()) { + if (isWeakPtr(CXXRD)) + return false; + } + } + return Decl->isLocalVarDeclOrParm(); + }; if (auto *Ref = dyn_cast<DeclRefExpr>(E)) { auto *FoundDecl = Ref->getFoundDecl(); if (auto *D = dyn_cast_or_null<VarDecl>(FoundDecl)) { - if (isa<ParmVarDecl>(D) || D->isLocalVarDecl()) + if (IsCheckedLocalVarOrParam(D)) return true; if (auto *ImplicitP = dyn_cast<ImplicitParamDecl>(D)) { auto Kind = ImplicitP->getParameterKind(); @@ -253,9 +262,10 @@ bool isASafeCallArg(const Expr *E) { return true; } } else if (auto *BD = dyn_cast_or_null<BindingDecl>(FoundDecl)) { - VarDecl *VD = BD->getHoldingVar(); - if (VD && (isa<ParmVarDecl>(VD) || VD->isLocalVarDecl())) - return true; + if (VarDecl *VD = BD->getHoldingVar()) { + if (IsCheckedLocalVarOrParam(VD)) + return true; + } } } if (isa<CXXTemporaryObjectExpr>(E)) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 7a7e22ead74a8..a95bf6faaab52 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -143,11 +143,17 @@ bool isOwnerPtr(const std::string &Name) { Name == "UniqueRef" || Name == "LazyUniqueRef"; } +static bool isWeakPtrClass(const std::string &Name) { + return Name == "WeakPtr" || Name == "SingleThreadPackedWeakPtr" || + Name == "SingleThreadWeakPtr" || Name == "ThreadSafeWeakPtr" || + Name == "ThreadSafeWeakOrStrongPtr" || Name == "InlineWeakPtr"; +} + bool isSmartPtrClass(const std::string &Name) { return isRefType(Name) || isCheckedPtr(Name) || isRetainPtrOrOSPtr(Name) || - Name == "WeakPtr" || Name == "WeakPtrFactory" || + isWeakPtrClass(Name) || Name == "WeakPtrFactory" || Name == "WeakPtrFactoryWithBitField" || Name == "WeakPtrImplBase" || - Name == "WeakPtrImplBaseSingleThread" || Name == "ThreadSafeWeakPtr" || + Name == "WeakPtrImplBaseSingleThread" || Name == "ThreadSafeWeakOrStrongPtr" || Name == "ThreadSafeWeakPtrControlBlock" || Name == "ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr"; @@ -399,6 +405,13 @@ bool isRetainPtrOrOSPtr(const CXXRecordDecl *R) { return false; } +bool isWeakPtr(const CXXRecordDecl *R) { + assert(R); + if (auto *TmplR = R->getTemplateInstantiationPattern()) + return isWeakPtrClass(safeGetName(TmplR)); + return false; +} + bool isSmartPtr(const CXXRecordDecl *R) { assert(R); if (auto *TmplR = R->getTemplateInstantiationPattern()) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h index bcb4eee16bd2c..d26a18c456c77 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -60,6 +60,10 @@ bool isCheckedPtr(const clang::CXXRecordDecl *Class); /// \returns true if \p Class is a RetainPtr, false if not. bool isRetainPtrOrOSPtr(const clang::CXXRecordDecl *Class); +/// \returns true if \p Class is a weak smart pointer (WeakPtr, InlineWeakPtr, +/// etc...), false if not. +bool isWeakPtr(const clang::CXXRecordDecl *Class); + /// \returns true if \p Class is a smart pointer (RefPtr, WeakPtr, etc...), /// false if not. bool isSmartPtr(const clang::CXXRecordDecl *Class); @@ -139,7 +143,7 @@ bool isCheckedPtr(const std::string &Name); /// \returns true if \p Name is RetainPtr or its variant, false if not. bool isRetainPtrOrOSPtr(const std::string &Name); -/// \returns true if \p Name is an owning smar pointer such as Ref, CheckedPtr, +/// \returns true if \p Name is an owning smart pointer such as Ref, CheckedPtr, /// and unique_ptr. bool isOwnerPtr(const std::string &Name); diff --git a/clang/test/Analysis/Checkers/WebKit/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp index 1a8bde29080ac..c74e7ab7b9d7e 100644 --- a/clang/test/Analysis/Checkers/WebKit/call-args.cpp +++ b/clang/test/Analysis/Checkers/WebKit/call-args.cpp @@ -440,3 +440,21 @@ namespace call_with_adopt_ref { adoptRef(new Obj)->method(); } } + +namespace call_with_weak_ptr { + + class RefCountableWithWeakPtr : public RefCountable, public CanMakeWeakPtr<RefCountableWithWeakPtr> { + }; + + RefCountableWithWeakPtr* provide(); + void consume(RefCountableWithWeakPtr*); + + void foo() { + WeakPtr weakPtr = provide(); + consume(weakPtr); + // expected-warning@-1{{Call argument is uncounted and unsafe}} + weakPtr->method(); + // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} + } + +} diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h index c139a5cb13de7..a5d9ed5ae174d 100644 --- a/clang/test/Analysis/Checkers/WebKit/mock-types.h +++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h @@ -415,6 +415,9 @@ class WeakPtr { return *this; } + T* operator->() { return get(); } + operator T*() { return get(); } + T* get() { return impl ? impl->get<T>() : nullptr; } `````````` </details> https://github.com/llvm/llvm-project/pull/184563 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
