Skip to content

Conversation

@oldnewthing
Copy link
Member

The apartment-switching function resume_apartment_sync takes a context and a coroutine handle and resumes the coroutine in that context. The context is taken by reference, which is a problem if the coroutine resumption is going to destruct the context: We destruct the context while its call to ContextCallback is still active.

The awaiter for IAsync* captures the context into the completion handler, which is a separate object from the coroutine, so resuming the coroutine will not destruct the context.

However, the awaiter for apartment_context uses the context stored directly inside the apartment_context itself, which could be destructed as part of resumption:

fire_and_forget Something()
{
    // Delay 1 second and return to original context.
    {
        apartment_context save;
        co_await resume_after(1s);
        co_await save;
    }
    // do more stuff
}

The first thing the coroutine does on resumption from co_await save is destruct the apartment_context, which releases the context while ContextCallback is still active.

Fix by copying the context and resuming the copy. That way, the context isn't destructed until after resume_context_sync has definitely returned from ContextCallback.

We can't std::move the context out of the apartment_context because that would prevent an apartment context from being co_awaited more than once.

The apartment-switching function `resume_apartment_sync`
takes a context and a coroutine handle and resumes the
coroutine in that context. The context is taken by reference,
which is a problem if the coroutine resumption is going to
destruct the context: We destruct the context while it is
call to `ContextCallback` is still active.

The awaiter for `IAsync*` captures the context into
the completion handler, which is a separate object from
the coroutine, so resuming the coroutine will not destruct
the context.

However, the awaiter for `apartment_context` uses the context
stored directly inside the `apartment_context` itself,
which could be destructed as part of resumption:

```cpp
fire_and_forget Something()
{
    // Delay 1 second and return to original context.
    {
        apartment_context save;
        co_await resume_after(1s);
        co_await save;
    }
    // do more stuff
}
```

The first thing the coroutine does on resumption from
`co_await save` is destruct the `apartment_context`,
which releases the `context` while `ContextCallback`
is still active.

Fix by copying the context and resuming the copy.
That way, the context isn't destructed until after
`resume_context_sync` has definitely returned from
`ContextCallback`.

We can't `std::move` the context out of the `apartment_context`
because that would prevent an apartment context from being
`co_await`ed more than once.
@oldnewthing
Copy link
Member Author

I chose this solution, rather than having resume_apartment_sync always copy, so we continue to avoid the copy in the co_await IAsyncXxx code path, which is far more common than co_await apartment_context.

I couldn't think of a way to unit test this, since the issue is not really observable to clients. The mismanaged reference-counted object belongs to COM, not the app.

@kennykerr
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@kennykerr kennykerr merged commit 9ddc0bb into microsoft:master Jul 14, 2020
@oldnewthing oldnewthing deleted the context-uaf branch February 16, 2021 15:33
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 this pull request may close these issues.

2 participants