diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCapturingLambdaCoroutinesCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCapturingLambdaCoroutinesCheck.h --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCapturingLambdaCoroutinesCheck.h +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCapturingLambdaCoroutinesCheck.h @@ -11,26 +11,23 @@ #include "../ClangTidyCheck.h" -namespace clang { -namespace tidy { -namespace cppcoreguidelines { +namespace clang::tidy::cppcoreguidelines { -/// The normal usage of captures in lambdas are problematic when the lambda is a -/// coroutine because the captures are destroyed after the first suspension -/// point. Using the captures after this point is a use-after-free issue. +/// Check flags C++20 coroutine lambdas with non-empty capture lists that may +/// cause use-after-free errors and suggests avoiding captures or ensuring the +/// lambda closure object has a guaranteed lifetime. /// /// For the user-facing documentation see: -/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-avoid-capturing-lambda-coroutines.html +/// https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/avoid-capturing-lambda-coroutines.html class AvoidCapturingLambdaCoroutinesCheck : public ClangTidyCheck { public: AvoidCapturingLambdaCoroutinesCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override; }; -} // namespace cppcoreguidelines -} // namespace tidy -} // namespace clang +} // namespace clang::tidy::cppcoreguidelines #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDCAPTURINGLAMBDACOROUTINESCHECK_H diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCapturingLambdaCoroutinesCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCapturingLambdaCoroutinesCheck.cpp --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCapturingLambdaCoroutinesCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCapturingLambdaCoroutinesCheck.cpp @@ -12,34 +12,34 @@ using namespace clang::ast_matchers; -namespace clang { -namespace tidy { -namespace cppcoreguidelines { +namespace clang::tidy::cppcoreguidelines { + +namespace { +AST_MATCHER(LambdaExpr, hasCoroutineBody) { + const Stmt *Body = Node.getBody(); + return Body != nullptr && CoroutineBodyStmt::classof(Body); +} + +AST_MATCHER(LambdaExpr, hasCaptures) { return Node.capture_size() != 0U; } +} // namespace void AvoidCapturingLambdaCoroutinesCheck::registerMatchers( MatchFinder *Finder) { - Finder->addMatcher(lambdaExpr().bind("lambda"), this); + Finder->addMatcher( + lambdaExpr(hasCaptures(), hasCoroutineBody()).bind("lambda"), this); +} + +bool AvoidCapturingLambdaCoroutinesCheck::isLanguageVersionSupported( + const LangOptions &LangOpts) const { + return LangOpts.CPlusPlus20; } void AvoidCapturingLambdaCoroutinesCheck::check( const MatchFinder::MatchResult &Result) { - const auto *Lambda = Result.Nodes.getNodeAs("lambda"); - if (!Lambda) { - return; - } - - const auto *Body = dyn_cast(Lambda->getBody()); - if (!Body) { - return; - } - - if (Lambda->captures().empty()) { - return; - } - - diag(Lambda->getBeginLoc(), "found capturing coroutine lambda"); + const auto *MatchedLambda = Result.Nodes.getNodeAs("lambda"); + diag(MatchedLambda->getExprLoc(), + "coroutine lambda may cause use-after-free, avoid captures or ensure " + "lambda closure object has guaranteed lifetime"); } -} // namespace cppcoreguidelines -} // namespace tidy -} // namespace clang +} // namespace clang::tidy::cppcoreguidelines diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -123,9 +123,9 @@ - New :doc:`cppcoreguidelines-avoid-capturing-lambda-coroutines ` check. - Adds check for cpp core guideline: "CP.51: Do not use capturing lambdas that - are coroutines." - + Check flags C++20 coroutine lambdas with non-empty capture lists that may + cause use-after-free errors and suggests avoiding captures or ensuring the + lambda closure object has a guaranteed lifetime. New check aliases ^^^^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capturing-lambda-coroutines.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capturing-lambda-coroutines.rst --- a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capturing-lambda-coroutines.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capturing-lambda-coroutines.rst @@ -3,29 +3,52 @@ cppcoreguidelines-avoid-capturing-lambda-coroutines =================================================== -Warns if a capturing lambda is a coroutine. For example: +Check flags C++20 coroutine lambdas with non-empty capture lists that may cause +use-after-free errors and suggests avoiding captures or ensuring the lambda +closure object has a guaranteed lifetime. + +This check implements `CP.51 +`_ +from the C++ Core Guidelines. + +Using coroutine lambdas with non-empty capture lists can be risky, as capturing +variables can lead to accessing freed memory after the first suspension point. +This issue can occur even with refcounted smart pointers and copyable types. +When a lambda expression creates a coroutine, it results in a closure object +with storage, which is often on the stack and will eventually go out of scope. +When the closure object goes out of scope, its captures also go out of scope. +While normal lambdas finish executing before this happens, coroutine lambdas may +resume from suspension after the closure object has been destructed, resulting +in use-after-free memory access for all captures. + +Consider the following example: .. code-block:: c++ - int c; - - [c] () -> task { co_return; }; - [&] () -> task { int y = c; co_return; }; - [=] () -> task { int y = c; co_return; }; - - struct s { - void m() { - [this] () -> task { co_return; }; - } - }; - -All of the cases above will trigger the warning. However, implicit captures -do not trigger the warning unless the body of the lambda uses the capture. -For example, the following do not trigger the warning. - -.. code-block:: c++ - - int c; - - [&] () -> task { co_return; }; - [=] () -> task { co_return; }; + int value = get_value(); + std::shared_ptr sharedFoo = get_foo(); + { + const auto lambda = [value, sharedFoo]() -> std::future + { + co_await something(); + // "sharedFoo" and "value" have already been destroyed + // the "shared" pointer didn't accomplish anything + }; + lambda(); + } // the lambda closure object has now gone out of scope + +In this example, the lambda object is defined with two captures: value and +``sharedFoo``. When ``lambda()`` is called, the lambda object is created on the +stack, and the captures are copied into the closure object. When the coroutine +is suspended, the lambda object goes out of scope, and the closure object is +destroyed. When the coroutine is resumed, the captured variables may have been +destroyed, resulting in use-after-free bugs. + +In conclusion, the use of coroutine lambdas with non-empty capture lists can +lead to use-after-free errors when resuming the coroutine after the closure +object has been destroyed. This check helps prevent such errors by flagging +C++20 coroutine lambdas with non-empty capture lists and suggesting avoiding +captures or ensuring the lambda closure object has a guaranteed lifetime. + +Following these guidelines can help ensure the safe and reliable use of +coroutine lambdas in C++ code. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -180,7 +180,7 @@ `concurrency-mt-unsafe `_, `concurrency-thread-canceltype-asynchronous `_, `cppcoreguidelines-avoid-capture-default-when-capturing-this `_, - `cppcoreguidelines-avoid-capturing-lambda-coroutines `_, "Yes" + `cppcoreguidelines-avoid-capturing-lambda-coroutines `_, `cppcoreguidelines-avoid-const-or-ref-data-members `_, `cppcoreguidelines-avoid-do-while `_, `cppcoreguidelines-avoid-goto `_, diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/Inputs/system/coroutines.h b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/Inputs/system/coroutines.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/Inputs/system/coroutines.h @@ -0,0 +1,32 @@ +#pragma once + +namespace std { + +template +struct coroutine_traits { + using promise_type = typename ret_t::promise_type; +}; + +template +struct coroutine_handle { + static constexpr coroutine_handle from_address(void *addr) noexcept { return {}; }; +}; + +} // namespace std + +struct never_suspend { + bool await_ready() noexcept { return false; } + template + void await_suspend(coro_t handle) noexcept {} + void await_resume() noexcept {} +}; + +struct task { + struct promise_type { + task get_return_object() noexcept { return {}; } + never_suspend initial_suspend() noexcept { return {}; } + never_suspend final_suspend() noexcept { return {}; } + void return_void() {} + void unhandled_exception() {} + }; +}; diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capturing-lambda-coroutines.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capturing-lambda-coroutines.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capturing-lambda-coroutines.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capturing-lambda-coroutines.cpp @@ -1,5 +1,4 @@ -// RUN: %check_clang_tidy -std=c++20-or-later %s cppcoreguidelines-avoid-capturing-lambda-coroutines %t -- -- \ -// RUN: -isystem %S/readability/Inputs/identifier-naming/system +// RUN: %check_clang_tidy -std=c++20-or-later %s cppcoreguidelines-avoid-capturing-lambda-coroutines %t -- -- -isystem %S/Inputs/system #include @@ -7,23 +6,23 @@ int v; [&] () -> task { int y = v; co_return; }; - // CHECK-MESSAGES: [[@LINE-1]]:5: warning: found capturing coroutine lambda [cppcoreguidelines-avoid-capturing-lambda-coroutines] + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: coroutine lambda may cause use-after-free, avoid captures or ensure lambda closure object has guaranteed lifetime [cppcoreguidelines-avoid-capturing-lambda-coroutines] [=] () -> task { int y = v; co_return; }; - // CHECK-MESSAGES: [[@LINE-1]]:5: warning: found capturing coroutine lambda [cppcoreguidelines-avoid-capturing-lambda-coroutines] + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: coroutine lambda may cause use-after-free, avoid captures or ensure lambda closure object has guaranteed lifetime [cppcoreguidelines-avoid-capturing-lambda-coroutines] [v] () -> task { co_return; }; - // CHECK-MESSAGES: [[@LINE-1]]:5: warning: found capturing coroutine lambda [cppcoreguidelines-avoid-capturing-lambda-coroutines] + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: coroutine lambda may cause use-after-free, avoid captures or ensure lambda closure object has guaranteed lifetime [cppcoreguidelines-avoid-capturing-lambda-coroutines] [&v] () -> task { co_return; }; - // CHECK-MESSAGES: [[@LINE-1]]:5: warning: found capturing coroutine lambda [cppcoreguidelines-avoid-capturing-lambda-coroutines] + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: coroutine lambda may cause use-after-free, avoid captures or ensure lambda closure object has guaranteed lifetime [cppcoreguidelines-avoid-capturing-lambda-coroutines] [y=v] () -> task { co_return; }; - // CHECK-MESSAGES: [[@LINE-1]]:5: warning: found capturing coroutine lambda [cppcoreguidelines-avoid-capturing-lambda-coroutines] + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: coroutine lambda may cause use-after-free, avoid captures or ensure lambda closure object has guaranteed lifetime [cppcoreguidelines-avoid-capturing-lambda-coroutines] [y{v}] () -> task { co_return; }; - // CHECK-MESSAGES: [[@LINE-1]]:5: warning: found capturing coroutine lambda [cppcoreguidelines-avoid-capturing-lambda-coroutines] + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: coroutine lambda may cause use-after-free, avoid captures or ensure lambda closure object has guaranteed lifetime [cppcoreguidelines-avoid-capturing-lambda-coroutines] } struct S { void m() { [this] () -> task { co_return; }; - // CHECK-MESSAGES: [[@LINE-1]]:9: warning: found capturing coroutine lambda [cppcoreguidelines-avoid-capturing-lambda-coroutines] + // CHECK-MESSAGES: [[@LINE-1]]:9: warning: coroutine lambda may cause use-after-free, avoid captures or ensure lambda closure object has guaranteed lifetime [cppcoreguidelines-avoid-capturing-lambda-coroutines] } }; @@ -32,4 +31,6 @@ [] () -> task { co_return; }; [&] () -> task { co_return; }; [=] () -> task { co_return; }; + + [&v]{++v;}(); }