From 506d5e198f079b114251e3e5c98bbecf89971bc8 Mon Sep 17 00:00:00 2001 From: Michael Vandeberg Date: Tue, 29 Sep 2026 15:18:12 -0700 Subject: [PATCH] fix: correct the workaround gate for symmetric transfer on MSVC `detail::symmetric_transfer` applied the workaround to each compiler that defines `_MSC_VER`. This was too wide. MSVC has two different defects, and the new gate keeps the workaround for each one: #if (BOOST_CAPY_WORKAROUND(_MSC_VER, < 1950) || \ defined(_M_ARM64) || defined(_M_ARM64EC)) && \ !defined(__clang__) The first defect occurs on MSVC 19.34 to 19.44, on all architectures. The caller puts the hidden return slot of `await_suspend` on the coroutine frame, at `__coro_frame_ptr$ + 0xC0`. When `await_suspend` destroys that frame, the runtime then reads released memory. MSVC 19.50 puts the slot on the stack, thus the workaround retires there. The second defect occurs on MSVC 19.51 for ARM64, in release builds only. A `try` region that spans the suspend point loses its handler. When the coroutine resumes from a different call stack, a `throw` in that region does not go to the adjacent `catch`. The exception goes to `unhandled_exception` of the promise. A `catch(...)` also does not get it, thus the runtime does not find the region at all. Debug builds are correct, and so are x64 builds at the same toolset. The gate includes `_M_ARM64EC` because that target also emits ARM64 code. The gate excludes Clang. `clang-cl` and `clang++` define `_MSC_VER` for ABI compatibility, but they emit a correct tail-call. Two tests cover the defects: - `test/unit/detail/await_suspend_helper.cpp` destroys the coroutine frame in `await_suspend` and then transfers. The test unmaps each frame on destruction, thus every subsequent access gives a fault. A poison pattern is not sufficient, because `symmetric_transfer` moves the write to the frame to a point after the destruction. - `test/unit/task.cpp` awaits a handle-returning awaitable in a `try` block, resumes the parked handle from a different call stack, and then throws. The test watches for an escape through the error handler of `run_async`. A failure thus gives an assertion and not a terminate, which keeps the other results. Refs #378 --- .../capy/detail/await_suspend_helper.hpp | 58 ++++++- test/unit/detail/await_suspend_helper.cpp | 141 ++++++++++++++++++ test/unit/task.cpp | 91 +++++++++++ 3 files changed, 283 insertions(+), 7 deletions(-) diff --git a/include/boost/capy/detail/await_suspend_helper.hpp b/include/boost/capy/detail/await_suspend_helper.hpp index c2d704ccb..7b88065d8 100644 --- a/include/boost/capy/detail/await_suspend_helper.hpp +++ b/include/boost/capy/detail/await_suspend_helper.hpp @@ -12,6 +12,7 @@ #define BOOST_CAPY_DETAIL_AWAIT_SUSPEND_HELPER_HPP #include +#include #include #include @@ -40,19 +41,62 @@ namespace detail { frame before the runtime reads `__$ReturnUdt$` (e.g. `boundary_trampoline` final_suspend). - On MSVC this function calls `h.resume()` on the current stack - and returns `void`, causing unconditional suspension. The - trade-off is O(n) stack growth instead of O(1) tail-calls. - - On other compilers the handle is returned directly for proper - symmetric transfer. + On affected compilers this function calls `h.resume()` on the + current stack and returns `void`, causing unconditional + suspension. The trade-off is O(n) stack growth instead of + O(1) tail-calls. + + On x64 the workaround applies to MSVC 19.34 through 19.44 and + self-retires on MSVC 19.50 (VS 2026 / 18.0). Measured on + 19.44 the caller builds the hidden return slot at + `__coro_frame_ptr$ + 0xC0`, on the coroutine frame; on 19.51 + it is an `rsp`-relative stack temporary, so destroying the + frame no longer invalidates it. + + Do not widen this gate on the basis of Developer Community + ticket 10251975, tagged "Fixed in VS 2022 17.9 Preview 2"; + 19.39 reproduces the fault identically to 19.34. + + On ARM64 the workaround does not retire at 19.50, because + that target has a second, unrelated defect. With a real + symmetric transfer, MSVC 19.51 release loses the handler for + a `try` region that spans the suspend point: after the + coroutine is resumed from another call stack, a `throw` + inside that region is not caught by the `catch` beside it and + escapes to the promise's `unhandled_exception`. A `catch(...)` + misses it too, so the region is not found at all rather than + the handler failing to match. Debug builds are unaffected, as + is x64 at the same toolset. `testCatchAfterDeferredResume` in + test/unit/task.cpp covers this. + + `_M_ARM64EC` is included conservatively. It generates ARM64 + code and has not been tested here; keeping the workaround on + is the safe direction, since it costs stack depth rather than + correctness. + + The gate deliberately excludes Clang. Both `clang-cl` and + `clang++` targeting Windows define `_MSC_VER` for ABI + compatibility, but generate a correct tail-call. + + Note that a probe which merely poisons the destroyed frame + cannot validate this gate. Routing the return through this + function moves the frame write to after `destroy()`, which + repairs the poison pattern and hides the defect. The + regression test in + test/unit/detail/await_suspend_helper.cpp unmaps the frame + instead, so any post-destroy access faults. + + On unaffected compilers the handle is returned directly for + proper symmetric transfer. Callers must use `auto` return type on their `await_suspend` so the return type adapts per platform. @param h The coroutine handle to transfer to. */ -#if BOOST_CAPY_WORKAROUND(_MSC_VER, >= 1) +#if (BOOST_CAPY_WORKAROUND(_MSC_VER, < 1950) || \ + defined(_M_ARM64) || defined(_M_ARM64EC)) && \ + !defined(__clang__) inline void symmetric_transfer(std::coroutine_handle<> h) noexcept { // safe_resume is not needed here: the calling coroutine is diff --git a/test/unit/detail/await_suspend_helper.cpp b/test/unit/detail/await_suspend_helper.cpp index 70b6eec96..45cb95fc7 100644 --- a/test/unit/detail/await_suspend_helper.cpp +++ b/test/unit/detail/await_suspend_helper.cpp @@ -13,6 +13,14 @@ #include #include +#include +#include + +#ifdef _WIN32 +# include +#else +# include +#endif #include "test_suite.hpp" @@ -20,6 +28,111 @@ namespace boost { namespace capy { namespace detail { +namespace { + +bool probe_continuation_ran = false; + +// Coroutine frames for the destroy-then-transfer probe, allocated so +// that destroying a frame UNMAPS it. Any access to the frame after +// destroy - read or write - then faults. +// +// A poisoning allocator is not sufficient. Routing the return through +// symmetric_transfer moves the compiler's write of the handle into the +// frame slot to after the frame is destroyed, which repairs a poison +// pattern and hides the defect. Unmapping cannot be repaired, so this +// probe sees the frame access that poisoning misses. +struct probe_frame +{ + static void* allocate(std::size_t n) noexcept + { + #ifdef _WIN32 + return ::VirtualAlloc( + nullptr, n, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE); + #else + void* const p = ::mmap(nullptr, n, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + return p == MAP_FAILED ? nullptr : p; + #endif + } + + static void release(void* p, std::size_t n) noexcept + { + #ifdef _WIN32 + (void)n; + ::VirtualFree(p, 0, MEM_RELEASE); + #else + ::munmap(p, n); + #endif + } +}; + +struct probe_task +{ + struct promise_type + { + void* operator new(std::size_t n) + { + void* const p = probe_frame::allocate(n); + if(! p) + throw std::bad_alloc(); + return p; + } + + void operator delete(void* p, std::size_t n) noexcept + { + probe_frame::release(p, n); + } + + probe_task get_return_object() noexcept + { + return { std::coroutine_handle< + promise_type>::from_promise(*this) }; + } + + std::suspend_always initial_suspend() noexcept { return {}; } + std::suspend_always final_suspend() noexcept { return {}; } + void return_void() noexcept {} + void unhandled_exception() noexcept { BOOST_TEST(false); } + }; + + std::coroutine_handle h; +}; + +// Mirrors the final_suspend awaiters in when_all_runner and +// when_any_runner: destroy our own frame, then transfer to the +// continuation. auto return type, because symmetric_transfer +// returns void on the workaround path. +struct destroy_then_transfer +{ + std::coroutine_handle<> next; + + bool await_ready() const noexcept { return false; } + + auto await_suspend(std::coroutine_handle<> self) noexcept + { + // Copy to the stack first: this awaiter lives on the frame + // about to be destroyed. + auto const continuation = next; + self.destroy(); + return symmetric_transfer(continuation); + } + + void await_resume() const noexcept {} +}; + +probe_task probe_continuation() +{ + probe_continuation_ran = true; + co_return; +} + +probe_task probe_victim(std::coroutine_handle<> next) +{ + co_await destroy_then_transfer{ next }; +} + +} // (anon) + class await_suspend_helper_test { // await_suspend returning void: caller suspends unconditionally. @@ -55,9 +168,37 @@ class await_suspend_helper_test }; public: + // capy#378: transferring out of an await_suspend that has already + // destroyed its own frame must not touch that frame afterwards. + // On affected MSVC toolsets symmetric_transfer resumes on the + // current stack to avoid the frame round-trip; elsewhere it + // performs a real tail-call. Either way the continuation must run + // and the process must survive. + // + // When the gate is wrong for the compiler in use this fails as a + // hard access violation rather than a BOOST_TEST failure. Each + // test runs in its own process under CTest, so the crash is + // reported as this test failing. + void + testDestroyThenTransfer() + { + probe_continuation_ran = false; + + auto cont = probe_continuation(); + auto victim = probe_victim(cont.h); + + victim.h.resume(); + + BOOST_TEST(probe_continuation_ran); + + cont.h.destroy(); + } + void run() { + testDestroyThenTransfer(); + auto const h = std::noop_coroutine(); { diff --git a/test/unit/task.cpp b/test/unit/task.cpp index a2200f03f..30006b898 100644 --- a/test/unit/task.cpp +++ b/test/unit/task.cpp @@ -1277,9 +1277,100 @@ struct task_test BOOST_TEST_EQ(result, 7); } + // Suspends by parking the handle and returning noop_coroutine, so + // the coroutine resumes later from whatever stack calls resume(), + // rather than being transferred into inline. This is the shape of + // an async read that completes on the io_context thread. + struct parking_awaitable + { + std::coroutine_handle<>& slot; + + bool await_ready() const noexcept { return false; } + + std::coroutine_handle<> + await_suspend( + std::coroutine_handle<> h, io_env const*) const noexcept + { + slot = h; + return std::noop_coroutine(); + } + + // Returning a value the body tests before throwing mirrors + // `auto [ec, n] = co_await sock.read_some(buf); if(ec) throw;`. + bool await_resume() const noexcept { return true; } + }; + + // The body whose catch must run. Kept as a coroutine taking + // references rather than a capturing lambda so the shape matches + // the corosio session function this reproduces. + static task + deferred_throw_session( + std::coroutine_handle<>& slot, + bool& caught) + { + try + { + if(co_await parking_awaitable{slot}) + throw_test_exception("deferred resume"); + } + catch(test_exception const&) + { + caught = true; + } + } + + // Regression test for an MSVC 14.51 ARM64 release codegen bug. + // + // When a co_await whose await_suspend returns a coroutine_handle + // suspends, and the coroutine is later resumed from a different + // call stack, a throw in the same try block was not caught by the + // catch beside it. The exception escaped to the promise's + // unhandled_exception and reached run_async's error handler; in + // corosio's 3a_echo_server documentation test it terminated the + // process. + // + // detail::symmetric_transfer hides the bug wherever its workaround + // is enabled, because it resumes on the current stack and returns + // void instead of performing a real symmetric transfer. So this + // test passes on every compiler the workaround covers, and is only + // meaningful where it does not. + // + // The escape is checked through run_async's error handler rather + // than allowed to propagate: an uncaught exception here would + // terminate the whole test binary and hide every other result. + void + testCatchAfterDeferredResume() + { + int dispatch_count = 0; + test_executor ex(dispatch_count); + + std::coroutine_handle<> parked; + bool caught = false; + bool escaped = false; + bool completed = false; + + run_async(ex, + [&]() { completed = true; }, + [&](std::exception_ptr) { escaped = true; })( + deferred_throw_session(parked, caught)); + + // The awaitable parked the handle and nothing has thrown yet. + BOOST_TEST(static_cast(parked)); + BOOST_TEST(!caught); + + // Resume from here, not from inside await_suspend: arriving on + // an unrelated call stack is what the bug needs. + parked.resume(); + + BOOST_TEST(caught); + BOOST_TEST(!escaped); + BOOST_TEST(completed); + } + void run() { + testCatchAfterDeferredResume(); testBoolSuspendAwaitable(); testReturnValue(); testException();