[Proposal] The coding standard should ban coroutine-lambdas because they are so dangerous

[dmachaj]

I’m proposing a new rule that would go into the "other function rules" section. I did a pass over both the guidelines and didn’t see anything regarding coroutines.

This pattern is very dangerous because it looks normal like any other lambda but in fact it is super dangerous. The issue is that the coroutine itself will go out of scope after the first suspension point. This means that even value types will be operating on freed memory. Taking a strong reference to a class doesn’t help because the smart pointer itself goes away. Basically passing the values in by value instead of capturing them in the lambda makes it safe again. At that point it might as well just be a function instead of a lambda.

Additionally, when debugging one of the crashes that came from this unsafe usage it became known that the coroutine optimizer does odd things with coroutine lambdas. The result is that even with valid symbols the debugger can only dump partial call stacks. Anything prior to the coroutine lambda will fail to dump because a jump is used instead of a return statement. The result is that stack backtrace commands will just… stop after the coroutine lambda.

For all these reasons I’m proposing that we ban usage of this language construct because it is too dangerous. There are ways to make it safe (not actually awaiting, capturing by parameter) but those defeat the purpose of using a lambda. They are also vulnerable to “I’ll change this to a capture because passing by parameter is dumb” fixes in the future that would make them unsafe again.

The proposed guideline is below:

Do not use lambdas that are coroutines

Reason

Usage patterns that are correct with normal lambdas are hazardous with coroutine lambdas. The obvious pattern of capturing variables will result in accessing freed memory after the first suspension point, even for strong references and value types.

Example, Bad

const auto strongThis { get_strong() };
const auto lambda = [strongThis]() -> winrt::fire_and_forget
{
  co_await winrt::resume_background();
  // strongThis now points to freed memory, even though we took a strong reference
};
lambda();

Example, Better

const auto lambda = [](auto strongThis) -> winrt::fire_and_forget              // take as by-value parameter instead of as a capture
{
  co_await winrt::resume_background();
};
const auto strongThis { get_strong() };
lambda(strongThis); 

Example, Best

Use a function for coroutines.

[oldnewthing]

Perhaps better example would involve other parameters beyond the strong reference. Also, use standard library example instead of C++/WinRT.

---

Reason

Usage patterns that are correct with normal lambdas are hazardous with coroutine lambdas. The obvious pattern of capturing variables will result in accessing freed memory after the first suspension point, even for strong references and value types.

Example, Bad

int value = get_value();
const auto lambda = [strongThis = std::shared_from_this(this), value]() -> std::future<void>
{
  co_await something();
  // "strongThis" and "value" have already been destroyed
  // the "strong" pointer didn't accomplish anything
};
lambda();

Example, Better

int value = get_value();
const auto lambda = [](auto strongThis, auto value) -> std::future<void>  // take as by-value parameter instead of as a capture
{
  co_await something();
  // strongThis and value are still valid at this point
};
lambda(std::shared_from_this(this), value); 

Example, Best

Use a function for coroutines.

std::future<void> Class::do_something(int value)
{
  std::shared_ptr<Class> strongThis = std::shared_from_this(this);
  co_await something();
  // strongThis and value are still valid at this point
}

int value = get_value();
do_something(value); 

[jwakely]

What is a strong reference?

[jwakely]

I think the suggested guideline would be improved by explaining that in these examples the closure object is just a local variable in the surrounding block scope. The problem is that the coroutine might be resumed after automatic variables in that scope have been destroyed. Talking about the closure object going out of scope (rather than its captures) is more precise and makes it clearer for me.

I think it is possible to use coroutine lambdas if they are passed as an argument to another coroutine. In that case the closure will get captured as part of the coroutine state, but that adds extra overhead. But to make the actual problem clear I think it would be useful to explicitly talk about the closure object going out of scope because it's just a local variable.

I still don't know what a strong reference or strong pointer is though. If that's a Microsoft-ism please use different terms.

