Skip to content

Commit

Permalink
[coroutines][coro_lifetimebound] Detect lifetime issues with lambda c…
Browse files Browse the repository at this point in the history
…aptures (#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 #76995

---------

Co-authored-by: Chuanqi Xu <yedeng.yd@linux.alibaba.com>
  • Loading branch information
usx95 and ChuanqiXu9 committed Jan 18, 2024
1 parent 40d952b commit 667e58a
Show file tree
Hide file tree
Showing 6 changed files with 119 additions and 8 deletions.
5 changes: 5 additions & 0 deletions clang/include/clang/Sema/Sema.h
Original file line number Diff line number Diff line change
Expand Up @@ -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]].
Expand Down
1 change: 1 addition & 0 deletions clang/lib/Sema/SemaCoroutine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
19 changes: 16 additions & 3 deletions clang/lib/Sema/SemaDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
19 changes: 16 additions & 3 deletions clang/lib/Sema/SemaInit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Expand Down
71 changes: 69 additions & 2 deletions clang/test/SemaCXX/coro-lifetimebound.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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}}
Expand All @@ -84,14 +88,59 @@ 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
// =============================================================================
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.
Expand Down Expand Up @@ -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
12 changes: 12 additions & 0 deletions clang/test/SemaCXX/coro-return-type-and-wrapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit 667e58a

Please sign in to comment.