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.
Version
1356
Summary
1356 introduces
resume_agile, but there is a design flaw. It saves the awaitable as a reference, expecting the usage to beHowever, the caller might do this:
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_agilecan be forwarded into theawait_adapter.There is another defect in
resume_agileif somebody awaits twice. This is not legal, but we should allow theillegal_delegate_assignmentexception to propagate normally rather than triggering UB. A second call toawait_suspend(newly possible thanks toresume_agile) would not setsuspendingback totrue, causing thedisconnect_aware_handlerto 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 fromawait_suspend" recovery. This results in double-resumption and hijinx ensue.The fix is to set
suspendingtotrueat the start ofawait_suspend. This extra step is present only in theresume_agilecode path. Throughout, we made sure to have no code generation effect on traditionalco_await IAsyncSomething.Reproducible example
Expected behavior
First
co_awaitawaitsDoSomething()and resumes in any apartment. Secondco_awaitthrowsillegal_delegate_assignmentwhich can be caught by standard means.Actual behavior
First
co_awaitcrashes. Second one double-resumes the coroutine and results in UB.Additional comments
No response