Bug: Access violation when using winrt::resume_on_signal

[fereeh]

Version

No response

Summary

We ran into an access violation when using winrt::resume_on_signal. It was caused by a race condition in signal_awaiter::create_threadpool_wait. The method accesses *this after scheduling a "transfer" of the coroutine handle across threads using a threadpool wait. By the time m_state is accessed, a worker thread may have already resumed the coroutine, destroying the signal_awaiter and eventually the coroutine state.

Reproducible example

No response

Expected behavior

No response

Actual behavior

No response

Additional comments

No response

[kennykerr]

Keep in mind that a co_await expression is about async synchronization and not about async ownership. Its still the coroutine's responsibility to stabilize any references (including this) prior to suspension.

[sylveon]

The coroutine code in this case is correct - it's just co_await winrt::resume_on_signal(handle) (which is done in sample code mind you) which will only destroy the signal_awaiter once the wait is over.

The issue is that if the threadpool wait immediately fires, the coroutine execution resumes and destroys the signal_awaiter before reaching this code (which seems to assume the threadpool wait won't immediately fire):

cppwinrt/strings/base_coroutine_threadpool.h

Lines 405 to 409 in 297454e

	state expected = state::idle;
	if (!m_state.compare_exchange_strong(expected, state::pending, std::memory_order_release))
	{
	fire_immediately();
	}

This seems like it could be a problem with the other awaiters that rely on SetThreadpoolXXX functions. Why not move this assignment to before SetThreadpoolXXX is called? I think @oldnewthing wrote this originally, he probably knows why it was done that way.

[fereeh]

Quoting cppreference for additional context:

Note that because the coroutine is fully suspended before entering awaiter.await_suspend(), that function is free to transfer the coroutine handle across threads, with no additional synchronization. For example, it can put it inside a callback, scheduled to run on a threadpool when async I/O operation completes. In that case, since the current coroutine may have been resumed and thus executed the awaiter object's destructor, all concurrently as await_suspend() continues its execution on the current thread, await_suspend() should treat *this as destroyed and not access it after the handle was published to other threads.

They have some sample code right below the quoted text that further illustrates the problem.

[github-actions]

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[sylveon]

What's the opinions of maintainers on this? Is moving the assignment before SetThreadpoolXxx being called an acceptable solution?

[kennykerr]

It was simpler when I originally wrote it, and I'm not comfortable messing with it now. Unless @oldnewthing is available, I suggest you write your own resume_on_signal and move on. It's not hard to get it right if you keep things simple. 😊

[oldnewthing]

I'm busy right now but I'll try to find time to look at this in the next few weeks.

[github-actions]

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[sylveon]

Still a problem

[github-actions]

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.