https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95111#c23 GCC Bugzilla - Bug 95111 coroutines use-after-free with lambdas Last modified: 2022-03-11 00:32:28 UTC * Home * | New * | Browse * | Search * | [ ] [Search] [?] * | Reports * * | Help * | New Account * | Log In [ ] [ ] [*] Remember [Log in] [x] * | Forgot Password Login: [ ] [Reset Password] [x] Bug 95111 - coroutines use-after-free with lambdas Summary: coroutines use-after-free with lambdas Reported: 2020-05-13 17:36 UTC by Avi Kivity Modified: 2022-03-11 00:32 UTC ( Status: RESOLVED History) INVALID 7 users (show) Alias: None Product: gcc Classification: Unclassified [ ]egallager Component: c++ (show other CC List: [ ]egor.pugin bugs) [ ]iains Version: 10.1.1 [ ]rafael Importance: P3 normal [ ]ssbssa Target --- [ ]ville.voutilainen Milestone: [ ]webrown.cpp Assignee: Iain Sandoe See Also: * 78352 URL: Keywords: c++-lambda Host: Depends on: Target: Blocks: lambdas Build: Show dependency Known to tree / graph work: Known to fail: Last reconfirmed: --------------------------------------------------------------------- Attachments lame testcase (280 bytes, text/plain) Details 2020-05-13 19:14 UTC, Avi Kivity less lame testcase (631 bytes, text/plain) Details 2020-05-13 20:11 UTC, Avi Kivity View All Add an attachment (proposed patch, testcase, etc.) Note You need to log in before you can comment on or make changes to this bug. Description Avi Kivity 2020-05-13 17:36:57 UTC coroutines copy their input to the coroutine frame, so a coroutine like future 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 { co_await something(); co_return x; } will fail, it is translated as something like struct lambda { T x; } future 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 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. Comment 1 Iain Sandoe 2020-05-13 19:12:37 UTC 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). Comment 2 Avi Kivity 2020-05-13 19:14:00 UTC Created attachment 48524 [details] 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. Comment 3 Avi Kivity 2020-05-13 19:15:14 UTC 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. Comment 4 Iain Sandoe 2020-05-13 19:21:54 UTC (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. Comment 5 Avi Kivity 2020-05-13 19:27:42 UTC 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. Comment 6 Iain Sandoe 2020-05-13 19:39:33 UTC (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. Comment 7 Avi Kivity 2020-05-13 20:10:47 UTC 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. Comment 8 Avi Kivity 2020-05-13 20:11:13 UTC Created attachment 48526 [details] less lame testcase Comment 9 Iain Sandoe 2020-05-13 20:17:03 UTC (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. Comment 10 Avi Kivity 2020-05-13 20:24:28 UTC 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. Comment 11 Avi Kivity 2020-05-14 11:15:57 UTC 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. Comment 12 Ville Voutilainen 2020-05-14 11:24:34 UTC 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. Comment 13 Avi Kivity 2020-05-14 11:29:56 UTC 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. Comment 14 Iain Sandoe 2020-05-14 11:46:53 UTC (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. Comment 15 Avi Kivity 2020-05-14 11:54:06 UTC 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. Comment 16 Ville Voutilainen 2020-05-14 11:57:00 UTC (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? Comment 17 Ville Voutilainen 2020-05-14 11:59:01 UTC (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;} Comment 18 Avi Kivity 2020-05-14 12:01:17 UTC The work-around works if initial_suspend() returns suspend_never or similar. If the lambda is suspended before execution, the reference may dangle. Comment 19 Iain Sandoe 2020-05-14 12:02:21 UTC (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. Comment 20 Avi Kivity 2020-05-14 12:02:35 UTC My coroutines do return suspend_never from initial_suspend(); so thanks for the workaround, I'll use it until we have a better fix. Comment 21 Iain Sandoe 2020-05-14 12:25:16 UTC 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. Comment 22 Avi Kivity 2020-05-14 12:33:44 UTC Certainly, closing as invalid. Comment 23 Avi Kivity 2020-05-14 12:34:53 UTC The bug is in the C++ standard, not gcc. Comment 24 Eric Gallager 2020-05-14 18:02:25 UTC (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)... --------------------------------------------------------------------- * Format For Printing * - XML * - Clone This Bug * - Top of page * + Home + | New + | Browse + | Search + | [ ] [Search] [?] + | Reports + + | Help + | New Account + | Log In [ ] [ ] [*] Remember [Log in] [x] + | Forgot Password Login: [ ] [Reset Password] [x]