[PATCH seastar v1] coroutine: explain and mitigate the lambda coroutine fiasco

[Avi Kivity]

Due to the way lambda coroutines are defined and how Seastar
continuations work, it is easy to have a use-after-free with lambda
coroutines.

Fortunately, it is easy to mitigate.

This patch documents the problem, provides a lambda coroutine wrapper
and describes how to use it, and documents APIs that are safe for lambda
coroutines without the wrapper.

A unit test is included.
---

Reminder: scripts/apply-mail.py

include/seastar/core/coroutine.hh | 35 ++++++
include/seastar/coroutine/all.hh | 2 +
.../seastar/coroutine/parallel_for_each.hh | 2 +
tests/unit/coroutines_test.cc | 27 +++++
doc/lambda-coroutine-fiasco.md | 100 ++++++++++++++++++
doc/tutorial.md | 33 ++++++
6 files changed, 199 insertions(+)
create mode 100644 doc/lambda-coroutine-fiasco.md

diff --git a/include/seastar/core/coroutine.hh b/include/seastar/core/coroutine.hh
index b8f217fafe..45d89ed191 100644
--- a/include/seastar/core/coroutine.hh
+++ b/include/seastar/core/coroutine.hh
@@ -223,10 +223,45 @@ template<typename T> struct SEASTAR_NODISCARD without_preemption_check<T> : publ
explicit without_preemption_check(seastar::future<T>&& f) noexcept : seastar::future<T>(std::move(f)) {}
};
template<> struct SEASTAR_NODISCARD without_preemption_check<> : public seastar::future<> {
explicit without_preemption_check(seastar::future<>&& f) noexcept : seastar::future<>(std::move(f)) {}
};
+
+/// Make a lambda coroutine safe for use in an outer coroutine with
+/// functions that accept continuations.
+///
+/// A lambda coroutine is not a safe parameter to a function that expects
+/// a regular Seastar continuation.
+///
+/// To use, wrap the lambda coroutine in seastar::coroutine::lambda(). The
+/// lambda coroutine must complete (co_await) in the same statement.
+///
+/// Example::
+/// ```
+/// // `future::then()` expects a continuation, so not safe for lambda
+/// // coroutines without seastar::coroutine::lambda.
+/// co_await seastar::yield().then(seastar::coroutine::lambda([captures] () -> future<> {
+/// co_await seastar::coroutine::maybe_yield();
+/// // use of `captures` here can break without seastar::coroutine::lambda.
+/// }));
+/// ```
+///
+/// \tparam Func type of function object (typically inferred)
+template <typename Func>
+class lambda {
+ Func* _func;
+public:
+ /// Create a lambda coroutine wrapper from a function object, to be passed
+ /// to a Seastar function that accepts a continuation.
+ explicit lambda(Func&& func) : _func(&func) {}
+ /// Calls the lambda coroutine object. Normally invoked by Seastar.
+ template <typename... Args>
+ decltype(auto) operator()(Args&&... args) const {
+ return std::invoke(*_func, std::forward<Args>(args)...);
+ }
+};
+
}

/// Wait for a future without a preemption check
///
/// \param f a \c future<> wrapped with \c without_preemption_check
diff --git a/include/seastar/coroutine/all.hh b/include/seastar/coroutine/all.hh
index 2c3ed1d9ce..d5a872af5e 100644
--- a/include/seastar/coroutine/all.hh
+++ b/include/seastar/coroutine/all.hh
@@ -108,10 +108,12 @@ using value_tuple_for_non_void_futures = typename value_tuple_for_non_void_futur
/// }
/// );
/// co_return a + b;
/// };
/// ```
+///
+/// Safe for use with lambda coroutines.
template <typename... Futures>
requires (sizeof ...(Futures) > 0)
class [[nodiscard("must co_await an all() object")]] all {
using tuple = std::tuple<Futures...>;
using value_tuple = typename internal::value_tuple_for_non_void_futures<Futures...>;
diff --git a/include/seastar/coroutine/parallel_for_each.hh b/include/seastar/coroutine/parallel_for_each.hh
index c19035bafe..3333cc4baa 100644
--- a/include/seastar/coroutine/parallel_for_each.hh
+++ b/include/seastar/coroutine/parallel_for_each.hh
@@ -49,10 +49,12 @@ namespace seastar::coroutine {
/// sum += x * x;
/// });
/// co_return sum;
/// };
/// ```
+///
+/// Safe for use with lambda coroutines.
template <typename Func>
// constaints for Func are defined at the parallel_for_each constructor
class [[nodiscard("must co_await an parallel_for_each() object")]] parallel_for_each final : continuation_base<> {
using coroutine_handle_t = SEASTAR_INTERNAL_COROUTINE_NAMESPACE::coroutine_handle<void>;

diff --git a/tests/unit/coroutines_test.cc b/tests/unit/coroutines_test.cc
index 23a792d571..8ab1c3cfac 100644
--- a/tests/unit/coroutines_test.cc
+++ b/tests/unit/coroutines_test.cc
@@ -892,6 +892,33 @@ SEASTAR_TEST_CASE(test_async_generator_throws_from_consumer_unbuffered) {
return test_async_generator_throws_from_consumer<std::optional>();
}

#endif

+SEASTAR_TEST_CASE(test_lambda_coroutine_in_continuation) {
+ auto dist = std::uniform_real_distribution<>(0.0, 1.0);
+ auto rand_eng = std::default_random_engine(std::random_device()());
+ double n = dist(rand_eng);
+ auto sin1 = std::sin(n); // avoid optimizer tricks
+ auto boo = std::array<char, 1025>(); // bias coroutine size towards 1024 size class
+ auto sin2 = co_await yield().then(coroutine::lambda([n, boo] () -> future<double> {
+ // Expect coroutine capture to be freed after co_await without coroutine::lambda
+ co_await yield();
+ // Try to overwrite recently-release coroutine frame by allocating in similar size-class
+ std::vector<char*> garbage;
+ for (size_t sz = 1024; sz < 2048; ++sz) {
+ for (int ctr = 0; ctr < 100; ++ctr) {
+ auto p = static_cast<char*>(malloc(sz));
+ std::memset(p, 0, sz);
+ garbage.push_back(p);
+ }
+ }
+ for (auto p : garbage) {
+ std::free(p);
+ }
+ (void)boo;
+ co_return std::sin(n);
+ }));
+ BOOST_REQUIRE_EQUAL(sin1, sin2);
+}
+
#endif
diff --git a/doc/lambda-coroutine-fiasco.md b/doc/lambda-coroutine-fiasco.md
new file mode 100644
index 0000000000..e679ccd636
--- /dev/null
+++ b/doc/lambda-coroutine-fiasco.md
@@ -0,0 +1,100 @@
+# The Lambda Coroutine Fiasco
+
+Lambda coroutines and Seastar APIs that accept continuations interact badly. This
+document explain the bad interaction and how it is mitigated.
+
+## Lambda coroutines revisited
+
+A lambda coroutine is a lambda function that is also a coroutine due
+to the use of the coroutine keywords (typically co_await). A lambda
+coroutine is notionally translated by the compiler into a struct with a
+function call operator:
+
+```cpp
+[captures] (arguments) -> seastar::future<> {
+ body
+ co_return result
+}
+```
+
+becomes (more or less)
+
+```cpp
+struct lambda {
+ captures;
+ seastar::future<> operator()(arguments) const {
+ body
+ }
+};
+```
+
+## Lambda coroutines and coroutine argument capture
+
+In addition to a lambda capturing variables from its environment, the
+coroutine also captures its arguments. This capture can happen by value
+or reference, depending on how each argument is declared.
+
+The lambda's captures however are captured by reference. To understand why,
+consider that the coroutine translation process notionally transforms a member function
+(`lambda::operator()`) to a free function:
+
+```cpp
+// before
+seastar::future<> lambda::operator()(arguments) const;
+
+// after
+seastar::future<> lambda_call_operator(const lambda& self, arguments);
+```
+
+This transform means that the lambda structure, which contains all the captured variables,
+is itself captured by the coroutine by reference.
+
+## Interaction with Seastar APIs accepting continuations
+
+Consider a Seastar API that accepts a continuation, such as
+`seastar::future::then(Func continuation)`. The behavior
+is that `continuation` is moved or copied into a private memory
+area managed by `then()`. Sometime later, the continuation is
+executed (`Func::operator()`) and the memory area is freed.
+Crucially, the memory area is freed as soon as `Func::operator()`
+returns, which can be before the future returned by it becomes
+ready. However, the coroutine can access the lambda captures
+stored in this memory area after the future is returned and before
+it becomes ready. This is a use-after-free.
+
+## Solution
+
+The solution is to avoid copying or moving the lambda into
+the memory area managed by `seastar::future::then()`. Instead,
+the lambda spends its life as a temporary. We then rely on C++
+temporary lifetime extension rules to extend its life until the
+future returned is ready, at which point the captures can longer
+be accessed.
+
+```cpp
+ co_await seastar::yield().then(seastar::coroutine::lambda([captures] () -> future<> {
+ co_await seastar::coroutine::maybe_yield();
+ // Can use `captures` here safely.
+ }));
+```
+
+`seastar::coroutine::lambda` is very similar to `std::reference_wrapper` (the
+only difference is that it works with temporaries); it can be safely moved to
+the memory area managed by `seastar::future::then()` since it's only used
+to call the real lambda, and then is safe to discard.
+
+## Alternative solution when lifetime extension cannot be used.
+
+If the lambda coroutine is not co_await'ed immediately, we cannot rely on
+lifetime extension and so we must name the coroutine and use `std::ref()` to
+refer to it without copying it from the coroutine frame:
+
+```cpp
+ auto a_lambda = [captures] () -> future<> {
+ co_await seastar::coroutine::maybe_yield();
+ // Can use `captures` here safely.
+ };
+ auto f = seastar::yield().then(std::ref(a_lambda));
+ co_await std::move(f);
+```
+
diff --git a/doc/tutorial.md b/doc/tutorial.md
index c988a59ee1..2060401117 100644
--- a/doc/tutorial.md
+++ b/doc/tutorial.md
@@ -370,10 +370,43 @@ Line #3 demonstrates the addition operation, with which the reader is assumed to

In #4, we call a function that returns a `seastar::future<>`. In this case, the future carries no value, and so no value is extracted and assigned.

Line #5 demonstrates returning a value. The integer value is used to satisfy the `future<int>` that our caller got when calling the coroutine.

+## Lambda coroutines
+
+A lambda function can be a coroutine. Due to an interaction between how C++ lambda coroutines are specified and how
+Seastar coroutines work, using lambda coroutines as continuations can result in use-after-free. To avoid such problems,
+take one of the following approaches:
+
+1. Use lambda coroutines as arguments to functions that explicitly claim support for them
+2. Wrap lambda coroutines with seastar::coroutine::lambda(), and ensure the lambda coroutine is fully awaited within the statement it is defined in.
+
+An example of wrapping a lambda coroutine is:
+
+```cpp
+#include <seastar/core/coroutine.hh>
+#include <seastar/coroutine/maybe_yield.hh>
+
+future<> foo() {
+ int n = 3;
+ int m = co_await seastar::yield().then(seastar::coroutine::lambda([n] () -> future<int> {
+ co_await seastar::coroutine::maybe_yield();
+ // `n` can be safely used here
+ co_return n;
+ }));
+ assert(n == m);
+}
+```
+
+Notes:
+1. seastar::future::then() accepts a continuation
+2. We wrap the argument to seastar::future::then() with seastar::coroutine::lambda()
+3. We ensure evaluation of the lambda completes within the same expression using the outer co_await.
+
+More information can be found in lambda-coroutine-fiasco.md.
+
## Generators in coroutines

Sometimes, it would be convenient to model a view of `input_range` with a coroutine which emits the elements one after
another asynchronously. From the consumer of the view's perspective, it can retrieve the elements by `co_await`ing
the return value of the coroutine. From the coroutine's perspective, it is able to produce the elements multiple times
--
2.37.3

[Benny Halevy]

LGTM

[Gleb Natapov]

Isn't this also fragile to use? Write 'return' instead of first co_await
and the code breaks. And I imagine if someone will co-routinize such
code he will write it as:

co_await seastar::yield();
co_await seastar::coroutine::maybe_yield();

Usually if continuations are left in such co-routinezed the purpose
is to have small background fiber. Which is exactly what this
seastar::coroutine::lambda does not support. Isn't it just simple to
advice to either co-routinize entire code or use the method below?

> --
> You received this message because you are subscribed to the Google Groups "seastar-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/20220907152430.62797-1-avi%40scylladb.com.

--
Gleb.

[Avi Kivity]

On 08/09/2022 10.57, Gleb Natapov wrote:
> +
>> +The solution is to avoid copying or moving the lambda into
>> +the memory area managed by `seastar::future::then()`. Instead,
>> +the lambda spends its life as a temporary. We then rely on C++
>> +temporary lifetime extension rules to extend its life until the
>> +future returned is ready, at which point the captures can longer
>> +be accessed.
>> +
>> +```cpp
>> + co_await seastar::yield().then(seastar::coroutine::lambda([captures] () -> future<> {
>> + co_await seastar::coroutine::maybe_yield();
>> + // Can use `captures` here safely.
>> + }));
>> +```
> Isn't this also fragile to use? Write 'return' instead of first co_await
> and the code breaks.

It will not compile (assuming the outer function remains a coroutine).

> And I imagine if someone will co-routinize such
> code he will write it as:
>
> co_await seastar::yield();
> co_await seastar::coroutine::maybe_yield();
>
> Usually if continuations are left in such co-routinezed the purpose
> is to have small background fiber. Which is exactly what this
> seastar::coroutine::lambda does not support. Isn't it just simple to
> advice to either co-routinize entire code or use the method below?

I used future::then() as the simplest API that accepts a coroutine.
Typically it will be more complicated APIs, like
seastar::parallel_for_each().


We're adding coroutine versions of them, but we don't have everything yet.

[Gleb Natapov]

On Thu, Sep 08, 2022 at 11:25:56AM +0300, Avi Kivity wrote:
>
> On 08/09/2022 10.57, Gleb Natapov wrote:
> > +
> > > +The solution is to avoid copying or moving the lambda into
> > > +the memory area managed by `seastar::future::then()`. Instead,
> > > +the lambda spends its life as a temporary. We then rely on C++
> > > +temporary lifetime extension rules to extend its life until the
> > > +future returned is ready, at which point the captures can longer
> > > +be accessed.
> > > +
> > > +```cpp
> > > + co_await seastar::yield().then(seastar::coroutine::lambda([captures] () -> future<> {
> > > + co_await seastar::coroutine::maybe_yield();
> > > + // Can use `captures` here safely.
> > > + }));
> > > +```
> > Isn't this also fragile to use? Write 'return' instead of first co_await
> > and the code breaks.
>
>
> It will not compile (assuming the outer function remains a coroutine).
>

Why will it not compile?

>
>
> > And I imagine if someone will co-routinize such
> > code he will write it as:
> >
> > co_await seastar::yield();
> > co_await seastar::coroutine::maybe_yield();
> >
> > Usually if continuations are left in such co-routinezed the purpose
> > is to have small background fiber. Which is exactly what this
> > seastar::coroutine::lambda does not support. Isn't it just simple to
> > advice to either co-routinize entire code or use the method below?
>
>
> I used future::then() as the simplest API that accepts a coroutine.
> Typically it will be more complicated APIs, like
> seastar::parallel_for_each().
>

I do not think seastar::parallel_for_each() has the problem (otherwise
at least this code is broken [1]).

>
> We're adding coroutine versions of them, but we don't have everything yet.
>

coroutine version of parallel_for_each is about allocation optimization
as far as I see.

[1] https://github.com/scylladb/scylladb/blob/ff4430d8ea1f6dfc38c43add38822fdefd773f1a/service/raft/raft_group0.cc#L250

--
Gleb.

[Avi Kivity]

On 08/09/2022 11.45, Gleb Natapov wrote:
> On Thu, Sep 08, 2022 at 11:25:56AM +0300, Avi Kivity wrote:
>> On 08/09/2022 10.57, Gleb Natapov wrote:
>>> +
>>>> +The solution is to avoid copying or moving the lambda into
>>>> +the memory area managed by `seastar::future::then()`. Instead,
>>>> +the lambda spends its life as a temporary. We then rely on C++
>>>> +temporary lifetime extension rules to extend its life until the
>>>> +future returned is ready, at which point the captures can longer
>>>> +be accessed.
>>>> +
>>>> +```cpp
>>>> + co_await seastar::yield().then(seastar::coroutine::lambda([captures] () -> future<> {
>>>> + co_await seastar::coroutine::maybe_yield();
>>>> + // Can use `captures` here safely.
>>>> + }));
>>>> +```
>>> Isn't this also fragile to use? Write 'return' instead of first co_await
>>> and the code breaks.
>>
>> It will not compile (assuming the outer function remains a coroutine).
>>
> Why will it not compile?

Because return is illegal in a coroutine.


Anyway, this doesn't attempt to fix every way C++ will make your code
not work, just one of them.

>
>>
>>> And I imagine if someone will co-routinize such
>>> code he will write it as:
>>>
>>> co_await seastar::yield();
>>> co_await seastar::coroutine::maybe_yield();
>>>
>>> Usually if continuations are left in such co-routinezed the purpose
>>> is to have small background fiber. Which is exactly what this
>>> seastar::coroutine::lambda does not support. Isn't it just simple to
>>> advice to either co-routinize entire code or use the method below?
>>
>> I used future::then() as the simplest API that accepts a coroutine.
>> Typically it will be more complicated APIs, like
>> seastar::parallel_for_each().
>>
> I do not think seastar::parallel_for_each() has the problem (otherwise
> at least this code is broken [1]).

I see it doesn't, it captures its Func by reference. But
with_semaphore() does.

>> We're adding coroutine versions of them, but we don't have everything yet.
>>
> coroutine version of parallel_for_each is about allocation optimization
> as far as I see.

The coroutine versions of concurrency primitives started by being about
improving efficiency, but now also gained the role of being safer to use
with lambda coroutines.

[Gleb Natapov]

On Thu, Sep 08, 2022 at 11:57:06AM +0300, Avi Kivity wrote:
>
> On 08/09/2022 11.45, Gleb Natapov wrote:
> > On Thu, Sep 08, 2022 at 11:25:56AM +0300, Avi Kivity wrote:
> > > On 08/09/2022 10.57, Gleb Natapov wrote:
> > > > +
> > > > > +The solution is to avoid copying or moving the lambda into
> > > > > +the memory area managed by `seastar::future::then()`. Instead,
> > > > > +the lambda spends its life as a temporary. We then rely on C++
> > > > > +temporary lifetime extension rules to extend its life until the
> > > > > +future returned is ready, at which point the captures can longer
> > > > > +be accessed.
> > > > > +
> > > > > +```cpp
> > > > > + co_await seastar::yield().then(seastar::coroutine::lambda([captures] () -> future<> {
> > > > > + co_await seastar::coroutine::maybe_yield();
> > > > > + // Can use `captures` here safely.
> > > > > + }));
> > > > > +```
> > > > Isn't this also fragile to use? Write 'return' instead of first co_await
> > > > and the code breaks.
> > >
> > > It will not compile (assuming the outer function remains a coroutine).
> > >
> > Why will it not compile?
>
>
> Because return is illegal in a coroutine.
>

What I meant is this:

return seastar::yield().then(seastar::coroutine::lambda([captures] () -> future<> {
co_await seastar::coroutine::maybe_yield();
// Cannot use `captures` here safely.
}));

Ah, you say that you assume the outer function remains a coroutine.
Well, this is big assumption :)

>
> Anyway, this doesn't attempt to fix every way C++ will make your code not
> work, just one of them.

I understand. But my concern is the API growth. Why not recommend one
way to fix them all instead of introducing yet one more API function
that helps only in some cases and still require deep understanding of
the internals?

>
>
> >
> > >
> > > > And I imagine if someone will co-routinize such
> > > > code he will write it as:
> > > >
> > > > co_await seastar::yield();
> > > > co_await seastar::coroutine::maybe_yield();
> > > >
> > > > Usually if continuations are left in such co-routinezed the purpose
> > > > is to have small background fiber. Which is exactly what this
> > > > seastar::coroutine::lambda does not support. Isn't it just simple to
> > > > advice to either co-routinize entire code or use the method below?
> > >
> > > I used future::then() as the simplest API that accepts a coroutine.
> > > Typically it will be more complicated APIs, like
> > > seastar::parallel_for_each().
> > >
> > I do not think seastar::parallel_for_each() has the problem (otherwise
> > at least this code is broken [1]).
>
>
> I see it doesn't, it captures its Func by reference. But with_semaphore()
> does.
>
>
> > > We're adding coroutine versions of them, but we don't have everything yet.
> > >
> > coroutine version of parallel_for_each is about allocation optimization
> > as far as I see.
>
>
> The coroutine versions of concurrency primitives started by being about
> improving efficiency, but now also gained the role of being safer to use
> with lambda coroutines.
>
>
> > [1] https://github.com/scylladb/scylladb/blob/ff4430d8ea1f6dfc38c43add38822fdefd773f1a/service/raft/raft_group0.cc#L250
> >
> > --
> > Gleb.

--
Gleb.

[Avi Kivity]

Not really. I want to encourage all-or-nothing approach to coroutines
where either everything is converted, or nothing is, so we only have to
thing in one paradigm. Of course there are exceptions to the rule, but
if you choose to work differently you have to be more careful. The goal
here is to provide a solution for the common case.


Maybe this example is bad since it violates the rule. But the idea here
is to provide glue between lambda coroutines used in pre-coroutine
seastar primitives within an outer coroutine.


If you want the outer function not to be a coroutine, you have to write
it as


   auto inner_coroutine = [captures] -> future<>() { ... };

   return do_with(std::move(inner_coroutine), [] (auto& inner_coroutine) {

         return seastar::yield().then(std::ref(inner_cooroutine));

   });


Just like you would with anything else that requires a stable lifetime.
Of course you'd just convert the outer function to a coroutine too.

>
>
>> Anyway, this doesn't attempt to fix every way C++ will make your code not
>> work, just one of them.
> I understand. But my concern is the API growth. Why not recommend one
> way to fix them all instead of introducing yet one more API function
> that helps only in some cases and still require deep understanding of
> the internals?

I don't know of one way to fix everything.


I don't think this requires deep understanding of the internals. The
rules are simple:


1. Are you using a lambda coroutine within an outer coroutine? Are you
co_awaiting everything immediately?

2. If not, then this doesn't apply, you're doing something complicated
(or not using coroutines)

3. If yes, you're on the common path. Is the seastar primitive you're
using the inner lambda coroutine with marked as safe for coroutines?

4. If yes, congratulations, done.

5. If not, use the coroutine::lambda wrapper to bridge the gap between
primitives that accept continuations and coroutines.

[Niek Bouman]

very nice solution;

congrats with finding it!

...and superior to an alternative wrapper that we sometimes use in our codebase which has non-negligible overhead (as it causes an additional allocation).

and thanks for adding guidance about this topic to the tutorial !

best,

Niek

[Avi Kivity]

Review bump

[Tomasz Grabiec]

LGTM

[Avi Kivity]

Then maybe merge?

[Commit Bot]

From: Avi Kivity' via seastar-dev <seast...@googlegroups.com>
Committer: Tomasz Grabiec <tgra...@scylladb.com>
Branch: master

coroutine: explain and mitigate the lambda coroutine fiasco

Due to the way lambda coroutines are defined and how Seastar
continuations work, it is easy to have a use-after-free with lambda
coroutines.

Fortunately, it is easy to mitigate.

This patch documents the problem, provides a lambda coroutine wrapper
and describes how to use it, and documents APIs that are safe for lambda
coroutines without the wrapper.

A unit test is included.

Message-Id: <2022090715243...@scylladb.com>

---
diff --git a/doc/lambda-coroutine-fiasco.md b/doc/lambda-coroutine-fiasco.md
--- a/doc/lambda-coroutine-fiasco.md

--- a/doc/tutorial.md
+++ b/doc/tutorial.md
@@ -372,6 +372,39 @@ In #4, we call a function that returns a `seastar::future<>`. In this case, the

diff --git a/include/seastar/core/coroutine.hh b/include/seastar/core/coroutine.hh
--- a/include/seastar/core/coroutine.hh
+++ b/include/seastar/core/coroutine.hh
@@ -225,6 +225,41 @@ template<typename T> struct SEASTAR_NODISCARD without_preemption_check<T> : publ

diff --git a/include/seastar/coroutine/all.hh b/include/seastar/coroutine/all.hh
--- a/include/seastar/coroutine/all.hh
+++ b/include/seastar/coroutine/all.hh
@@ -110,6 +110,8 @@ using value_tuple_for_non_void_futures = typename value_tuple_for_non_void_futur

/// co_return a + b;
/// };
/// ```
+///
+/// Safe for use with lambda coroutines.
template <typename... Futures>
requires (sizeof ...(Futures) > 0)
class [[nodiscard("must co_await an all() object")]] all {

diff --git a/include/seastar/coroutine/parallel_for_each.hh b/include/seastar/coroutine/parallel_for_each.hh
--- a/include/seastar/coroutine/parallel_for_each.hh
+++ b/include/seastar/coroutine/parallel_for_each.hh
@@ -51,6 +51,8 @@ namespace seastar::coroutine {

/// co_return sum;
/// };
/// ```
+///
+/// Safe for use with lambda coroutines.
template <typename Func>
// constaints for Func are defined at the parallel_for_each constructor
class [[nodiscard("must co_await an parallel_for_each() object")]] parallel_for_each final : continuation_base<> {

diff --git a/tests/unit/coroutines_test.cc b/tests/unit/coroutines_test.cc
--- a/tests/unit/coroutines_test.cc
+++ b/tests/unit/coroutines_test.cc
@@ -894,4 +894,31 @@ SEASTAR_TEST_CASE(test_async_generator_throws_from_consumer_unbuffered) {

[Avi Kivity]

Author name got corrupted by googlegroups. Please use
scripts/apply-email from scylladb.git in the future.

[Commit Bot]

From: Avi Kivity' via seastar-dev <seast...@googlegroups.com>

Committer: Avi Kivity <a...@scylladb.com>
Branch: branch-5.1

coroutine: explain and mitigate the lambda coroutine fiasco

Due to the way lambda coroutines are defined and how Seastar
continuations work, it is easy to have a use-after-free with lambda
coroutines.

Fortunately, it is easy to mitigate.

This patch documents the problem, provides a lambda coroutine wrapper
and describes how to use it, and documents APIs that are safe for lambda
coroutines without the wrapper.

A unit test is included.

Message-Id: <2022090715243...@scylladb.com>

(cherry picked from commit 601e0776c088c5fda33c868ac1ac20a792417c93)

---
diff --git a/doc/lambda-coroutine-fiasco.md b/doc/lambda-coroutine-fiasco.md
--- a/doc/lambda-coroutine-fiasco.md

--- a/doc/tutorial.md
+++ b/doc/tutorial.md
@@ -372,6 +372,39 @@ In #4, we call a function that returns a `seastar::future<>`. In this case, the

diff --git a/include/seastar/core/coroutine.hh b/include/seastar/core/coroutine.hh
--- a/include/seastar/core/coroutine.hh
+++ b/include/seastar/core/coroutine.hh
@@ -225,6 +225,41 @@ template<typename T> struct SEASTAR_NODISCARD without_preemption_check<T> : publ

diff --git a/include/seastar/coroutine/all.hh b/include/seastar/coroutine/all.hh
--- a/include/seastar/coroutine/all.hh
+++ b/include/seastar/coroutine/all.hh
@@ -110,6 +110,8 @@ using value_tuple_for_non_void_futures = typename value_tuple_for_non_void_futur

/// co_return a + b;
/// };
/// ```
+///
+/// Safe for use with lambda coroutines.
template <typename... Futures>
requires (sizeof ...(Futures) > 0)
class [[nodiscard("must co_await an all() object")]] all {

diff --git a/include/seastar/coroutine/parallel_for_each.hh b/include/seastar/coroutine/parallel_for_each.hh
--- a/include/seastar/coroutine/parallel_for_each.hh
+++ b/include/seastar/coroutine/parallel_for_each.hh
@@ -51,6 +51,8 @@ namespace seastar::coroutine {

/// co_return sum;
/// };
/// ```
+///
+/// Safe for use with lambda coroutines.
template <typename Func>
// constaints for Func are defined at the parallel_for_each constructor
class [[nodiscard("must co_await an parallel_for_each() object")]] parallel_for_each final : continuation_base<> {

diff --git a/tests/unit/coroutines_test.cc b/tests/unit/coroutines_test.cc
--- a/tests/unit/coroutines_test.cc
+++ b/tests/unit/coroutines_test.cc
@@ -894,4 +894,31 @@ SEASTAR_TEST_CASE(test_async_generator_throws_from_consumer_unbuffered) {