Skip to content

Fix missing synchronization on m_httpProxyStateTracker access - #41907

Open
Blue (OneBlue) wants to merge 5 commits into
masterfrom
user/oneblue/fix-race-7
Open

Blue (OneBlue) wants to merge 5 commits into
masterfrom
user/oneblue/fix-race-7

Conversation

@OneBlue

Copy link
Copy Markdown
Collaborator

Summary of the Pull Request

This change adds missing synchronization around reading and writing to m_httpProxyStateTracker.

PR Checklist

  • Closes: Link to issue #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated if needed and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated if needed
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@OneBlue
Blue (OneBlue) requested a review from a team as a code owner October 13, 2026 18:42
Copilot AI balanced review requested due to automatic review settings October 13, 2026 18:42

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.

🟡 Changes recommended

The tracker remains a plain std::shared_ptr, so the new atomic member calls do not compile.

1 open finding
What changed in this PR

Adds synchronized access to the HTTP proxy state tracker.

Changes:

  • Replaces direct accesses with atomic load/store operations.
  • Uses compare-and-exchange during tracker initialization.
File Description
src/​windows/​service/​exe/​LxssUserSession.cpp Synchronizes proxy tracker access and initialization.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/windows/service/exe/LxssUserSession.cpp Outdated
Copilot AI balanced review requested due to automatic review settings October 13, 2026 18:44

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.

🟡 Changes recommended

Concurrent proxy initialization can still race while populating shared WinHTTP function objects.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced

Comment thread src/windows/service/exe/LxssUserSession.cpp Outdated
Copilot AI balanced review requested due to automatic review settings October 13, 2026 19:11

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.

🟢 Approval recommended

All tracker accesses now consistently satisfy the locking contract without introducing unresolved correctness issues.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@dkbennett David Bennett (dkbennett) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As with similar bugs, regression test would be nice but these fixes look like goodness to me regardless and synchronization tests can be tricky to get right and could be flaky so I'm OK skipping them.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants