Skip to content

Commit 667e58a

Browse files
usx95ChuanqiXu9
andauthored
[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 #76995 --------- Co-authored-by: Chuanqi Xu <[email protected]>
1 parent 40d952b commit 667e58a

File tree

6 files changed

+119
-8
lines changed

6 files changed

+119
-8
lines changed

clang/include/clang/Sema/Sema.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11249,6 +11249,11 @@ class Sema final {
1124911249
VarDecl *buildCoroutinePromise(SourceLocation Loc);
1125011250
void CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *&Body);
1125111251

11252+
// Heuristically tells if the function is `get_return_object` member of a
11253+
// coroutine promise_type by matching the function name.
11254+
static bool CanBeGetReturnObject(const FunctionDecl *FD);
11255+
static bool CanBeGetReturnTypeOnAllocFailure(const FunctionDecl *FD);
11256+
1125211257
// As a clang extension, enforces that a non-coroutine function must be marked
1125311258
// with [[clang::coro_wrapper]] if it returns a type marked with
1125411259
// [[clang::coro_return_type]].

clang/lib/Sema/SemaCoroutine.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#include "CoroutineStmtBuilder.h"
1717
#include "clang/AST/ASTLambda.h"
1818
#include "clang/AST/Decl.h"
19+
#include "clang/AST/Expr.h"
1920
#include "clang/AST/ExprCXX.h"
2021
#include "clang/AST/StmtCXX.h"
2122
#include "clang/Basic/Builtins.h"

clang/lib/Sema/SemaDecl.cpp

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15912,13 +15912,26 @@ static void diagnoseImplicitlyRetainedSelf(Sema &S) {
1591215912
<< FixItHint::CreateInsertion(P.first, "self->");
1591315913
}
1591415914

15915+
static bool methodHasName(const FunctionDecl *FD, StringRef Name) {
15916+
return isa<CXXMethodDecl>(FD) && FD->param_empty() &&
15917+
FD->getDeclName().isIdentifier() && FD->getName().equals(Name);
15918+
}
15919+
15920+
bool Sema::CanBeGetReturnObject(const FunctionDecl *FD) {
15921+
return methodHasName(FD, "get_return_object");
15922+
}
15923+
15924+
bool Sema::CanBeGetReturnTypeOnAllocFailure(const FunctionDecl *FD) {
15925+
return FD->isStatic() &&
15926+
methodHasName(FD, "get_return_object_on_allocation_failure");
15927+
}
15928+
1591515929
void Sema::CheckCoroutineWrapper(FunctionDecl *FD) {
1591615930
RecordDecl *RD = FD->getReturnType()->getAsRecordDecl();
1591715931
if (!RD || !RD->getUnderlyingDecl()->hasAttr<CoroReturnTypeAttr>())
1591815932
return;
15919-
// Allow `get_return_object()`.
15920-
if (FD->getDeclName().isIdentifier() &&
15921-
FD->getName().equals("get_return_object") && FD->param_empty())
15933+
// Allow some_promise_type::get_return_object().
15934+
if (CanBeGetReturnObject(FD) || CanBeGetReturnTypeOnAllocFailure(FD))
1592215935
return;
1592315936
if (!FD->hasAttr<CoroWrapperAttr>())
1592415937
Diag(FD->getLocation(), diag::err_coroutine_return_type) << RD;

clang/lib/Sema/SemaInit.cpp

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
#include "clang/AST/ASTContext.h"
1414
#include "clang/AST/DeclObjC.h"
15+
#include "clang/AST/Expr.h"
1516
#include "clang/AST/ExprCXX.h"
1617
#include "clang/AST/ExprObjC.h"
1718
#include "clang/AST/ExprOpenMP.h"
@@ -7583,15 +7584,27 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
75837584
Path.pop_back();
75847585
};
75857586

7586-
if (ObjectArg && implicitObjectParamIsLifetimeBound(Callee))
7587-
VisitLifetimeBoundArg(Callee, ObjectArg);
7588-
75897587
bool CheckCoroCall = false;
75907588
if (const auto *RD = Callee->getReturnType()->getAsRecordDecl()) {
75917589
CheckCoroCall = RD->hasAttr<CoroLifetimeBoundAttr>() &&
75927590
RD->hasAttr<CoroReturnTypeAttr>() &&
75937591
!Callee->hasAttr<CoroDisableLifetimeBoundAttr>();
75947592
}
7593+
7594+
if (ObjectArg) {
7595+
bool CheckCoroObjArg = CheckCoroCall;
7596+
// Coroutine lambda objects with empty capture list are not lifetimebound.
7597+
if (auto *LE = dyn_cast<LambdaExpr>(ObjectArg->IgnoreImplicit());
7598+
LE && LE->captures().empty())
7599+
CheckCoroObjArg = false;
7600+
// Allow `get_return_object()` as the object param (__promise) is not
7601+
// lifetimebound.
7602+
if (Sema::CanBeGetReturnObject(Callee))
7603+
CheckCoroObjArg = false;
7604+
if (implicitObjectParamIsLifetimeBound(Callee) || CheckCoroObjArg)
7605+
VisitLifetimeBoundArg(Callee, ObjectArg);
7606+
}
7607+
75957608
for (unsigned I = 0,
75967609
N = std::min<unsigned>(Callee->getNumParams(), Args.size());
75977610
I != N; ++I) {

clang/test/SemaCXX/coro-lifetimebound.cpp

Lines changed: 69 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,10 @@ Co<int> bar_coro(const int &b, int c) {
6464
: bar_coro(0, 1); // expected-warning {{returning address of local temporary object}}
6565
}
6666

67+
// =============================================================================
68+
// Lambdas
69+
// =============================================================================
70+
namespace lambdas {
6771
void lambdas() {
6872
auto unsafe_lambda = [] [[clang::coro_wrapper]] (int b) {
6973
return foo_coro(b); // expected-warning {{address of stack memory associated with parameter}}
@@ -84,14 +88,59 @@ void lambdas() {
8488
co_return x + co_await foo_coro(b);
8589
};
8690
}
91+
92+
Co<int> lambda_captures() {
93+
int a = 1;
94+
// Temporary lambda object dies.
95+
auto lamb = [a](int x, const int& y) -> Co<int> { // expected-warning {{temporary whose address is used as value of local variable 'lamb'}}
96+
co_return x + y + a;
97+
}(1, a);
98+
// Object dies but it has no capture.
99+
auto no_capture = []() -> Co<int> { co_return 1; }();
100+
auto bad_no_capture = [](const int& a) -> Co<int> { co_return a; }(1); // expected-warning {{temporary}}
101+
// Temporary lambda object with lifetime extension under co_await.
102+
int res = co_await [a](int x, const int& y) -> Co<int> {
103+
co_return x + y + a;
104+
}(1, a);
105+
// Lambda object on stack should be fine.
106+
auto lamb2 = [a]() -> Co<int> { co_return a; };
107+
auto on_stack = lamb2();
108+
auto res2 = co_await on_stack;
109+
co_return 1;
110+
}
111+
} // namespace lambdas
112+
113+
// =============================================================================
114+
// Member coroutines
115+
// =============================================================================
116+
namespace member_coroutines{
117+
struct S {
118+
Co<int> member(const int& a) { co_return a; }
119+
};
120+
121+
Co<int> use() {
122+
S s;
123+
int a = 1;
124+
auto test1 = s.member(1); // expected-warning {{temporary whose address is used as value of local variable}}
125+
auto test2 = s.member(a);
126+
auto test3 = S{}.member(a); // expected-warning {{temporary whose address is used as value of local variable}}
127+
co_return 1;
128+
}
129+
130+
[[clang::coro_wrapper]] Co<int> wrapper(const int& a) {
131+
S s;
132+
return s.member(a); // expected-warning {{address of stack memory}}
133+
}
134+
} // member_coroutines
135+
87136
// =============================================================================
88137
// Safe usage when parameters are value
89138
// =============================================================================
90139
namespace by_value {
91140
Co<int> value_coro(int b) { co_return co_await foo_coro(b); }
92141
[[clang::coro_wrapper]] Co<int> wrapper1(int b) { return value_coro(b); }
93142
[[clang::coro_wrapper]] Co<int> wrapper2(const int& b) { return value_coro(b); }
94-
}
143+
} // namespace by_value
95144

96145
// =============================================================================
97146
// Lifetime bound but not a Coroutine Return Type: No analysis.
@@ -122,11 +171,29 @@ CoNoCRT<int> bar(int a) {
122171
namespace disable_lifetimebound {
123172
Co<int> foo(int x) { co_return x; }
124173

125-
[[clang::coro_wrapper, clang::coro_disable_lifetimebound]]
174+
[[clang::coro_wrapper, clang::coro_disable_lifetimebound]]
126175
Co<int> foo_wrapper(const int& x) { return foo(x); }
127176

128177
[[clang::coro_wrapper]] Co<int> caller() {
129178
// The call to foo_wrapper is wrapper is safe.
130179
return foo_wrapper(1);
131180
}
181+
182+
struct S{
183+
[[clang::coro_wrapper, clang::coro_disable_lifetimebound]]
184+
Co<int> member(const int& x) { return foo(x); }
185+
};
186+
187+
Co<int> use() {
188+
S s;
189+
int a = 1;
190+
auto test1 = s.member(1); // param is not flagged.
191+
auto test2 = S{}.member(a); // 'this' is not flagged.
192+
co_return 1;
193+
}
194+
195+
[[clang::coro_wrapper]] Co<int> return_stack_addr(const int& a) {
196+
S s;
197+
return s.member(a); // return of stack addr is not flagged.
198+
}
132199
} // namespace disable_lifetimebound

clang/test/SemaCXX/coro-return-type-and-wrapper.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,23 @@ using std::suspend_always;
55
using std::suspend_never;
66

77

8+
namespace std {
9+
struct nothrow_t {};
10+
constexpr nothrow_t nothrow = {};
11+
}
12+
13+
using SizeT = decltype(sizeof(int));
14+
15+
void* operator new(SizeT __sz, const std::nothrow_t&) noexcept;
16+
817
template <typename T> struct [[clang::coro_return_type]] Gen {
918
struct promise_type {
1019
Gen<T> get_return_object() {
1120
return {};
1221
}
22+
static Gen<T> get_return_object_on_allocation_failure() {
23+
return {};
24+
}
1325
suspend_always initial_suspend();
1426
suspend_always final_suspend() noexcept;
1527
void unhandled_exception();

0 commit comments

Comments
 (0)