llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Ryosuke Niwa (rniwa) <details> <summary>Changes</summary> Split alpha.webkit.UncheckedLambdaCapturesChecker off of UncountedLambdaCapturesChecker, which currently checks for the use of ref counted objects as well as CheckedPtr capable types unlike all other WebKit checkers, which only check for one type of smart pointers. Also improve the wording of the warning text so that it can be easily extended to support checking for the lambda capturing of raw pointers and references to CheckedPtr/CheckedRef. --- Patch is 56.57 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/201044.diff 9 Files Affected: - (modified) clang/docs/analyzer/checkers.rst (+17) - (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+4) - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp (+100-30) - (modified) clang/test/Analysis/Checkers/WebKit/mock-types.h (+1) - (added) clang/test/Analysis/Checkers/WebKit/unchecked-lambda-captures.cpp (+713) - (modified) clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures-decl-protects-this-crash.cpp (+2-2) - (modified) clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp (+25-36) - (modified) clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures-arc.mm (+7-7) - (modified) clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures.mm (+25-24) ``````````diff diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index 61f591916018e..09b49c779b451 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -3940,6 +3940,23 @@ The goal of this rule is to make sure that lifetime of any dynamically allocated The rules of when to use and not to use CheckedPtr / CheckedRef are same as alpha.webkit.UncountedCallArgsChecker for ref-counted objects. +alpha.webkit.UncheckedLambdaCapturesChecker +""""""""""""""""""""""""""""""""""""""""""" +Raw pointers and references to unchecked types can't be captured in lambdas. Only CheckedPtr or CheckedRef is allowed. + +.. code-block:: cpp + + struct CheckedObject { + void incrementCheckedPtr() {} + void decrementCheckedPtr() {} + }; + + void foo(CheckedObject* a, CheckedObject& b) { + [&, a](){ // warn about 'a' + do_something(b); // warn about 'b' + }; + }; + alpha.webkit.UnretainedCallArgsChecker """""""""""""""""""""""""""""""""""""" The goal of this rule is to make sure that lifetime of any dynamically allocated NS or CF objects passed as a call argument keeps its memory region past the end of the call. This applies to call to any function, method, lambda, function pointer or functor. NS or CF objects aren't supposed to be allocated on stack so we check arguments for parameters of raw pointers and references to unretained types. diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index eca2afbe340a9..27415781193e7 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1755,6 +1755,10 @@ def UncountedLocalVarsChecker : Checker<"UncountedLocalVarsChecker">, HelpText<"Check uncounted local variables.">, Documentation<HasDocumentation>; +def UncheckedLambdaCapturesChecker : Checker<"UncheckedLambdaCapturesChecker">, + HelpText<"Check unchecked lambda captures.">, + Documentation<HasDocumentation>; + def UncheckedLocalVarsChecker : Checker<"UncheckedLocalVarsChecker">, HelpText<"Check unchecked local variables.">, Documentation<HasDocumentation>; diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp index 287c7dd039dd8..4d0381a82f250 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp @@ -1,4 +1,4 @@ -//=======- UncountedLambdaCapturesChecker.cpp --------------------*- C++ -*-==// +//=======- RawPtrRefLambdaCapturesChecker.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. @@ -36,7 +36,7 @@ class RawPtrRefLambdaCapturesChecker virtual std::optional<bool> isUnsafePtr(QualType) const = 0; virtual bool isPtrType(const std::string &) const = 0; - virtual const char *ptrKind(QualType QT) const = 0; + virtual const char *typeName() const = 0; void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR, BugReporter &BRArg) const { @@ -542,25 +542,40 @@ class RawPtrRefLambdaCapturesChecker SmallString<100> Buf; llvm::raw_svector_ostream Os(Buf); - if (Capture.isExplicit()) { + if (Capture.isExplicit()) Os << "Captured "; - } else { + else Os << "Implicitly captured "; - } - if (isa<PointerType>(T) || isa<ObjCObjectPointerType>(T)) { - Os << "raw-pointer "; - } else { - Os << "reference "; - } - + Os << "variable "; printQuotedQualifiedName(Os, CapturedVar); - Os << " to " << ptrKind(T) << " type is unsafe."; + + bool IsUnsafePtr = CapturedVar->getType() == T; + if (IsUnsafePtr) + Os << " is a "; + else + Os << " contains a "; + auto *CapturedType = T.getTypePtrOrNull(); + printPointer(Os, CapturedType); PathDiagnosticLocation BSLoc(Location, BR->getSourceManager()); auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc); BR->emitReport(std::move(Report)); } + ObjCInterfaceDecl *getObjCDecl(const Type *TypePtr) const { + auto *PointeeType = TypePtr->getPointeeType().getTypePtrOrNull(); + if (!PointeeType) + return nullptr; + auto *Desugared = PointeeType->getUnqualifiedDesugaredType(); + if (!Desugared) + return nullptr; + if (auto *ObjCType = dyn_cast<ObjCInterfaceType>(Desugared)) + return ObjCType->getDecl(); + if (auto *ObjCType = dyn_cast<ObjCObjectType>(Desugared)) + return ObjCType->getInterface(); + return nullptr; + } + void reportBugOnThisPtr(const LambdaCapture &Capture, const QualType T) const { SmallString<100> Buf; @@ -572,41 +587,64 @@ class RawPtrRefLambdaCapturesChecker Os << "Implicitly captured "; } - Os << "raw-pointer 'this' to " << ptrKind(T) << " type is unsafe."; + Os << "variable 'this' is a raw pointer to " << typeName(); + if (auto *RD = T->getPointeeCXXRecordDecl()) { + Os << " "; + printQuotedQualifiedName(Os, RD); + } PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager()); auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc); BR->emitReport(std::move(Report)); } + + virtual void 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" : "raw reference") << " to "; + Os << typeName(); + + if (auto *RD = T->getPointeeType()->getAsRecordDecl()) { + Os << " "; + printQuotedQualifiedName(Os, RD); + } else if (auto *ObjCDecl = getObjCDecl(T)) { + Os << " "; + printQuotedQualifiedName(Os, ObjCDecl); + } + } }; class UncountedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker { public: UncountedLambdaCapturesChecker() - : RawPtrRefLambdaCapturesChecker("Lambda capture of uncounted or " - "unchecked variable") {} + : RawPtrRefLambdaCapturesChecker("Lambda capture of uncounted variable") {} std::optional<bool> isUnsafePtr(QualType QT) const final { - auto result1 = isUncountedPtr(QT); - auto result2 = isUncheckedPtr(QT); - if (result1 && *result1) - return true; - if (result2 && *result2) - return true; - if (result1) - return *result1; - return result2; + return isUncountedPtr(QT.getCanonicalType()); } virtual bool isPtrType(const std::string &Name) const final { - return isRefType(Name) || isCheckedPtr(Name); + return isRefType(Name); + } + + const char *typeName() const final { return "ref-countable type"; } +}; + +class UncheckedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker { +public: + UncheckedLambdaCapturesChecker() + : RawPtrRefLambdaCapturesChecker("Lambda capture of unchecked variable") {} + + std::optional<bool> isUnsafePtr(QualType QT) const final { + return isUncheckedPtr(QT.getCanonicalType()); } - const char *ptrKind(QualType QT) const final { - if (isUncounted(QT)) - return "uncounted"; - return "unchecked"; + virtual bool isPtrType(const std::string &Name) const final { + return isCheckedPtr(Name); } + + const char *typeName() const final { return "CheckedPtr capable type"; } }; class UnretainedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker { @@ -627,7 +665,30 @@ class UnretainedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker { return isRetainPtrOrOSPtr(Name); } - const char *ptrKind(QualType QT) const final { return "unretained"; } + const char *typeName() const final { return "retainable type"; } + + void printPointer(llvm::raw_svector_ostream &Os, const Type *T) const final { + if (auto *ObjCPtr = dyn_cast<ObjCObjectPointerType>(T)) { + for (ObjCProtocolDecl *P : ObjCPtr->quals()) { + if (const auto *II = P->getIdentifier()) { + auto Name = II->getName(); + if (Name.starts_with("OS_")) { + Os << typeName() << " "; + printQuotedQualifiedName(Os, P); + return; + } + } + } + } + if (!isa<ObjCObjectPointerType>(T) && T->getAs<TypedefType>()) { + auto Typedef = T->getAs<TypedefType>(); + assert(Typedef); + Os << typeName() << " "; + printQuotedQualifiedName(Os, Typedef->getDecl()); + return; + } + return RawPtrRefLambdaCapturesChecker::printPointer(Os, T); + } }; } // namespace @@ -641,6 +702,15 @@ bool ento::shouldRegisterUncountedLambdaCapturesChecker( return true; } +void ento::registerUncheckedLambdaCapturesChecker(CheckerManager &Mgr) { + Mgr.registerChecker<UncheckedLambdaCapturesChecker>(); +} + +bool ento::shouldRegisterUncheckedLambdaCapturesChecker( + const CheckerManager &mgr) { + return true; +} + void ento::registerUnretainedLambdaCapturesChecker(CheckerManager &Mgr) { Mgr.registerChecker<UnretainedLambdaCapturesChecker>(); } diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h index af63268ac9695..2861ba9c090a0 100644 --- a/clang/test/Analysis/Checkers/WebKit/mock-types.h +++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h @@ -307,6 +307,7 @@ class CheckedObj { void incrementCheckedPtrCount() { ++m_ptrCount; } void decrementCheckedPtrCount() { --m_ptrCount; } void method(); + void constMethod() const; int trivial() { return 123; } CheckedObj* next(); diff --git a/clang/test/Analysis/Checkers/WebKit/unchecked-lambda-captures.cpp b/clang/test/Analysis/Checkers/WebKit/unchecked-lambda-captures.cpp new file mode 100644 index 0000000000000..c63e44b4e51cf --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/unchecked-lambda-captures.cpp @@ -0,0 +1,713 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncheckedLambdaCapturesChecker -verify %s + +#include "mock-types.h" + +namespace std { + +template <typename T> +T&& move(T& t) { + return static_cast<T&&>(t); +} + +namespace ranges { + +template<typename IteratorType, typename CallbackType> +void for_each(IteratorType first, IteratorType last, CallbackType callback) { + for (auto it = first; !(it == last); ++it) + callback(*it); +} + +struct all_of_impl { + template <typename Collection, typename Predicate> + constexpr bool operator()(const Collection& collection, Predicate predicate) const { + for (auto it = collection.begin(); it != collection.end(); ++it) { + if (!predicate(*it)) + return false; + } + return true; + } +}; +inline constexpr auto all_of = all_of_impl {}; + +} + +} + +namespace WTF { + +namespace Detail { + +template<typename Out, typename... In> +class CallableWrapperBase { +public: + virtual ~CallableWrapperBase() { } + virtual Out call(In...) = 0; +}; + +template<typename, typename, typename...> class CallableWrapper; + +template<typename CallableType, typename Out, typename... In> +class CallableWrapper : public CallableWrapperBase<Out, In...> { +public: + explicit CallableWrapper(CallableType& callable) + : m_callable(callable) { } + Out call(In... in) final { return m_callable(in...); } + +private: + CallableType m_callable; +}; + +} // namespace Detail + +template<typename> class Function; + +template<typename Out, typename... In> Function<Out(In...)> adopt(Detail::CallableWrapperBase<Out, In...>*); + +template <typename Out, typename... In> +class Function<Out(In...)> { +public: + using Impl = Detail::CallableWrapperBase<Out, In...>; + + Function() = default; + + template<typename FunctionType> + Function(FunctionType f) + : m_callableWrapper(new Detail::CallableWrapper<FunctionType, Out, In...>(f)) { } + + Out operator()(In... in) const { return m_callableWrapper->call(in...); } + explicit operator bool() const { return !!m_callableWrapper; } + +private: + enum AdoptTag { Adopt }; + Function(Impl* impl, AdoptTag) + : m_callableWrapper(impl) + { + } + + friend Function adopt<Out, In...>(Impl*); + + std::unique_ptr<Impl> m_callableWrapper; +}; + +template<typename Out, typename... In> Function<Out(In...)> adopt(Detail::CallableWrapperBase<Out, In...>* impl) +{ + return Function<Out(In...)>(impl, Function<Out(In...)>::Adopt); +} + +template <typename KeyType, typename ValueType> +class HashMap { +public: + HashMap(); + HashMap([[clang::noescape]] const Function<ValueType()>&); + void ensure(const KeyType&, [[clang::noescape]] const Function<ValueType()>&); + bool operator+([[clang::noescape]] const Function<ValueType()>&) const; + static void ifAny(HashMap, [[clang::noescape]] const Function<bool(ValueType)>&); + +private: + ValueType* m_table { nullptr }; +}; + +class ScopeExit final { +public: + template<typename ExitFunctionParameter> + explicit ScopeExit(ExitFunctionParameter&& exitFunction) + : m_exitFunction(std::move(exitFunction)) { + } + + ScopeExit(ScopeExit&& other) + : m_exitFunction(std::move(other.m_exitFunction)) + , m_execute(std::move(other.m_execute)) { + } + + ~ScopeExit() { + if (m_execute) + m_exitFunction(); + } + + WTF::Function<void()> take() { + m_execute = false; + return std::move(m_exitFunction); + } + + void release() { m_execute = false; } + + ScopeExit(const ScopeExit&) = delete; + ScopeExit& operator=(const ScopeExit&) = delete; + ScopeExit& operator=(ScopeExit&&) = delete; + +private: + WTF::Function<void()> m_exitFunction; + bool m_execute { true }; +}; + +template<typename ExitFunction> ScopeExit makeScopeExit(ExitFunction&&); +template<typename ExitFunction> +ScopeExit makeScopeExit(ExitFunction&& exitFunction) +{ + return ScopeExit(std::move(exitFunction)); +} + +// Visitor adapted from http://stackoverflow.com/questions/25338795/is-there-a-name-for-this-tuple-creation-idiom + +template<class A, class... B> struct Visitor : Visitor<A>, Visitor<B...> { + Visitor(A a, B... b) + : Visitor<A>(a) + , Visitor<B...>(b...) + { + } + + using Visitor<A>::operator (); + using Visitor<B...>::operator (); +}; + +template<class A> struct Visitor<A> : A { + Visitor(A a) + : A(a) + { + } + + using A::operator(); +}; + +template<class... F> Visitor<F...> makeVisitor(F... f) +{ + return Visitor<F...>(f...); +} + +void opaqueFunction(); +template <typename Visitor, typename... Variants> void visit(Visitor&& v, Variants&&... values) +{ + opaqueFunction(); +} + +} // namespace WTF + +struct A { + static void b(); +}; + +CheckedObj* make_obj(); + +void someFunction(); +template <typename Callback> void call(Callback callback) { + someFunction(); + callback(); +} +void callAsync(const WTF::Function<void()>&); + +void raw_ptr() { + CheckedObj* checked_ptr_capable = make_obj(); + auto foo1 = [checked_ptr_capable](){ + // expected-warning@-1{{Captured variable 'checked_ptr_capable' is a raw pointer to CheckedPtr capable type 'CheckedObj' [alpha.webkit.UncheckedLambdaCapturesChecker]}} + checked_ptr_capable->method(); + }; + auto foo2 = [&checked_ptr_capable](){ + // expected-warning@-1{{Captured variable 'checked_ptr_capable' is a raw pointer to CheckedPtr capable type 'CheckedObj' [alpha.webkit.UncheckedLambdaCapturesChecker]}} + checked_ptr_capable->method(); + }; + auto foo3 = [&](){ + checked_ptr_capable->method(); + // expected-warning@-1{{Implicitly captured variable 'checked_ptr_capable' is a raw pointer to CheckedPtr capable type 'CheckedObj' [alpha.webkit.UncheckedLambdaCapturesChecker]}} + checked_ptr_capable = nullptr; + }; + + auto foo4 = [=](){ + checked_ptr_capable->method(); + // expected-warning@-1{{Implicitly captured variable 'checked_ptr_capable' is a raw pointer to CheckedPtr capable type 'CheckedObj' [alpha.webkit.UncheckedLambdaCapturesChecker]}} + }; + + call(foo1); + call(foo2); + call(foo3); + call(foo4); + + // Confirm that the checker respects [[clang::suppress]]. + CheckedObj* suppressed_checked_ptr_capable = nullptr; + [[clang::suppress]] auto foo5 = [suppressed_checked_ptr_capable](){}; + // no warning. + call(foo5); +} + +void references() { + CheckedObj automatic; + CheckedObj& checked_ptr_capable_ref = automatic; + auto foo1 = [checked_ptr_capable_ref](){ checked_ptr_capable_ref.constMethod(); }; + auto foo2 = [&checked_ptr_capable_ref](){ checked_ptr_capable_ref.method(); }; + // expected-warning@-1{{Captured variable 'checked_ptr_capable_ref' is a raw reference to CheckedPtr capable type 'CheckedObj' [alpha.webkit.UncheckedLambdaCapturesChecker]}} + auto foo3 = [&](){ checked_ptr_capable_ref.method(); }; + // expected-warning@-1{{Implicitly captured variable 'checked_ptr_capable_ref' is a raw reference to CheckedPtr capable type 'CheckedObj' [alpha.webkit.UncheckedLambdaCapturesChecker]}} + auto foo4 = [=](){ checked_ptr_capable_ref.constMethod(); }; + + call(foo1); + call(foo2); + call(foo3); + call(foo4); +} + +void quiet() { +// This code is not expected to trigger any warnings. + { + CheckedObj automatic; + CheckedObj &checked_ptr_capable_ref = automatic; + } + + auto foo3 = [&]() {}; + auto foo4 = [=]() {}; + + call(foo3); + call(foo4); + + CheckedObj *checked_ptr_capable = nullptr; +} + +template <typename Callback> +void map(CheckedObj* start, [[clang::noescape]] Callback&& callback) +{ + while (start) { + callback(*start); + start = start->next(); + } +} + +template <typename Callback1, typename Callback2> +void doubleMap(CheckedObj* start, [[clang::noescape]] Callback1&& callback1, Callback2&& callback2) +{ + while (start) { + callback1(*start); + callback2(*start); + start = start->next(); + } +} + +void noescape_lambda() { + CheckedObj* someObj = make_obj(); + CheckedObj* otherObj = make_obj(); + map(make_obj(), [&](CheckedObj& obj) { + otherObj->method(); + }); + doubleMap(make_obj(), [&](CheckedObj& obj) { + otherObj->method(); + }, [&](CheckedObj& obj) { + otherObj->method(); + // expected-warning@-1{{Implicitly captured variable 'otherObj' is a raw pointer to CheckedPtr capable type 'CheckedObj' [alpha.webkit.UncheckedLambdaCapturesChecker]}} + }); + ([&] { + someObj->method(); + })(); +} + +void lambda_capture_param(CheckedObj* obj) { + auto someLambda = [&]() { + obj->method(); + }; + someLambda(); + someLambda(); +} + +struct CheckedObjWithLambdaCapturingThis { + void incrementCheckedPtrCount() const; + void decrementCheckedPtrCount() const; + void nonTrivial(); + + void method_captures_this_safe() { + auto lambda = [&]() { + nonTrivial(); + }; + lambda(); + } + + void method_captures_this_unsafe() { + auto lambda = [&]() { + nonTrivial(); + // expected-warning@-1{{Implicitly captured variable 'this' is a raw pointer to CheckedPtr capable type 'CheckedObjWithLambdaCapturingThis' [alpha.webkit.UncheckedLambdaCapturesChecker]}} + }; + call(lambda); + } + + void method_captures_this_unsafe_capture_local_var_explicitly() { + CheckedObj* x = make_obj(); + call([this, protectedThis = CheckedPtr { this }, x]() { + // expected-warning@-1{{Captured variable 'x' is a raw pointer to CheckedPtr capable type 'CheckedObj' [alpha.webkit.UncheckedLambdaCapturesChecker]}} + nonTrivial(); + x->method(); + }); + } + + void method_captures_this_with_other_protected_var() { + CheckedObj* x = make_obj(); + call([this, protectedX = CheckedPtr { x }]() { + // expected-warning@-1{{Captured variable 'this' is a raw pointer to CheckedPtr capable type 'CheckedObjWithLambdaCapturingThis' [alpha.webkit.UncheckedLambdaCapturesChecker]}} + nonTrivial(); + protectedX->method(); + }); + } + + void method_captures_this_unsafe_capture_local_var_explicitly_with_deref() { + CheckedObj* x = make_obj(); + call([this, protectedThis = CheckedRef { *this }, x]() { + // expected-warning@-1{{Captured variable 'x' is a raw pointer to CheckedPtr capable type 'CheckedObj' [alpha.webkit.UncheckedLambdaCapturesChecker]}} + nonTrivial(); + x->method... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/201044 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
