Author: Utkarsh Saxena Date: 2024-01-18T11:56:55+01:00 New Revision: 667e58a72e0d81abe0ab3500b5d5563b6a598e7f
URL: https://github.com/llvm/llvm-project/commit/667e58a72e0d81abe0ab3500b5d5563b6a598e7f DIFF: https://github.com/llvm/llvm-project/commit/667e58a72e0d81abe0ab3500b5d5563b6a598e7f.diff LOG: [coroutines][coro_lifetimebound] Detect lifetime issues with lambda captures (#77066) ### Problem ```cpp co_task<int> coro() { int a = 1; auto lamb = [a]() -> co_task<int> { co_return a; // 'a' in the lambda object dies after the iniital_suspend in the lambda coroutine. }(); co_return co_await lamb; } ``` [use-after-free](https://godbolt.org/z/GWPEovWWc) Lambda captures (even by value) are prone to use-after-free once the lambda object dies. In the above example, the lambda object appears only as a temporary in the call expression. It dies after the first suspension (`initial_suspend`) in the lambda. On resumption in `co_await lamb`, the lambda accesses `a` which is part of the already-dead lambda object. --- ### Solution This problem can be formulated by saying that the `this` parameter of the lambda call operator is a lifetimebound parameter. The lambda object argument should therefore live atleast as long as the return object. That said, this requirement does not hold if the lambda does not have a capture list. In principle, the coroutine frame still has a reference to a dead lambda object, but it is easy to see that the object would not be used in the lambda-coroutine body due to no capture list. It is safe to use this pattern inside a`co_await` expression due to the lifetime extension of temporaries. Example: ```cpp co_task<int> coro() { int a = 1; int res = co_await [a]() -> co_task<int> { co_return a; }(); co_return res; } ``` --- ### Background This came up in the discussion with seastar folks on [RFC](https://discourse.llvm.org/t/rfc-lifetime-bound-check-for-parameters-of-coroutines/74253/19?u=usx95). This is a fairly common pattern in continuation-style-passing (CSP) async programming involving futures and continuations. Document ["Lambda coroutine fiasco"](https://github.com/scylladb/seastar/blob/master/doc/lambda-coroutine-fiasco.md) by Seastar captures the problem. This pattern makes the migration from CSP-style async programming to coroutines very bugprone. Fixes https://github.com/llvm/llvm-project/issues/76995 --------- Co-authored-by: Chuanqi Xu <yedeng...@linux.alibaba.com> Added: Modified: clang/include/clang/Sema/Sema.h clang/lib/Sema/SemaCoroutine.cpp clang/lib/Sema/SemaDecl.cpp clang/lib/Sema/SemaInit.cpp clang/test/SemaCXX/coro-lifetimebound.cpp clang/test/SemaCXX/coro-return-type-and-wrapper.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 6ce422d66ae5b0e..0db39333b0ee347 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -11249,6 +11249,11 @@ class Sema final { VarDecl *buildCoroutinePromise(SourceLocation Loc); void CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *&Body); + // Heuristically tells if the function is `get_return_object` member of a + // coroutine promise_type by matching the function name. + static bool CanBeGetReturnObject(const FunctionDecl *FD); + static bool CanBeGetReturnTypeOnAllocFailure(const FunctionDecl *FD); + // As a clang extension, enforces that a non-coroutine function must be marked // with [[clang::coro_wrapper]] if it returns a type marked with // [[clang::coro_return_type]]. diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp index bee80db8d166a68..0e0f8f67dcd73e1 100644 --- a/clang/lib/Sema/SemaCoroutine.cpp +++ b/clang/lib/Sema/SemaCoroutine.cpp @@ -16,6 +16,7 @@ #include "CoroutineStmtBuilder.h" #include "clang/AST/ASTLambda.h" #include "clang/AST/Decl.h" +#include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" #include "clang/AST/StmtCXX.h" #include "clang/Basic/Builtins.h" diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 5472b43aafd4f39..eb28631ee6939fd 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -15912,13 +15912,26 @@ static void diagnoseImplicitlyRetainedSelf(Sema &S) { << FixItHint::CreateInsertion(P.first, "self->"); } +static bool methodHasName(const FunctionDecl *FD, StringRef Name) { + return isa<CXXMethodDecl>(FD) && FD->param_empty() && + FD->getDeclName().isIdentifier() && FD->getName().equals(Name); +} + +bool Sema::CanBeGetReturnObject(const FunctionDecl *FD) { + return methodHasName(FD, "get_return_object"); +} + +bool Sema::CanBeGetReturnTypeOnAllocFailure(const FunctionDecl *FD) { + return FD->isStatic() && + methodHasName(FD, "get_return_object_on_allocation_failure"); +} + void Sema::CheckCoroutineWrapper(FunctionDecl *FD) { RecordDecl *RD = FD->getReturnType()->getAsRecordDecl(); if (!RD || !RD->getUnderlyingDecl()->hasAttr<CoroReturnTypeAttr>()) return; - // Allow `get_return_object()`. - if (FD->getDeclName().isIdentifier() && - FD->getName().equals("get_return_object") && FD->param_empty()) + // Allow some_promise_type::get_return_object(). + if (CanBeGetReturnObject(FD) || CanBeGetReturnTypeOnAllocFailure(FD)) return; if (!FD->hasAttr<CoroWrapperAttr>()) Diag(FD->getLocation(), diag::err_coroutine_return_type) << RD; diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp index 408ee5f775804b6..96900efa75fcdff 100644 --- a/clang/lib/Sema/SemaInit.cpp +++ b/clang/lib/Sema/SemaInit.cpp @@ -12,6 +12,7 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/DeclObjC.h" +#include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" #include "clang/AST/ExprObjC.h" #include "clang/AST/ExprOpenMP.h" @@ -7583,15 +7584,27 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call, Path.pop_back(); }; - if (ObjectArg && implicitObjectParamIsLifetimeBound(Callee)) - VisitLifetimeBoundArg(Callee, ObjectArg); - bool CheckCoroCall = false; if (const auto *RD = Callee->getReturnType()->getAsRecordDecl()) { CheckCoroCall = RD->hasAttr<CoroLifetimeBoundAttr>() && RD->hasAttr<CoroReturnTypeAttr>() && !Callee->hasAttr<CoroDisableLifetimeBoundAttr>(); } + + if (ObjectArg) { + bool CheckCoroObjArg = CheckCoroCall; + // Coroutine lambda objects with empty capture list are not lifetimebound. + if (auto *LE = dyn_cast<LambdaExpr>(ObjectArg->IgnoreImplicit()); + LE && LE->captures().empty()) + CheckCoroObjArg = false; + // Allow `get_return_object()` as the object param (__promise) is not + // lifetimebound. + if (Sema::CanBeGetReturnObject(Callee)) + CheckCoroObjArg = false; + if (implicitObjectParamIsLifetimeBound(Callee) || CheckCoroObjArg) + VisitLifetimeBoundArg(Callee, ObjectArg); + } + for (unsigned I = 0, N = std::min<unsigned>(Callee->getNumParams(), Args.size()); I != N; ++I) { diff --git a/clang/test/SemaCXX/coro-lifetimebound.cpp b/clang/test/SemaCXX/coro-lifetimebound.cpp index 3fc7ca70a14a124..9e96a296562a059 100644 --- a/clang/test/SemaCXX/coro-lifetimebound.cpp +++ b/clang/test/SemaCXX/coro-lifetimebound.cpp @@ -64,6 +64,10 @@ Co<int> bar_coro(const int &b, int c) { : bar_coro(0, 1); // expected-warning {{returning address of local temporary object}} } +// ============================================================================= +// Lambdas +// ============================================================================= +namespace lambdas { void lambdas() { auto unsafe_lambda = [] [[clang::coro_wrapper]] (int b) { return foo_coro(b); // expected-warning {{address of stack memory associated with parameter}} @@ -84,6 +88,51 @@ void lambdas() { co_return x + co_await foo_coro(b); }; } + +Co<int> lambda_captures() { + int a = 1; + // Temporary lambda object dies. + auto lamb = [a](int x, const int& y) -> Co<int> { // expected-warning {{temporary whose address is used as value of local variable 'lamb'}} + co_return x + y + a; + }(1, a); + // Object dies but it has no capture. + auto no_capture = []() -> Co<int> { co_return 1; }(); + auto bad_no_capture = [](const int& a) -> Co<int> { co_return a; }(1); // expected-warning {{temporary}} + // Temporary lambda object with lifetime extension under co_await. + int res = co_await [a](int x, const int& y) -> Co<int> { + co_return x + y + a; + }(1, a); + // Lambda object on stack should be fine. + auto lamb2 = [a]() -> Co<int> { co_return a; }; + auto on_stack = lamb2(); + auto res2 = co_await on_stack; + co_return 1; +} +} // namespace lambdas + +// ============================================================================= +// Member coroutines +// ============================================================================= +namespace member_coroutines{ +struct S { + Co<int> member(const int& a) { co_return a; } +}; + +Co<int> use() { + S s; + int a = 1; + auto test1 = s.member(1); // expected-warning {{temporary whose address is used as value of local variable}} + auto test2 = s.member(a); + auto test3 = S{}.member(a); // expected-warning {{temporary whose address is used as value of local variable}} + co_return 1; +} + +[[clang::coro_wrapper]] Co<int> wrapper(const int& a) { + S s; + return s.member(a); // expected-warning {{address of stack memory}} +} +} // member_coroutines + // ============================================================================= // Safe usage when parameters are value // ============================================================================= @@ -91,7 +140,7 @@ namespace by_value { Co<int> value_coro(int b) { co_return co_await foo_coro(b); } [[clang::coro_wrapper]] Co<int> wrapper1(int b) { return value_coro(b); } [[clang::coro_wrapper]] Co<int> wrapper2(const int& b) { return value_coro(b); } -} +} // namespace by_value // ============================================================================= // Lifetime bound but not a Coroutine Return Type: No analysis. @@ -122,11 +171,29 @@ CoNoCRT<int> bar(int a) { namespace disable_lifetimebound { Co<int> foo(int x) { co_return x; } -[[clang::coro_wrapper, clang::coro_disable_lifetimebound]] +[[clang::coro_wrapper, clang::coro_disable_lifetimebound]] Co<int> foo_wrapper(const int& x) { return foo(x); } [[clang::coro_wrapper]] Co<int> caller() { // The call to foo_wrapper is wrapper is safe. return foo_wrapper(1); } + +struct S{ +[[clang::coro_wrapper, clang::coro_disable_lifetimebound]] +Co<int> member(const int& x) { return foo(x); } +}; + +Co<int> use() { + S s; + int a = 1; + auto test1 = s.member(1); // param is not flagged. + auto test2 = S{}.member(a); // 'this' is not flagged. + co_return 1; +} + +[[clang::coro_wrapper]] Co<int> return_stack_addr(const int& a) { + S s; + return s.member(a); // return of stack addr is not flagged. +} } // namespace disable_lifetimebound diff --git a/clang/test/SemaCXX/coro-return-type-and-wrapper.cpp b/clang/test/SemaCXX/coro-return-type-and-wrapper.cpp index ac49e03ba9d90ad..b08e1c9c065a0e0 100644 --- a/clang/test/SemaCXX/coro-return-type-and-wrapper.cpp +++ b/clang/test/SemaCXX/coro-return-type-and-wrapper.cpp @@ -5,11 +5,23 @@ using std::suspend_always; using std::suspend_never; +namespace std { + struct nothrow_t {}; + constexpr nothrow_t nothrow = {}; +} + +using SizeT = decltype(sizeof(int)); + +void* operator new(SizeT __sz, const std::nothrow_t&) noexcept; + template <typename T> struct [[clang::coro_return_type]] Gen { struct promise_type { Gen<T> get_return_object() { return {}; } + static Gen<T> get_return_object_on_allocation_failure() { + return {}; + } suspend_always initial_suspend(); suspend_always final_suspend() noexcept; void unhandled_exception(); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
