Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resume_agile can be stored in a variable, resulting in hilarity #1357

Closed
oldnewthing opened this issue Sep 13, 2023 · 0 comments · Fixed by #1358
Closed

resume_agile can be stored in a variable, resulting in hilarity #1357

oldnewthing opened this issue Sep 13, 2023 · 0 comments · Fixed by #1358

Comments

@oldnewthing
Copy link
Member

Version

#1356

Summary

#1356 introduces resume_agile, but there is a design flaw. It saves the awaitable as a reference, expecting the usage to be

co_await resume_agile(DoSomething());

However, the caller might do this:

auto op = resume_agile(DoSomething());
co_await op;

This might happen when the developer is breaking down a complex expression into smaller pieces to help narrow down the source of a problem.

Storing the awaitable as a reference leads to a UAF in this case. We need to store the awaitable as a strong reference in the case where we are not preserving COM context. To avoid introducing unnecessary virtual calls to AddRef and Release, the argument to resume_agile can be forwarded into the await_adapter.

There is another defect in resume_agile if somebody awaits twice. This is not legal, but we should allow the illegal_delegate_assignment exception to propagate normally rather than triggering UB. A second call to await_suspend (newly possible thanks to resume_agile) would not set suspending back to true, causing the disconnect_aware_handler to try to resume the coroutine when the awaiter is destructed. But the coroutine is already going to be resumed by the C++ infrastructure as part of the "an exception was thrown from await_suspend" recovery. This results in double-resumption and hijinx ensue.

The fix is to set suspending to true at the start of await_suspend. This extra step is present only in the resume_agile code path. Throughout, we made sure to have no code generation effect on traditional co_await IAsyncSomething.

Reproducible example

auto op = resume_agile(DoSomething());
co_await op; // should not crash
co_await op; // should not exhibit UB

Expected behavior

First co_await awaits DoSomething() and resumes in any apartment. Second co_await throws illegal_delegate_assignment which can be caught by standard means.

Actual behavior

First co_await crashes. Second one double-resumes the coroutine and results in UB.

Additional comments

No response

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant