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>

Author: benhillis

src/windows/service/exe/LxssHttpProxy.cpp

@@ -277,8 +277,11 @@ try
 
     // It is guaranteed that after closing the handles, a callback with status WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING
     // will be issued to indicate that the callback has been cleaned up.
-    m_resolver.reset();
-    m_session.reset();
+    {
+        const auto requestLock = m_requestLock.lock();
+        m_resolver.reset();
+        m_session.reset();
+    }
 }
 CATCH_LOG()
 
@@ -442,6 +445,12 @@ void HttpProxyStateTracker::QueryProxySettingsAsync()
     try
     {
         WI_ASSERT(m_callbackQueue.isRunningInQueue());
+        const auto requestLock = m_requestLock.lock();
+        if (m_stopping)
+        {
+            return;
+        }
+
         if (m_queryState == QueryState::PendingAndQueueAdditional)
         {
             return;
@@ -469,6 +478,10 @@ void HttpProxyStateTracker::QueryProxySettingsAsync()
         wil::unique_winhttp_hinternet resolver{};
         THROW_IF_WIN32_ERROR(WinHttpCreateProxyResolver(session.get(), &resolver));
 
+        executionStep = "WinHttpSetOption";
+        DWORD_PTR context = reinterpret_cast<DWORD_PTR>(this);
+        THROW_IF_WIN32_BOOL_FALSE(WinHttpSetOption(resolver.get(), WINHTTP_OPTION_CONTEXT_VALUE, &context, sizeof(context)));
+
         executionStep = "WinHttpSetStatusCallback";
         // We need to set flag WINHTTP_CALLBACK_FLAG_HANDLES in order to get the WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING
         // status callback when a handle is closed.
@@ -483,10 +496,13 @@ void HttpProxyStateTracker::QueryProxySettingsAsync()
 
         WINHTTP_PROXY_SETTINGS_PARAM ProxySettingsParam{0, nullptr, nullptr};
 
+        // Track the request before calling WinHttp because it can complete asynchronously before returning.
+        m_requestFinished.ResetEvent();
+        m_queryState = QueryState::Pending;
+
         executionStep = "WinHttpGetProxySettingsEx";
         // Query the proxy settings
-        const DWORD dwError = s_WinHttpGetProxySettingsEx.value()(
-            resolver.get(), WinHttpProxySettingsTypeWsl, &ProxySettingsParam, reinterpret_cast<DWORD_PTR>(this));
+        const DWORD dwError = s_WinHttpGetProxySettingsEx.value()(resolver.get(), WinHttpProxySettingsTypeWsl, &ProxySettingsParam, context);
 
         if (dwError != ERROR_IO_PENDING && dwError != ERROR_SUCCESS)
         {
@@ -496,8 +512,6 @@ void HttpProxyStateTracker::QueryProxySettingsAsync()
         // Transfer ownership of HTTP handles and track request
         m_resolver = std::move(resolver);
         m_session = std::move(session);
-        m_requestFinished.ResetEvent();
-        m_queryState = QueryState::Pending;
     }
     catch (...)
     {
@@ -514,8 +528,8 @@ HttpProxyStateTracker::HttpProxyStateTracker(int ProxyTimeout, HANDLE UserToken,
     m_localizedProxyChangeString{wsl::shared::Localization::MessageHttpProxyChangeDetected()}
 {
     THROW_IF_WIN32_BOOL_FALSE(::DuplicateTokenEx(UserToken, MAXIMUM_ALLOWED, nullptr, SecurityImpersonation, TokenImpersonation, &m_userToken));
-    m_callbackQueue.submit([this] { QueryProxySettingsAsync(); });
     THROW_IF_WIN32_ERROR(s_WinHttpRegisterProxyChangeNotification.value()(WINHTTP_PROXY_NOTIFY_CHANGE, s_OnProxyChange, this, &m_proxyRegistrationHandle));
+    m_callbackQueue.submit([this] { QueryProxySettingsAsync(); });
 }
 
 HttpProxyStateTracker::~HttpProxyStateTracker()
@@ -529,10 +543,16 @@ HttpProxyStateTracker::~HttpProxyStateTracker()
         }
         CATCH_LOG()
     }
-    // It is guaranteed that after closing the handles, a callback with status WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING
-    // will be issued and it will be the last callback for this request, making it safe to delete the ProxyStateTracker.
-    m_resolver.reset();
-    m_session.reset();
+
+    {
+        const auto requestLock = m_requestLock.lock();
+        m_stopping = true;
+
+        // It is guaranteed that after closing the handles, a callback with status WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING
+        // will be issued and it will be the last callback for this request, making it safe to delete the ProxyStateTracker.
+        m_resolver.reset();
+        m_session.reset();
+    }
 
     // Wait for all requests to complete. At this point no new requests can be started since we unregistered for proxy change
     // notifications. Without this we risk racing proxy callbacks and them calling into a cleaned up ProxyStateTracker.

src/windows/service/exe/LxssHttpProxy.h

@@ -198,6 +198,12 @@ class HttpProxyStateTracker
     /// </summary>
     QueryState m_queryState{QueryState::NoQuery};
 
+    /// <summary>
+    /// Synchronizes request startup and teardown.
+    /// </summary>
+    wil::critical_section m_requestLock{};
+    _Guarded_by_(m_requestLock) bool m_stopping = false;
+
     /// <summary>
     /// Used to impersonate user, as it is required for the proxy queries to run as the user; otherwise, the results will be incorrect.
     /// </summary>
@@ -229,8 +235,8 @@ class HttpProxyStateTracker
     wil::slim_event_manual_reset m_requestFinished{true};
 
     // Handles associated with the request
-    wil::unique_winhttp_hinternet m_session{};
-    wil::unique_winhttp_hinternet m_resolver{};
+    _Guarded_by_(m_requestLock) wil::unique_winhttp_hinternet m_session {};
+    _Guarded_by_(m_requestLock) wil::unique_winhttp_hinternet m_resolver {};
 
     /// <summary>
     /// Single-threaded queue to trigger work from winhttp callbacks.