[Bug c++/95111] New: coroutines use-after-free with lambdas

[Bug c++/95111] New: coroutines use-after-free with lambdas

[avi@cloudius-systems.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

            Bug ID: 95111
           Summary: coroutines use-after-free with lambdas
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: avi@cloudius-systems.com
  Target Milestone: ---

coroutines copy their input to the coroutine frame, so a coroutine like

    future<T> f(T x) {
        co_await something();
        co_return x;
    }


will copy x back from the coroutine frame. However, lambdas are passed by
pointer, so something like


    [x] () -> future<T> {
        co_await something();
        co_return x;
    }

will fail, it is translated as something like


    struct lambda {
        T x;
    }

    future<T> lambda_operator_parens(const lambda* l) {
        co_await something();
        co_return l->x;
    }

Since l is captured by value, *l is dangling and is leaked.


I think the following translation would be more useful:


    future<T> lambda_operator_parens_rref(const lambda l) {
        co_await something();
        co_return l.x;
    }

l would be copied by value and would survive copying/moving into the coroutine
frame.

I don't know if the current behavior is mandated by the standard, but if it is,
it seems a serious defect.

[Bug c++/95111] coroutines use-after-free with lambdas

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #1 from Iain Sandoe <iains at gcc dot gnu.org> ---
There are some gotchas with coroutines and references (both regular and
rvalue).

* there could still be a bug here, so I want to double-check.

Please could you expand your snippets of code into a small test-case that would
would expect to work?

(FWIW, in my original impl. I tried to be more defensive about lambda captures
- but that wasn't correct in the presence of a mutable lambda - and didn't
agree with other implementations - or the collective understanding of the
intent of the standard - so I backed that out).

[Bug c++/95111] coroutines use-after-free with lambdas

[avi@cloudius-systems.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #2 from Avi Kivity <avi@cloudius-systems.com> ---
Created attachment 48524
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48524&action=edit
lame testcase

Lame testcase that shows that the lambda is passed as a pointer rather than by
value, leading to a leak if the coroutine is suspended.

[Bug c++/95111] coroutines use-after-free with lambdas

[avi@cloudius-systems.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #3 from Avi Kivity <avi@cloudius-systems.com> ---
The test case I uploaded only shows the failure, it won't work if gcc worked as
I expect it. I'll try to get a better testcase, unfortunately a small coroutine
testcase is still some work.

[Bug c++/95111] coroutines use-after-free with lambdas

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #4 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Avi Kivity from comment #3)
> The test case I uploaded only shows the failure, it won't work if gcc worked
> as I expect it. I'll try to get a better testcase, unfortunately a small
> coroutine testcase is still some work.

yeah, I have some boiler-plate code in headers in the GCC test-suite that can
help.

[Bug c++/95111] coroutines use-after-free with lambdas

[avi@cloudius-systems.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #5 from Avi Kivity <avi@cloudius-systems.com> ---
This snippet from cppreference:

  If the coroutine is a non-static member function, such as task<void> 
  my_class::method1(int x) const;, its Promise type is 
  std::coroutine_traits<task<void>, const my_class&, int>::promise_type

implies that both gcc and my interpretation are wrong. gcc passes a pointer for
the lambda, and I'd like the lambda to be passed by value. The difference
between gcc and cppreference is trivial, and both of them make coroutine
lambdas unusable if they contain state and if the lambda is asynchronous.

[Bug c++/95111] coroutines use-after-free with lambdas

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #6 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Avi Kivity from comment #5)
> This snippet from cppreference:
> 
>   If the coroutine is a non-static member function, such as task<void> 
>   my_class::method1(int x) const;, its Promise type is 
>   std::coroutine_traits<task<void>, const my_class&, int>::promise_type
> 
> implies that both gcc and my interpretation are wrong. gcc passes a pointer
> for the lambda, and I'd like the lambda to be passed by value. The
> difference between gcc and cppreference is trivial, and both of them make
> coroutine lambdas unusable if they contain state and if the lambda is
> asynchronous.

( assuming that this is the problem, I haven't yet had a chance to check *
could still be a bug ;) ).

I have a pending change to pass a reference to the lambda object to the traits,
promise preview and allocator lookups.

This was a source of considerable discussion amongst the implementors (GCC,
clang, MSVC) about how the std should be interpreted.  The change I mention
will make the lambda capture object pointer behave in the same manner as 'this'
(which was the intended behaviour as determined by the long discussion).  MSVC
implements this now, and clang will be changing to match it also.

That won't solve any lifetime issue with the capture object - you will still
need to ensure that it exists for the duration of the coroutine * as you would
for a class instance with a method coroutine *.

We (at least several of us) agree that this is a source of easy programming
errors - and I expect there to be some revisiting in c++23.  That's a
considerable challenge in the face of a mutable lambda - maybe (thinking aloud)
needs something like Apple blocks, but with an automatic promotion of the
closure to a heap object if it escapes the creating scope.

[Bug c++/95111] coroutines use-after-free with lambdas

[avi@cloudius-systems.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #7 from Avi Kivity <avi@cloudius-systems.com> ---
I have a simple reproducer. A lambda fails while a fake lambda using structs
passes. I don't think gcc is at fault, but the standard is problematic here
IMO.

[Bug c++/95111] coroutines use-after-free with lambdas

[avi@cloudius-systems.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #8 from Avi Kivity <avi@cloudius-systems.com> ---
Created attachment 48526
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48526&action=edit
less lame testcase

[Bug c++/95111] coroutines use-after-free with lambdas

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

Iain Sandoe <iains at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |iains at gcc dot gnu.org

--- Comment #9 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Avi Kivity from comment #8)
> Created attachment 48526 [details]
> less lame testcase

thanks, it's in my queue to look at.

FWIW see the note in bullet 13 here:
http://eel.is/c++draft/dcl.fct.def.coroutine#13

consider also:
gcc/testsuite/g++.dg/coroutines/torture/lambda-10-mutable.C

where, if the closure object were copied into the coroutine frame, the
operation would be incorrect.

[Bug c++/95111] coroutines use-after-free with lambdas

[avi@cloudius-systems.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #10 from Avi Kivity <avi@cloudius-systems.com> ---
Well, the standard is useless here.

In

    [foo] () -> lazy<foo> { co_return foo; } ()

a temporary is clearly passed to the lambda body, yet the standard mandates
that we capture it by reference. As a result, a use-after-free is guaranteed.

[Bug c++/95111] coroutines use-after-free with lambdas

[avi@cloudius-systems.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #11 from Avi Kivity <avi@cloudius-systems.com> ---
I started a conversation on the std-proposals list about this.

Meanwhile, how about a -fnonstandard-coroutines-that-actually-work flag that
captures the parameter to a non-static member function coroutine by value, if
it is an rvalue when the call happens? Without it, lambda coroutines are
useless for asynchronous coroutines.

[Bug c++/95111] coroutines use-after-free with lambdas

[ville.voutilainen at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #12 from Ville Voutilainen <ville.voutilainen at gmail dot com> ---
It sure seems to me that a coroutine lambda's captures should be copied to the
coroutine state. I don't think the standard says that anywhere.

[Bug c++/95111] coroutines use-after-free with lambdas

[avi@cloudius-systems.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #13 from Avi Kivity <avi@cloudius-systems.com> ---
Yes. gcc has a minor bug in that the lambda is reflected as a pointer instead
of a reference in coroutine_traits. The major bug is in the standard.

[Bug c++/95111] coroutines use-after-free with lambdas

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #14 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Ville Voutilainen from comment #12)
> It sure seems to me that a coroutine lambda's captures should be copied to
> the coroutine state. I don't think the standard says that anywhere.

Maybe I am missing your point (some of these things were decided long before I
joined the fray)

----

Well, the standard is pretty much silent on coros + lambdas.  However, the
implementors (Richard, Gor, me, et al) had a long discussion on the topic
before Prague - and took what we could from that to Core.

GCC does not comply with the (agreed in that discussion) intent that the
capture object should be treated in the same manner as 'this', and a reference
passed to the traits lookup, promise parms preview and allocator lookup.  I
have a patch for this (will post this week) - but the only implementation with
this correct so far is MSVC
clang passes a ref to the traits but does not pass anything for the closure
object pointer to promise param preview or allocator)
GCC currently passes the object pointer to all three.

====

The idea of bringing the lambda's captures into the coro frame was what I
originally implemented.  Richard pointed out that this is wrong when the lambda
is mutable (see gcc/testsuite/g++.dg/coroutines/torture/lambda-10-mutable.C)

so if one has

auto X = [...] () -> some_coro<xxx> {};

X must exist for the duration of the lambda coro [it was pointed out by Lewis
that really this is only the same as saying that if you have a class with a
member function lambda, the instance of that class has to persist for the
duration of the coro].

We are, I believe, collectively agreed that this is a 'foot gun' (and rvalue
refs doubly so); however, making a better mousetrap here might require some
considerable thought.

I'm happy to be educated if there's some different consensus as to what to do,
and to amend the GCC impl. accordingly.

[Bug c++/95111] coroutines use-after-free with lambdas

[avi@cloudius-systems.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #15 from Avi Kivity <avi@cloudius-systems.com> ---
I believe that my suggestion works for mutable lambdas (and for any coroutine
called as a member function):

 - if the object passeed to the member function is an lvalue, then the
coroutine captures a reference to the object
 - if the object passed to the member function is an rvalue, then the coroutine
captures the object by copying it (typically using it move constructor).

Examples:

   auto a = [blah] () mutable -> task<> { ... };
   a();   // a is captured by reference

   [blah] () mutable -> task<> { ... } (); // captured by value


   my_class a;
   a.some_coroutine();  // captured by reference


   my_class a;
   std::move(a).some_coroutine();   // captured by value


Currently, the "captured by value" cases are captured by reference, and cause a
use-after-free if the coroutine outlives the caller scope.

[Bug c++/95111] coroutines use-after-free with lambdas

[ville.voutilainen at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

Ville Voutilainen <ville.voutilainen at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ville.voutilainen at gmail dot com

--- Comment #16 from Ville Voutilainen <ville.voutilainen at gmail dot com> ---
(In reply to Iain Sandoe from comment #14)
> (In reply to Ville Voutilainen from comment #12)
> The idea of bringing the lambda's captures into the coro frame was what I
> originally implemented.  Richard pointed out that this is wrong when the
> lambda is mutable (see
> gcc/testsuite/g++.dg/coroutines/torture/lambda-10-mutable.C)
> 
> so if one has
> 
> auto X = [...] () -> some_coro<xxx> {};
> 
> X must exist for the duration of the lambda coro [it was pointed out by
> Lewis that really this is only the same as saying that if you have a class
> with a member function lambda, the instance of that class has to persist for
> the duration of the coro].

Ah. So the work-around for this problem is to copy the capture to a local
variable, and co_return that; then the local variable is in the coro-state.
Right?

[Bug c++/95111] coroutines use-after-free with lambdas

[ville.voutilainen at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #17 from Ville Voutilainen <ville.voutilainen at gmail dot com> ---
(In reply to Ville Voutilainen from comment #16)
> (In reply to Iain Sandoe from comment #14)
> > (In reply to Ville Voutilainen from comment #12)
> > The idea of bringing the lambda's captures into the coro frame was what I
> > originally implemented.  Richard pointed out that this is wrong when the
> > lambda is mutable (see
> > gcc/testsuite/g++.dg/coroutines/torture/lambda-10-mutable.C)
> > 
> > so if one has
> > 
> > auto X = [...] () -> some_coro<xxx> {};
> > 
> > X must exist for the duration of the lambda coro [it was pointed out by
> > Lewis that really this is only the same as saying that if you have a class
> > with a member function lambda, the instance of that class has to persist for
> > the duration of the coro].
> 
> Ah. So the work-around for this problem is to copy the capture to a local
> variable, and co_return that; then the local variable is in the coro-state.
> Right?

That is, instead of writing

[x] {co_return x;}

write

[x] {auto xx = x; co_return xx;}

[Bug c++/95111] coroutines use-after-free with lambdas

[avi@cloudius-systems.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #18 from Avi Kivity <avi@cloudius-systems.com> ---
The work-around works if initial_suspend() returns suspend_never or similar. If
the lambda is suspended before execution, the reference may dangle.

[Bug c++/95111] coroutines use-after-free with lambdas

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #19 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Ville Voutilainen from comment #17)
> (In reply to Ville Voutilainen from comment #16)
> > (In reply to Iain Sandoe from comment #14)
> > > (In reply to Ville Voutilainen from comment #12)
> > > The idea of bringing the lambda's captures into the coro frame was what I
> > > originally implemented.  Richard pointed out that this is wrong when the
> > > lambda is mutable (see
> > > gcc/testsuite/g++.dg/coroutines/torture/lambda-10-mutable.C)
> > > 
> > > so if one has
> > > 
> > > auto X = [...] () -> some_coro<xxx> {};
> > > 
> > > X must exist for the duration of the lambda coro [it was pointed out by
> > > Lewis that really this is only the same as saying that if you have a class
> > > with a member function lambda, the instance of that class has to persist for
> > > the duration of the coro].
> > 
> > Ah. So the work-around for this problem is to copy the capture to a local
> > variable, and co_return that; then the local variable is in the coro-state.
> > Right?
> 
> That is, instead of writing
> 
> [x] {co_return x;}
> 
> write
> 
> [x] {auto xx = x; co_return xx;}

hmm I'm not sure this is sufficient; if the initial suspend is suspend-always,
and the closure object goes away before the initial resume, then xx will be
initialised with garbage.

[Bug c++/95111] coroutines use-after-free with lambdas

[avi@cloudius-systems.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #20 from Avi Kivity <avi@cloudius-systems.com> ---
My coroutines do return suspend_never from initial_suspend(); so thanks for the
workaround, I'll use it until we have a better fix.

[Bug c++/95111] coroutines use-after-free with lambdas

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #21 from Iain Sandoe <iains at gcc dot gnu.org> ---
Avi, If we are agreed that there is no GCC bug here (the change from pointer to
reference is already in the queue)

I would suggest that new design discussion would be better by putting a paper
or suggestions to the WG21 evolution list (and certainly involving folks who
are not present 'here').  In which case the bug could be closed.

[Bug c++/95111] coroutines use-after-free with lambdas

[avi@cloudius-systems.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

--- Comment #22 from Avi Kivity <avi@cloudius-systems.com> ---
Certainly, closing as invalid.

[Bug c++/95111] coroutines use-after-free with lambdas

[avi@cloudius-systems.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

Avi Kivity <avi@cloudius-systems.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #23 from Avi Kivity <avi@cloudius-systems.com> ---
The bug is in the C++ standard, not gcc.

[Bug c++/95111] coroutines use-after-free with lambdas

[egallager at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111

Eric Gallager <egallager at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=78352
             Blocks|                            |54367
                 CC|                            |egallager at gcc dot gnu.org

--- Comment #24 from Eric Gallager <egallager at gcc dot gnu.org> ---
(In reply to Iain Sandoe from comment #6)
> 
> We (at least several of us) agree that this is a source of easy programming
> errors - and I expect there to be some revisiting in c++23.  That's a
> considerable challenge in the face of a mutable lambda - maybe (thinking
> aloud) needs something like Apple blocks, but with an automatic promotion of
> the closure to a heap object if it escapes the creating scope.

...(Apple blocks support is bug 78352, for reference)...


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54367
[Bug 54367] [meta-bug] lambda expressions