[Bug c++/95111] coroutines use-after-free with lambdas
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 Eric Gallager 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 --- (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
[Bug c++/95111] coroutines use-after-free with lambdas
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 Avi Kivity changed: What|Removed |Added Resolution|--- |INVALID Status|UNCONFIRMED |RESOLVED --- Comment #23 from Avi Kivity --- The bug is in the C++ standard, not gcc.
[Bug c++/95111] coroutines use-after-free with lambdas
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 --- Comment #22 from Avi Kivity --- Certainly, closing as invalid.
[Bug c++/95111] coroutines use-after-free with lambdas
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 --- Comment #21 from Iain Sandoe --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 --- Comment #20 from Avi Kivity --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111
--- Comment #19 from Iain Sandoe ---
(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 {};
> > >
> > > 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 --- Comment #18 from Avi Kivity --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111
--- Comment #17 from Ville Voutilainen ---
(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 {};
> >
> > 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111
Ville Voutilainen changed:
What|Removed |Added
CC||ville.voutilainen at gmail dot
com
--- Comment #16 from Ville Voutilainen ---
(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 {};
>
> 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111
--- Comment #15 from Avi Kivity ---
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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111
--- Comment #14 from Iain Sandoe ---
(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 {};
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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 --- Comment #13 from Avi Kivity --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 --- Comment #12 from Ville Voutilainen --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 --- Comment #11 from Avi Kivity --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111
--- Comment #10 from Avi Kivity ---
Well, the standard is useless here.
In
[foo] () -> lazy { 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 Iain Sandoe changed: What|Removed |Added Assignee|unassigned at gcc dot gnu.org |iains at gcc dot gnu.org --- Comment #9 from Iain Sandoe --- (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 --- Comment #7 from Avi Kivity --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 --- Comment #8 from Avi Kivity --- Created attachment 48526 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48526=edit less lame testcase
[Bug c++/95111] coroutines use-after-free with lambdas
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 --- Comment #6 from Iain Sandoe --- (In reply to Avi Kivity from comment #5) > This snippet from cppreference: > > If the coroutine is a non-static member function, such as task > my_class::method1(int x) const;, its Promise type is > std::coroutine_traits, 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 --- Comment #5 from Avi Kivity --- This snippet from cppreference: If the coroutine is a non-static member function, such as task my_class::method1(int x) const;, its Promise type is std::coroutine_traits, 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 --- Comment #4 from Iain Sandoe --- (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 --- Comment #3 from Avi Kivity --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 --- Comment #2 from Avi Kivity --- Created attachment 48524 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48524=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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111 --- Comment #1 from Iain Sandoe --- 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).
