Skip to content

Fix a teardown race in HttpProxyStateTracker - #41097

Merged
Ben Hillis (benhillis) merged 2 commits into
microsoft:masterfrom
benhillis:user/benhill/fix-httpproxy-teardown-race
Jul 24, 2026
Merged

Ben Hillis (benhillis) merged 2 commits into
microsoft:masterfrom
benhillis:user/benhill/fix-httpproxy-teardown-race

Conversation

@benhillis

Copy link
Copy Markdown
Member

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 it even returns, the destructor could run concurrently with request setup: it could observe m_requestFinished still signaled and tear down the message queue / unregister the proxy-change notification while a request was still starting up, or tear down handles before they were fully published.

This PR fixes that 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 can't 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.

Testing

  • Full local build.
  • Multiple rounds of code review.
  • Repeated concurrent wsl -e / wsl --shutdown races (including tight construct-then-shutdown loops) with no crashes or hangs observed, and the service PID remained stable throughout.
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.
@benhillis
Ben Hillis (benhillis) requested a review from a team as a code owner July 16, 2026 21:06
Copilot AI review requested due to automatic review settings July 16, 2026 21:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a teardown/startup race in HttpProxyStateTracker (WSL service) around WinHTTP async proxy queries, ensuring the tracker can’t begin a new query while teardown is in progress and that request lifetime is reliably tracked even when WinHTTP completes (or fails) synchronously.

Changes:

  • Added a request lifecycle lock (m_requestLock) plus m_stopping to serialize request startup/teardown and prevent new requests during destruction.
  • Marked requests as in-flight before calling WinHttpGetProxySettingsEx to prevent teardown from observing a “finished” state while a request is actually starting.
  • Set WINHTTP_OPTION_CONTEXT_VALUE on the resolver handle so WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING always has a valid context, enabling RequestClosed() to reliably re-signal m_requestFinished.
  • Reordered constructor operations to register proxy-change notifications before submitting the initial query task.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/windows/service/exe/LxssHttpProxy.h Introduces request lifecycle synchronization members (m_requestLock, m_stopping) and guards WinHTTP handles with the new lock.
src/windows/service/exe/LxssHttpProxy.cpp Applies the new synchronization to query start/completion/destruction, sets resolver context via WINHTTP_OPTION_CONTEXT_VALUE, and reorders initialization to avoid partially-constructed usage.
Copilot AI review requested due to automatic review settings July 16, 2026 21:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@benhillis
Ben Hillis (benhillis) merged commit 404453c into microsoft:master Jul 24, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants