From 518616c500a2f0ee7ddb9fe4d3a8767e679de308 Mon Sep 17 00:00:00 2001 From: Steve Gerbino Date: Wed, 9 Sep 2026 13:18:15 -0700 Subject: [PATCH] fix(resolver): keep Winsock initialized for the process lifetime win_wsa_init reference counts WSAStartup/WSACleanup across the services that derive from it, so the count reaches zero every time the last io_context in a process is destroyed. An asynchronous GetAddrInfoExW is still settling inside ws2_32 well past its completion routine, with no way to observe when, and the WSACleanup lands under it: the fault is an access violation on a system DNS thread, with no corosio frame on the stack. That is the intermittent boost.corosio.resolver SEGFAULT on Windows. Hold one reference until static destruction so the count never falls back to zero between one io_context and the next. Suppressing the WSACleanup alone took the failure rate from 9/15 runs to 0/15 on a repro that tears down a context with a lookup still in flight. The earlier reorder of post() against work_finished() addressed a different, and as it turns out inapplicable, theory. Drop it in favour of the reactor's own convention: the work credit taken at initiation rides with the op and is released where the op is consumed, on the scheduler thread, so no foreign thread balances the count and the count cannot reach zero with a completion still queued. Both directions now match posix_resolver_service. Two further defects on the same path: - The GetAddrInfoExCancel handle is valid only until the completion routine is entered or the first cancel consumes it, but it was cleared in do_complete, which runs on the scheduler and may never run at all. A resolver destroyed with a lookup pending therefore handed Windows a handle it had already reclaimed. Retire it in the completion routine, and let cancel() claim it, so exactly one of the two owns it. The claim is taken under a lock but the cancel itself is issued outside it, because GetAddrInfoExCancel can wait on the completion routine. - The ADDRINFOEXW hints were a local of resolve(), passed to a lookup that outlives that frame. Move them into the op. The new test tears down a context with the lookup still live in the system resolver rather than already queued, which the existing test does not reach: it resolves localhost, which the hosts file answers synchronously. --- .../native/detail/iocp/win_resolver.hpp | 10 ++ .../detail/iocp/win_resolver_service.hpp | 101 +++++++++++++----- .../native/detail/iocp/win_wsa_init.hpp | 28 +++++ test/unit/resolver.cpp | 40 +++++++ 4 files changed, 151 insertions(+), 28 deletions(-) diff --git a/include/boost/corosio/native/detail/iocp/win_resolver.hpp b/include/boost/corosio/native/detail/iocp/win_resolver.hpp index e8c46a160..2fe0bb703 100644 --- a/include/boost/corosio/native/detail/iocp/win_resolver.hpp +++ b/include/boost/corosio/native/detail/iocp/win_resolver.hpp @@ -153,6 +153,10 @@ class win_resolver; struct resolve_op : overlapped_op { ADDRINFOEXW* results = nullptr; + + // GetAddrInfoExW reads these past the frame that starts the lookup. + ADDRINFOEXW hints{}; + HANDLE cancel_handle = nullptr; resolver_results* out = nullptr; std::string host; @@ -241,6 +245,7 @@ class win_resolver final { friend class win_resolver_service; friend struct resolve_op; + friend struct reverse_resolve_op; public: /// Embedded pool work item for thread pool dispatch. @@ -286,6 +291,11 @@ class win_resolver final static void do_reverse_resolve_work(pool_work_item*) noexcept; private: + // The cancel handle dies when the completion callback is entered, so + // cancel() and that callback must not both claim it. Held across the + // claim only: GetAddrInfoExCancel can wait on the callback. + win_mutex cancel_mutex_; + win_resolver_service& svc_; }; diff --git a/include/boost/corosio/native/detail/iocp/win_resolver_service.hpp b/include/boost/corosio/native/detail/iocp/win_resolver_service.hpp index 358b288f5..d40e8b5cc 100644 --- a/include/boost/corosio/native/detail/iocp/win_resolver_service.hpp +++ b/include/boost/corosio/native/detail/iocp/win_resolver_service.hpp @@ -230,13 +230,29 @@ convert_results( inline void CALLBACK resolve_op::completion(DWORD dwError, DWORD /*bytes*/, OVERLAPPED* ov) { - auto* op = static_cast(ov); + auto* op = static_cast(ov); + + // The post below can be drained and the win_resolver freed before + // this returns, and Windows owns the OVERLAPPED embedded in it until + // then. + auto keepalive = op->impl->shared_from_this(); + op->dwError = dwError; - // Post before work_finished, or the count can hit zero and run() frees - // svc_ before the post; cache svc_ since posting may free the impl. + + // Posting may free the impl svc_ is read through. auto& svc = op->impl->svc_; + + // The handle dies with this callback's entry; a racing cancel() must + // not reach one Windows has already reclaimed. + { + std::lock_guard lock(op->impl->cancel_mutex_); + op->cancel_handle = nullptr; + } + + // The initiation credit rides with the op and is released where it is + // consumed, as on POSIX; releasing it here would let the count reach + // zero with the op still queued. svc.post(op); - svc.work_finished(); } inline resolve_op::resolve_op() noexcept : overlapped_op(&do_complete) {} @@ -250,6 +266,9 @@ resolve_op::do_complete( { auto* op = static_cast(base); + // Dropping the keepalive below may free the impl svc_ is read through. + auto& svc = op->impl->svc_; + if (!owner) { // Destroy path @@ -259,10 +278,10 @@ resolve_op::do_complete( ::FreeAddrInfoExW(op->results); op->results = nullptr; } - op->cancel_handle = nullptr; // Dropping the keepalive may destroy the implementation this op // is embedded in, so nothing may touch it afterwards. - auto suicide = std::move(op->impl_ptr); + op->impl_ptr.reset(); + svc.work_finished(); return; } @@ -291,12 +310,11 @@ resolve_op::do_complete( op->results = nullptr; } - op->cancel_handle = nullptr; - op->cont.h = op->h; // Hold the keepalive across the dispatch: it may be the last // reference to the implementation this op is embedded in. auto prevent_destroy = std::move(op->impl_ptr); + svc.work_finished(); dispatch_coro(op->ex, op->cont).resume(); } @@ -316,12 +334,16 @@ reverse_resolve_op::do_complete( { auto* op = static_cast(base); + // Cached before the keepalive drops: see resolve_op::do_complete. + auto& svc = op->impl->svc_; + if (!owner) { op->stop_cb.reset(); // Dropping the keepalive may destroy the implementation this // op is embedded in, so nothing may touch it afterwards. - auto suicide = std::move(op->impl_ptr); + op->impl_ptr.reset(); + svc.work_finished(); return; } @@ -348,6 +370,7 @@ reverse_resolve_op::do_complete( // Hold the keepalive across the dispatch: it may be the last // reference to the implementation this op is embedded in. auto prevent_destroy = std::move(op->impl_ptr); + svc.work_finished(); dispatch_coro(op->ex, op->cont).resume(); } @@ -382,13 +405,14 @@ win_resolver::resolve( op.service_w = resolver_detail::to_wide(service); op.start(token); - ADDRINFOEXW hints{}; - hints.ai_family = AF_UNSPEC; - hints.ai_socktype = SOCK_STREAM; - hints.ai_flags = resolver_detail::flags_to_hints(flags); + op.hints = ADDRINFOEXW{}; + op.hints.ai_family = AF_UNSPEC; + op.hints.ai_socktype = SOCK_STREAM; + op.hints.ai_flags = resolver_detail::flags_to_hints(flags); // Keep io_context alive while resolution is pending - svc_.work_started(); + auto& svc = svc_; + svc.work_started(); // Prevent impl destruction while the async resolve is in flight and // its completion waits in the scheduler queue: the op is embedded in @@ -396,11 +420,23 @@ win_resolver::resolve( // queued completion drains. Mirrors the reverse path's keepalive. op.impl_ptr = this->shared_from_this(); - int result = ::GetAddrInfoExW( - op.host_w.empty() ? nullptr : op.host_w.c_str(), - op.service_w.empty() ? nullptr : op.service_w.c_str(), NS_DNS, nullptr, - &hints, &op.results, nullptr, &op, &resolve_op::completion, - &op.cancel_handle); + // Under the lock: the handle written here and the callback that + // retires it must not interleave. + int result; + { + std::lock_guard lock(cancel_mutex_); + op.cancel_handle = nullptr; + + result = ::GetAddrInfoExW( + op.host_w.empty() ? nullptr : op.host_w.c_str(), + op.service_w.empty() ? nullptr : op.service_w.c_str(), NS_DNS, + nullptr, &op.hints, &op.results, nullptr, &op, + &resolve_op::completion, &op.cancel_handle); + + // No callback runs on synchronous completion. + if (result != WSA_IO_PENDING) + op.cancel_handle = nullptr; + } if (result != WSA_IO_PENDING) { @@ -414,8 +450,9 @@ win_resolver::resolve( op.dwError = static_cast(::WSAGetLastError()); } - svc_.post(&op); - svc_.work_finished(); + // Post only, as above; svc_ is read through this, which the post + // may free. + svc.post(&op); } // completion is always posted to scheduler queue, never inline. return std::noop_coroutine(); @@ -473,10 +510,20 @@ win_resolver::cancel() noexcept op_.request_cancel(); reverse_op_.request_cancel(); - if (op_.cancel_handle) + // Whoever claims the handle owns it: the callback retires it on + // entry, and a claim consumes it, so neither a racing callback nor a + // second cancel() reaches one Windows has reclaimed. + HANDLE h = nullptr; { - ::GetAddrInfoExCancel(&op_.cancel_handle); + std::lock_guard lock(cancel_mutex_); + h = op_.cancel_handle; + op_.cancel_handle = nullptr; } + + // Outside the lock: GetAddrInfoExCancel can wait on the completion + // routine, which takes that same lock. + if (h) + ::GetAddrInfoExCancel(&h); } inline void @@ -529,11 +576,9 @@ win_resolver::do_reverse_resolve_work(pool_work_item* w) noexcept // outlive that wait. Nothing may touch *self after the post. self->reverse_op_.impl_ptr = std::move(pw->ref_); - // Post before work_finished (see resolve_op::completion); cache svc_ - // since posting may free *self. - auto& svc = self->svc_; - svc.post(&self->reverse_op_); - svc.work_finished(); + // Post only; the initiation credit is released where the op is + // consumed. + self->svc_.post(&self->reverse_op_); } // win_resolver_service diff --git a/include/boost/corosio/native/detail/iocp/win_wsa_init.hpp b/include/boost/corosio/native/detail/iocp/win_wsa_init.hpp index 66267871b..125d1d45a 100644 --- a/include/boost/corosio/native/detail/iocp/win_wsa_init.hpp +++ b/include/boost/corosio/native/detail/iocp/win_wsa_init.hpp @@ -52,6 +52,31 @@ inline long& win_wsa_init_count() noexcept return count; } +// Hold Winsock for the life of the process, so the count never falls back +// to zero between one io_context and the next. An asynchronous +// GetAddrInfoExW keeps settling inside ws2_32 past its completion +// routine, with no way to observe when, and a WSACleanup under it faults +// on a system DNS thread. +inline void +win_wsa_hold_process() noexcept +{ + struct holder + { + holder() noexcept + { + ::InterlockedIncrement(&win_wsa_init_count()); + } + + ~holder() + { + if (::InterlockedDecrement(&win_wsa_init_count()) == 0) + ::WSACleanup(); + } + }; + + static holder const instance; +} + inline win_wsa_init::win_wsa_init() { if (::InterlockedIncrement(&win_wsa_init_count()) == 1) @@ -64,6 +89,9 @@ inline win_wsa_init::win_wsa_init() throw_system_error(make_err(result)); } } + + // After the startup above, which must own the first count. + win_wsa_hold_process(); } inline win_wsa_init::~win_wsa_init() diff --git a/test/unit/resolver.cpp b/test/unit/resolver.cpp index 15c400ab3..d7252d9eb 100644 --- a/test/unit/resolver.cpp +++ b/test/unit/resolver.cpp @@ -24,9 +24,11 @@ #include #include +#include #include #include #include +#include #include #include "context.hpp" @@ -1225,6 +1227,43 @@ struct resolver_test } BOOST_TEST(!resumed); } + + // The same teardown with the lookup still live inside the system + // resolver rather than already queued. Each iteration drops the last + // io_context, and with it the library's Winsock reference; a + // WSACleanup there lands under the in-flight GetAddrInfoExW. + // + // A name no cache can answer is what forces the asynchronous path. + // Where the system answers synchronously the iteration is a no-op. + void testDestroyWithForwardResolvePending() + { + for (int i = 0; i < 4; ++i) + { + std::optional ex; + std::optional env; + std::optional> parked; + + std::string host = "corosio-no-such-host-" + std::to_string(i) + + "-" + + std::to_string(std::chrono::steady_clock::now() + .time_since_epoch() + .count()) + + ".invalid"; + { + io_context ioc; + resolver r(ioc); + auto query = [&]() -> capy::task<> { + std::ignore = co_await r.resolve(host, "80"); + }; + + ex.emplace(ioc.get_executor()); + env.emplace(capy::io_env{*ex, std::stop_token{}, nullptr}); + parked.emplace(query()); + parked->await_suspend(std::noop_coroutine(), &*env).resume(); + } + } + BOOST_TEST_PASS(); + } #endif #if BOOST_COROSIO_POSIX @@ -1339,6 +1378,7 @@ struct resolver_test #if BOOST_COROSIO_HAS_IOCP testDestroyWithForwardResolveQueued(); + testDestroyWithForwardResolvePending(); #endif #if !COROSIO_TEST_HAS_ASAN