[oldnewthing]

"strong reference" is the opposite of a "weak reference": A weak reference (std::weak_ptr) does not keep the object alive. A strong reference (std::shared_ptr) does keep the object alive. It's a common term in other languages.

Yes, the underlying issue is that the lambda is destructed before the coroutine completes.

[dmachaj]

Thank you @oldnewthing for the suggestions. I was expecting a request to use more standard library types in the examples but I wasn't sure whether that would actually be requested so I did not proactively rewrite them into STL types before opening this issue.

@jwakely Do you think it would improve the clarity to add an outer { } scope around the lambdas in the examples so that it is more clear that they are going out of scope without anyone keeping them alive? I'm not sure what guidance is in place for saying things in the description vs. expressing them in code. I've been biasing towards keeping the written explanation brief so the focus is on the code examples.

[jwakely]

Yes, I think a bit more context in the examples would help, but I think saying something like "the closure object is just an automatic variable on the function's stack and goes out of scope" in words would be even more helpful.

[dmachaj]

Ok here's my attempt at incorporating feedback from both of you. I took Raymond's edits and added an extra scope on top to make it clear the lambda type is going out of scope. I also adjusted the reason a bit to explain further.

Reason

Usage patterns that are correct with normal lambdas are hazardous with coroutine lambdas. The obvious pattern of capturing variables will result in accessing freed memory after the first suspension point, even for strong references and value types.

A lambda is itself an object with storage, often on the stack, that will go out of scope at some point. When the lambda goes out of scope the captures will also go out of scope. Synchronous lambdas will have finished executing by this time so it is not a problem. Coroutine lambdas may resume from suspension after the lambda itself is freed and at that point all captures will be use-after-free memory access.

Example, Bad

int value = get_value();
{
  const auto lambda = [strongThis = std::shared_from_this(this), value]() -> std::future<void>
  {
    co_await something();
    // "strongThis" and "value" have already been destroyed
    // the "strong" pointer didn't accomplish anything
  };
  lambda();
} // the lambda closure object has now gone out of scope

##Example, Better

int value = get_value();
{
  const auto lambda = [](auto strongThis, auto value) -> std::future<void>  // take as by-value parameter instead of as a capture
  {
    co_await something();
    // strongThis and value are still valid at this point
  };
  lambda(std::shared_from_this(this), value); 
} // the lambda closure object has now gone out of scope

Example, Best

Use a function for coroutines.

std::future<void> Class::do_something(int value)
{
  std::shared_ptr<Class> strongThis = std::shared_from_this(this);
  co_await something();
  // strongThis and value are still valid at this point
}

int value = get_value();
do_something(value); 

[MikeGitb]

I think what needs clarification is where the this from std::shared_from_this(this) comes from.

[dmachaj]

Could you elaborate on what you mean by that? The lambdas in these examples are not class fields so I'm not sure how interesting the source of the this pointer is. The examples would be just as true if it was a std::shared_ptr to some generic Foo type that didn't involve a this pointer at all.

[jwakely]

I still think "strong reference" to mean shared_ptr is confusing. I'm hardly a noob at C++ but the term means nothing to me in this context.

A lambda is itself an object with storage,

No, a lambda is an expression, which creates a closure object. The closure is the object with storage.

I don't think the term "synchronous lambda" is helpful either. Maybe "normal" or "non-resumable" or "non-coroutine" lambda?

std::shared_from_this(this)

That's not valid syntax, I think you mean this->shared_from_this()

after the lambda itself is freed

In C++ "freed" usually refers to dynamic allocations, but the point here is that it's on the stack. Maybe "destroyed" instead.

In the "best" example I think you need to make it clear the last two lines are in some other function, not namespace scope following the function above.

I think this is much improved though, thanks.

[dmachaj]

Alright, let's try this again. There are no longer mention of strong references or to this. I tried to make the language more accurate based on your feedback.

