@samitouri / QOSAMI-WSL-2 / commits / 404453c9

Fork 0

QOSAI WSL = Windows Subsystem for Linux

Fix a teardown race in HttpProxyStateTracker (#41097)

* Fix a teardown race in HttpProxyStateTracker HttpProxyStateTracker::QueryProxySettingsAsync() published the WinHTTP resolver/session handles and reset m_requestFinished only after calling WinHttpGetProxySettingsEx(). Since that call can complete (or fail synchronously) before returning, the destructor could run concurrently with request setup: it could observe m_requestFinished still signaled and tear down the queue and unregister the proxy-change notification while a request was still starting up, or close handles before they were fully published. Fix this by: - Adding a lock (m_requestLock) that serializes handle creation/teardown between QueryProxySettingsAsync, RequestCompleted, and the destructor, plus an m_stopping flag so no new request can start once teardown has begun. - Marking the request as in-flight (resetting m_requestFinished and setting m_queryState) before calling WinHttpGetProxySettingsEx, so the destructor cannot proceed past m_requestFinished.wait() while a request is actually outstanding. - Setting WINHTTP_OPTION_CONTEXT_VALUE on the resolver handle so that WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING always carries a valid context, including when WinHttpGetProxySettingsEx fails synchronously (which produces no completion callback), so RequestClosed() reliably runs and re-signals m_requestFinished instead of leaving it stuck. - Reordering the constructor to register the proxy-change notification before submitting the initial query, so a throw during registration can't leave a queued task running against a partially-constructed object. Tested locally with repeated concurrent wsl -e / wsl --shutdown races (including tight construct-then-shutdown loops) with no crashes or hangs observed. * fix formatting --------- Co-authored-by: Ben Hillis <benhill@ntdev.microsoft.com>

Ben Hillis committed Jul 23, 2026 at 17:55 UTC 404453c940459b25b06b672060cf80fa0bdec204

2 files changed +39 -13

src/windows/service/exe/LxssHttpProxy.cpp

+31 -11

@@ -277,8 +277,11 @@ try

277

278 // It is guaranteed that after closing the handles, a callback with status WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING

279 // will be issued to indicate that the callback has been cleaned up.

280 - m_resolver.reset();

281 - m_session.reset();

280 + {

281 + const auto requestLock = m_requestLock.lock();

282 + m_resolver.reset();

283 + m_session.reset();

284 + }

285 }

286 CATCH_LOG()

287

@@ -442,6 +445,12 @@ void HttpProxyStateTracker::QueryProxySettingsAsync()

445 try

446 {

447 WI_ASSERT(m_callbackQueue.isRunningInQueue());

448 + const auto requestLock = m_requestLock.lock();

449 + if (m_stopping)

450 + {

451 + return;

452 + }

453 +

454 if (m_queryState == QueryState::PendingAndQueueAdditional)

455 {

456 return;

@@ -469,6 +478,10 @@ void HttpProxyStateTracker::QueryProxySettingsAsync()

478 wil::unique_winhttp_hinternet resolver{};

479 THROW_IF_WIN32_ERROR(WinHttpCreateProxyResolver(session.get(), &resolver));

480

481 + executionStep = "WinHttpSetOption";

482 + DWORD_PTR context = reinterpret_cast<DWORD_PTR>(this);

483 + THROW_IF_WIN32_BOOL_FALSE(WinHttpSetOption(resolver.get(), WINHTTP_OPTION_CONTEXT_VALUE, &context, sizeof(context)));

484 +

485 executionStep = "WinHttpSetStatusCallback";

486 // We need to set flag WINHTTP_CALLBACK_FLAG_HANDLES in order to get the WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING

487 // status callback when a handle is closed.

@@ -483,10 +496,13 @@ void HttpProxyStateTracker::QueryProxySettingsAsync()

496

497 WINHTTP_PROXY_SETTINGS_PARAM ProxySettingsParam{0, nullptr, nullptr};

498

499 + // Track the request before calling WinHttp because it can complete asynchronously before returning.

500 + m_requestFinished.ResetEvent();

501 + m_queryState = QueryState::Pending;

502 +

503 executionStep = "WinHttpGetProxySettingsEx";

504 // Query the proxy settings

488 - const DWORD dwError = s_WinHttpGetProxySettingsEx.value()(

489 - resolver.get(), WinHttpProxySettingsTypeWsl, &ProxySettingsParam, reinterpret_cast<DWORD_PTR>(this));

505 + const DWORD dwError = s_WinHttpGetProxySettingsEx.value()(resolver.get(), WinHttpProxySettingsTypeWsl, &ProxySettingsParam, context);

506

507 if (dwError != ERROR_IO_PENDING && dwError != ERROR_SUCCESS)

508 {

@@ -496,8 +512,6 @@ void HttpProxyStateTracker::QueryProxySettingsAsync()

512 // Transfer ownership of HTTP handles and track request

513 m_resolver = std::move(resolver);

514 m_session = std::move(session);

499 - m_requestFinished.ResetEvent();

500 - m_queryState = QueryState::Pending;

515 }

516 catch (...)

517 {

@@ -514,8 +528,8 @@ HttpProxyStateTracker::HttpProxyStateTracker(int ProxyTimeout, HANDLE UserToken,

528 m_localizedProxyChangeString{wsl::shared::Localization::MessageHttpProxyChangeDetected()}

529 {

530 THROW_IF_WIN32_BOOL_FALSE(::DuplicateTokenEx(UserToken, MAXIMUM_ALLOWED, nullptr, SecurityImpersonation, TokenImpersonation, &m_userToken));

517 - m_callbackQueue.submit([this] { QueryProxySettingsAsync(); });

531 THROW_IF_WIN32_ERROR(s_WinHttpRegisterProxyChangeNotification.value()(WINHTTP_PROXY_NOTIFY_CHANGE, s_OnProxyChange, this, &m_proxyRegistrationHandle));

532 + m_callbackQueue.submit([this] { QueryProxySettingsAsync(); });

533 }

534

535 HttpProxyStateTracker::~HttpProxyStateTracker()

@@ -529,10 +543,16 @@ HttpProxyStateTracker::~HttpProxyStateTracker()

543 }

544 CATCH_LOG()

545 }

532 - // It is guaranteed that after closing the handles, a callback with status WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING

533 - // will be issued and it will be the last callback for this request, making it safe to delete the ProxyStateTracker.

534 - m_resolver.reset();

535 - m_session.reset();

546 +

547 + {

548 + const auto requestLock = m_requestLock.lock();

549 + m_stopping = true;

550 +

551 + // It is guaranteed that after closing the handles, a callback with status WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING

552 + // will be issued and it will be the last callback for this request, making it safe to delete the ProxyStateTracker.

553 + m_resolver.reset();

554 + m_session.reset();

555 + }

556

557 // Wait for all requests to complete. At this point no new requests can be started since we unregistered for proxy change

558 // notifications. Without this we risk racing proxy callbacks and them calling into a cleaned up ProxyStateTracker.

src/windows/service/exe/LxssHttpProxy.h

+8 -2

@@ -198,6 +198,12 @@ private:

198 /// </summary>

199 QueryState m_queryState{QueryState::NoQuery};

200

201 + /// <summary>

202 + /// Synchronizes request startup and teardown.

203 + /// </summary>

204 + wil::critical_section m_requestLock{};

205 + _Guarded_by_(m_requestLock) bool m_stopping = false;

206 +

207 /// <summary>

208 /// Used to impersonate user, as it is required for the proxy queries to run as the user; otherwise, the results will be incorrect.

209 /// </summary>

@@ -229,8 +235,8 @@ private:

235 wil::slim_event_manual_reset m_requestFinished{true};

236

237 // Handles associated with the request

232 - wil::unique_winhttp_hinternet m_session{};

233 - wil::unique_winhttp_hinternet m_resolver{};

238 + _Guarded_by_(m_requestLock) wil::unique_winhttp_hinternet m_session {};

239 + _Guarded_by_(m_requestLock) wil::unique_winhttp_hinternet m_resolver {};

240

241 /// <summary>

242 /// Single-threaded queue to trigger work from winhttp callbacks.