https://github.com/geoffreygaren created https://github.com/llvm/llvm-project/pull/226288
Like alpha.webkit.UnborrowedLocalVarsChecker, but for lambda captures. This checker is pretty restrictive because, unlike a refcounted object, a lifetime-dependent pointer/reference/view cannot be captured in an escaping closure at all. Still, we permit capturing in a NOESCAPE closure. >From d4fb481af62a59239fa0dd42ab0af701222cf952 Mon Sep 17 00:00:00 2001 From: Geoff Garen <[email protected]> Date: Tue, 25 Aug 2026 10:32:46 -0700 Subject: [PATCH] [WebKit Checkers] Add alpha.webkit.UnborrowedLambdaCapturesChecker Like alpha.webkit.UnborrowedLocalVarsChecker, but for lambda captures. This checker is pretty restrictive because, unlike a refcounted object, a lifetime-dependent pointer/reference/view cannot be captured in an escaping closure at all. Still, we permit capturing in a NOESCAPE closure. --- clang/docs/analyzer/checkers.md | 34 ++++ .../clang/StaticAnalyzer/Checkers/Checkers.td | 4 + .../WebKit/RawPtrRefLambdaCapturesChecker.cpp | 88 +++++++++- .../Checkers/WebKit/RawPtrRefSafetyModel.cpp | 10 +- .../Analysis/Checkers/WebKit/mock-canborrow.h | 32 ++++ .../WebKit/unborrowed-lambda-captures.cpp | 166 ++++++++++++++++++ 6 files changed, 325 insertions(+), 9 deletions(-) create mode 100644 clang/test/Analysis/Checkers/WebKit/unborrowed-lambda-captures.cpp diff --git a/clang/docs/analyzer/checkers.md b/clang/docs/analyzer/checkers.md index 343b9482f8a62..f6b6aa3212c12 100644 --- a/clang/docs/analyzer/checkers.md +++ b/clang/docs/analyzer/checkers.md @@ -4330,6 +4330,40 @@ These examples do not warn: > } > ``` +#### alpha.webkit.UnborrowedLambdaCapturesChecker + +The same rule as alpha.webkit.UnborrowedLocalVarsChecker, applied to lambda captures. + +Note: It is impossible for an escaping closure to capture a Borrow since Borrow is stack-only. + +> ```cpp +> void takesCallback(const Function<void()>&); +> void takesNoEscapeCallback([[clang::noescape]] const Function<void()>&); +> +> void foo1(Vector<char>& buffer) { +> takesCallback([data = buffer.data()] { use(data); }); // warn +> +> Borrow<Vector<char>> borrowed(buffer); +> takesCallback([data = borrowed.get().data()] { use(data); }); // warn +> takesCallback([&borrowed] { use(borrowed.get().data()); }); // warn +> } +> ``` + +A NOESCAPE callee runs the lambda before returning, so a `Borrow` in the enclosing scope protects the capture: + +> ```cpp +> void foo2(Vector<char>& buffer) { +> Borrow<Vector<char>> borrowed(buffer); +> takesNoEscapeCallback([data = borrowed.get().data()] { use(data); }); // ok +> takesNoEscapeCallback([&borrowed] { use(borrowed.get().data()); }); // ok +> +> takesNoEscapeCallback([&buffer] { +> Borrow<Vector<char>> b(buffer); +> use(b.get().data()); // ok +> }); +> } +> ``` + #### webkit.RetainPtrCtorAdoptChecker The goal of this rule is to make sure the constructors of RetainPtr and OSObjectPtr as well as adoptNS, adoptCF, and adoptOSObject are used correctly. diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 3d2428bdf92a5..e1a1046efab43 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1810,6 +1810,10 @@ def UnborrowedLocalVarsChecker : Checker<"UnborrowedLocalVarsChecker">, HelpText<"Check local variables holding a loan on a CanBorrow object that is not guarded by a Borrow.">, Documentation<HasDocumentation>; +def UnborrowedLambdaCapturesChecker : Checker<"UnborrowedLambdaCapturesChecker">, + HelpText<"Check lambda captures holding a loan on a CanBorrow object that is not guarded by a Borrow.">, + Documentation<HasDocumentation>; + def RetainPtrCtorAdoptChecker : Checker<"RetainPtrCtorAdoptChecker">, HelpText<"Check for correct use of RetainPtr/OSObjectPtr constructor, adoptNS, adoptCF, and adoptOSObject">, Documentation<HasDocumentation>; diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp index 3fc4c38c8bed7..93c1d8224a36d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp @@ -551,9 +551,16 @@ class RawPtrRefLambdaCapturesChecker bool ignoreParamVarDecl = false) const { if (BR->getSourceManager().isInSystemHeader(L->getBeginLoc())) return; + // FIXME: This check is unsound. Destruction can happen in three places, + // and a capture is only safe when all three are trivial: the lambda's + // body, the callee that receives it, and the scope that creates it. if (TFA.isTrivial(L->getBody())) return; + unsigned Index = 0; for (const LambdaCapture &C : L->captures()) { + const Expr *CaptureInit = + Index < L->capture_size() ? L->capture_init_begin()[Index] : nullptr; + ++Index; if (C.capturesVariable()) { ValueDecl *CapturedVar = C.getCapturedVar(); if (ignoreParamVarDecl && isa<ParmVarDecl>(CapturedVar)) @@ -566,22 +573,70 @@ class RawPtrRefLambdaCapturesChecker continue; } QualType CapturedVarQualType = CapturedVar->getType(); - auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType()); + auto IsUncountedPtr = isUnsafePtr(CapturedVarQualType); if (C.getCaptureKind() == LCK_ByCopy && CapturedVarQualType->isReferenceType()) continue; - if (IsUncountedPtr && *IsUncountedPtr) - reportBug(C, CapturedVar, CapturedVarQualType, L); + if (!IsUncountedPtr || !*IsUncountedPtr) + continue; + const Expr *Origin = nullptr; + if (Model->checksForInteriorDestruction()) { + if (!CaptureInit) + continue; + if (isCaptureOriginSafeForInteriorDestruction(CaptureInit, Origin)) + continue; + } + reportBug(C, CapturedVar, CapturedVarQualType, L, Origin); } else if (C.capturesThis() && shouldCheckThis) { - if (ignoreParamVarDecl) // this is always a parameter to this function. + if (ignoreParamVarDecl) + continue; + if (Model->checksForInteriorDestruction()) continue; reportBugOnThisPtr(C, T); } } } + bool isCaptureOriginSafeForInteriorDestruction(const Expr *CaptureInit, + const Expr *&Origin) const { + return tryToFindPtrOrigin( + CaptureInit, /*StopAtFirstRefCountedObj=*/false, + Model->checksForInteriorDestruction(), + [&](const clang::CXXRecordDecl *Record) { + return Model->isSafePtr(Record); + }, + [&](const clang::QualType Type) { return Model->isSafePtrType(Type); }, + [&](const clang::Decl *D) { + return Model->isSafeDecl(D, BR->getSourceManager()); + }, + [&](const clang::Expr *CaptureOrigin, bool IsSafe, + bool /*OriginDependsOnFullExpressionTemporary*/, + bool PtrIsLifetimeBoundToOrigin) { + if (!CaptureOrigin) + return true; + if (isa<CXXThisExpr>(CaptureOrigin)) + return true; + // A Borrow in the enclosing scope does not travel with the lambda, + // so a loan taken through it is unguarded once the lambda escapes. + QualType OriginType = pointeeType(CaptureOrigin->getType()); + if (!OriginType.isNull() && isBorrowType(OriginType)) { + if (!Origin) + Origin = CaptureOrigin; + return false; + } + if (IsSafe) + return true; + if (Model->isSafeExpr(CaptureOrigin, PtrIsLifetimeBoundToOrigin)) + return true; + if (!Origin) + Origin = CaptureOrigin; + return false; + }); + } + void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar, - const QualType T, const LambdaExpr *L) const { + const QualType T, const LambdaExpr *L, + const Expr *Origin) const { assert(CapturedVar); auto Location = Capture.getLocation(); @@ -603,8 +658,10 @@ class RawPtrRefLambdaCapturesChecker Os << " is a "; else Os << " contains a "; - auto *CapturedType = T.getTypePtrOrNull(); - printPointer(Os, CapturedType); + if (Model->checksForInteriorDestruction()) + Model->describeHazard(Os, Origin, T); + else + printPointer(Os, T.getTypePtrOrNull()); PathDiagnosticLocation BSLoc(Location, BR->getSourceManager()); auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc); @@ -696,6 +753,14 @@ class UnretainedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker { makeRetainPtrSafetyModel()) {} }; +class UnborrowedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker { +public: + UnborrowedLambdaCapturesChecker() + : RawPtrRefLambdaCapturesChecker("Lambda capture of a loan on a " + "CanBorrow object", + makeBorrowSafetyModel()) {} +}; + } // namespace void ento::registerUncountedLambdaCapturesChecker(CheckerManager &Mgr) { @@ -724,3 +789,12 @@ bool ento::shouldRegisterUnretainedLambdaCapturesChecker( const CheckerManager &mgr) { return true; } + +void ento::registerUnborrowedLambdaCapturesChecker(CheckerManager &Mgr) { + Mgr.registerChecker<UnborrowedLambdaCapturesChecker>(); +} + +bool ento::shouldRegisterUnborrowedLambdaCapturesChecker( + const CheckerManager &mgr) { + return true; +} diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefSafetyModel.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefSafetyModel.cpp index d0e8c1bee899e..970ba8ef44fa8 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefSafetyModel.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefSafetyModel.cpp @@ -152,12 +152,18 @@ class BorrowSafetyModel : public PtrRefSafetyModel { const char *typeName() const override { return "CanBorrow type"; } void describeHazard(llvm::raw_ostream &Os, const Expr *Origin, - QualType) const override { + QualType SinkType) const override { + QualType SinkObject = pointeeType(SinkType); + if (!SinkObject.isNull() && isBorrowType(SinkObject)) { + Os << "Borrow that does not travel with the lambda"; + return; + } + Os << "loan on "; QualType OriginType = Origin ? pointeeType(Origin->getType()) : QualType(); // Name the borrowed type, not the Borrow<T> guard, when the loan was - // taken from a Borrow<T> temporary. + // taken from a Borrow<T>. if (!OriginType.isNull() && isBorrowType(OriginType)) OriginType = borrowedType(OriginType); diff --git a/clang/test/Analysis/Checkers/WebKit/mock-canborrow.h b/clang/test/Analysis/Checkers/WebKit/mock-canborrow.h index 0bce868fc11ff..7b68bba4999a3 100644 --- a/clang/test/Analysis/Checkers/WebKit/mock-canborrow.h +++ b/clang/test/Analysis/Checkers/WebKit/mock-canborrow.h @@ -223,4 +223,36 @@ class Element { void inspect() const; }; +namespace detail { +class CallableBase { +public: + virtual ~CallableBase() {} + virtual void call() = 0; +}; + +template <typename F> class Callable : public CallableBase { +public: + Callable(F f) : m_f(f) {} + void call() override { m_f(); } + +private: + F m_f; +}; +} // namespace detail + +class Function { +public: + template <typename F> + Function(F f) : m_impl(new detail::Callable<F>(f)) {} + ~Function() { delete m_impl; } + + void operator()() const { m_impl->call(); } + +private: + detail::CallableBase *m_impl { nullptr }; +}; + +void callEscaping(const Function &); +void callNoEscape([[clang::noescape]] const Function &); + #endif diff --git a/clang/test/Analysis/Checkers/WebKit/unborrowed-lambda-captures.cpp b/clang/test/Analysis/Checkers/WebKit/unborrowed-lambda-captures.cpp new file mode 100644 index 0000000000000..7f35625fc521b --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/unborrowed-lambda-captures.cpp @@ -0,0 +1,166 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UnborrowedLambdaCapturesChecker -verify %s + +#include "mock-canborrow.h" + +void someFunction(); +void use(char *); + +namespace loan_shapes { + +void init_capture_computing_a_loan() { + Vector<char> vec; + callEscaping([q = vec.data()] { someFunction(); }); + // expected-warning@-1{{Captured variable 'q' is a loan on CanBorrow type 'Vector<char>' that is not guarded by a Borrow [alpha.webkit.UnborrowedLambdaCapturesChecker]}} +} + +void reference_to_an_element() { + Vector<char> vec; + callEscaping([&c = vec[0]] { someFunction(); }); + // expected-warning@-1{{Captured variable 'c' is a loan on CanBorrow type 'Vector<char>' that is not guarded by a Borrow [alpha.webkit.UnborrowedLambdaCapturesChecker]}} +} + +void loan_on_a_parameter(Vector<char> ¶meter) { + callEscaping([q = parameter.data()] { someFunction(); }); + // expected-warning@-1{{Captured variable 'q' is a loan on CanBorrow type 'Vector<char>' that is not guarded by a Borrow [alpha.webkit.UnborrowedLambdaCapturesChecker]}} +} + +void loan_on_a_nested_container() { + Vector<Vector<char>> outer; + callEscaping([&inner = outer[0]] { someFunction(); }); + // expected-warning@-1{{Captured variable 'inner' is a loan on CanBorrow type 'Vector<Vector<char>>' that is not guarded by a Borrow [alpha.webkit.UnborrowedLambdaCapturesChecker]}} +} + +} // namespace loan_shapes + +namespace borrow_does_not_travel { + +void loan_through_a_borrow() { + Vector<char> vec; + Borrow<Vector<char>> b(vec); + callEscaping([q = b.get().data()] { someFunction(); }); + // expected-warning@-1{{Captured variable 'q' is a loan on CanBorrow type 'Vector<char>' that is not guarded by a Borrow [alpha.webkit.UnborrowedLambdaCapturesChecker]}} +} + +void element_through_a_borrow() { + Vector<char> vec; + Borrow<Vector<char>> b(vec); + callEscaping([&c = b.get()[0]] { someFunction(); }); + // expected-warning@-1{{Captured variable 'c' is a loan on CanBorrow type 'Vector<char>' that is not guarded by a Borrow [alpha.webkit.UnborrowedLambdaCapturesChecker]}} +} + +void loan_through_a_borrow_temporary() { + Vector<char> vec; + callEscaping([q = borrow(vec).get().data()] { someFunction(); }); + // expected-warning@-1{{Captured variable 'q' is a loan on CanBorrow type 'Vector<char>' that is not guarded by a Borrow [alpha.webkit.UnborrowedLambdaCapturesChecker]}} +} + +void capture_of_the_borrow_by_reference() { + Vector<char> vec; + Borrow<Vector<char>> b(vec); + callEscaping([&b] { someFunction(); }); + // expected-warning@-1{{Captured variable 'b' is a Borrow that does not travel with the lambda [alpha.webkit.UnborrowedLambdaCapturesChecker]}} +} + +void borrow_inside_the_body() { + Vector<char> vec; + callEscaping([&vec] { + Borrow<Vector<char>> b(vec); + use(b.get().data()); + }); +} + +} // namespace borrow_does_not_travel + +namespace noescape_borrows_work { + +void loan_guarded_for_the_whole_call() { + Vector<char> vec; + Borrow<Vector<char>> b(vec); + callNoEscape([q = b.get().data()] { use(q); }); +} + +void borrow_captured_by_reference() { + Vector<char> vec; + Borrow<Vector<char>> b(vec); + callNoEscape([&b] { use(b.get().data()); }); +} + +void borrow_inside_the_body() { + Vector<char> vec; + callNoEscape([&vec] { + Borrow<Vector<char>> b(vec); + use(b.get().data()); + }); +} + +void nested_container_guarded_at_the_inner() { + Vector<Vector<char>> outer; + Borrow<Vector<char>> b(outer[0]); + callNoEscape([q = b.get().data()] { use(q); }); +} + +} // namespace noescape_borrows_work + +namespace not_a_loan { + +void reference_to_the_container() { + Vector<char> vec; + callEscaping([&vec] { vec.append('x'); }); +} + +void copy_of_an_element() { + Vector<char> vec; + callEscaping([c = vec[0]] { someFunction(); }); +} + +void copy_through_a_reference_variable() { + Vector<char> vec; + Vector<char> &r = vec; + callEscaping([r] { someFunction(); }); +} + +void view_on_a_non_container() { + NotBorrowable buffer; + callEscaping([&c = buffer.at(0)] { someFunction(); }); +} + +void unrelated_pointer_parameter(char *unrelated) { + callEscaping([unrelated] { someFunction(); }); +} + +class Holder { +public: + void capturesThis() { callEscaping([this] { someFunction(); }); } + +private: + Vector<char> m_vec; +}; + +extern const Vector<char> globalConstBuffer; + +void loan_on_const_global() { + callEscaping([p = globalConstBuffer.data()] { someFunction(); }); +} + +} // namespace not_a_loan + +namespace known_gaps { + +void noescape_parameter() { + Vector<char> vec; + callNoEscape([q = vec.data()] { someFunction(); }); +} + +void trivial_body() { + Vector<char> vec; + callEscaping([q = vec.data()] {}); +} + +void loan_through_a_named_local() { + Vector<char> vec; + char *p = vec.data(); + callNoEscape([p] { someFunction(); }); + callEscaping([p] { someFunction(); }); +} + +} // namespace known_gaps _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