Reason

Usage patterns that are correct with normal lambdas are hazardous with coroutine lambdas. The obvious pattern of capturing variables will result in accessing freed memory after the first suspension point, even for refcounted smart pointers and value types.

A lambda results in a closure object with storage, often on the stack, that will go out of scope at some point. When the closure object goes out of scope the captures will also go out of scope. Normal lambdas will have finished executing by this time so it is not a problem. Coroutine lambdas may resume from suspension after the closure object has destructed and at that point all captures will be use-after-free memory access.

Example, Bad

int value = get_value();
std::shared_ptr<Foo> sharedFoo = get_foo();
{
  const auto lambda = [value, sharedFoo]() -> std::future<void>
  {
    co_await something();
    // "sharedFoo" and "value" have already been destroyed
    // the "shared" pointer didn't accomplish anything
  };
  lambda();
} // the lambda closure object has now gone out of scope

Example, Better

int value = get_value();
std::shared_ptr<Foo> sharedFoo = get_foo();
{
  const auto lambda = [](auto sharedFoo, auto value) -> std::future<void>  // take as by-value parameter instead of as a capture
  {
    co_await something();
    // sharedFoo and value are still valid at this point
  };
  lambda(sharedFoo, value); 
} // the lambda closure object has now gone out of scope

Example, Best

Use a function for coroutines.

std::future<void> Class::do_something(int value, std::shared_ptr<Foo> sharedFoo)
{
  co_await something();
  // sharedFoo and value are still valid at this point
}

void SomeOtherFunction()
{
  int value = get_value();
  std::shared_ptr<Foo> sharedFoo = get_foo();
  do_something(value, sharedFoo); 
}

[MikeGitb]

Imho Much better

Regarding your question:

Could you elaborate on what you mean by that? The lambdas in these examples are not class fields so I'm not sure how interesting the source of the this pointer is. The examples would be just as true if it was a std::shared_ptr to some generic Foo type that didn't involve a this pointer at all.

The point is that a reader doesn't know, what is and what is not relevant in this example. For some it might be the first time they see shared_from_this.

What makes me wonder tho:

// "sharedFoo" and "value" have already been destroyed 
// the "shared" pointer didn't accomplish anything

If those values have been destroyed, why are we talking about shared ptr at all? Wouldn't accessing any captured variable after the co_await be UB anyway?

And why doesn't the example code (Good or bad) access the variables to demonstrate the actual problem? And why the heck is the standard allowing that at all?

[jwakely]

If those values have been destroyed, why are we talking about shared ptr at all? Wouldn't accessing any captured variable after the co_await be UB anyway?

Yes. But I think the reason to show it is that asynchronous code often uses shared_ptr to manage lifetimes of data shared across asynchronous calls. The point here is that the shared_ptr itself goes out of scope, so it doesn't help.

And why doesn't the example code (Good or bad) access the variables to demonstrate the actual problem?

Personally I think the comment is enough.

And why the heck is the standard allowing that at all?

How should it prevent it? Serious question. It's been discussed, a solution isn't obvious.

If the lambda is resumed before it goes out of scope, it works. Or if the lambda itself is an argument to a coroutine it gets copied into a coroutine frame and its lifetime is that of the coroutine. So simply banning resumable lambdas would be too much. That's off-topic here anyway.

[jwakely]

The latest revision looks great to me btw.

[oldnewthing]

How about instead of just commenting about the sharedFoo and value, we actually use it.

int value = get_value();
std::shared_ptr<Foo> sharedFoo = get_foo();
{
  const auto lambda = [value, sharedFoo]() -> std::future<void>
  {
    co_await something();
    // "sharedFoo" and "value" have already been destroyed.
    sharedFoo->DoAfterSomething(value); // BUG: use-after-free
    // The "shared" pointer failed to extend the lifetime of the "Foo".
  };
  lambda();
} // the lambda closure object has now gone out of scope