You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
#1356 introduces resume_agile, but there is a design flaw. It saves the awaitable as a reference, expecting the usage to be
co_awaitresume_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 crashco_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
The text was updated successfully, but these errors were encountered:
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_agile
can be forwarded into theawait_adapter
.There is another defect in
resume_agile
if somebody awaits twice. This is not legal, but we should allow theillegal_delegate_assignment
exception to propagate normally rather than triggering UB. A second call toawait_suspend
(newly possible thanks toresume_agile
) would not setsuspending
back totrue
, causing thedisconnect_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 fromawait_suspend
" recovery. This results in double-resumption and hijinx ensue.The fix is to set
suspending
totrue
at the start ofawait_suspend
. This extra step is present only in theresume_agile
code path. Throughout, we made sure to have no code generation effect on traditionalco_await IAsyncSomething
.Reproducible example
Expected behavior
First
co_await
awaitsDoSomething()
and resumes in any apartment. Secondco_await
throwsillegal_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
The text was updated successfully, but these errors were encountered: