Skip to content

Fix use-after-free in WSLCContainerImpl exec-process teardown - #40822

Merged
yao-msft merged 2 commits into
masterfrom
user/yaosun/secb1
Jun 17, 2026
Merged

Fix use-after-free in WSLCContainerImpl exec-process teardown#40822
yao-msft merged 2 commits into
masterfrom
user/yaosun/secb1

Conversation

@yao-msft

Copy link
Copy Markdown
Contributor

Summary of the Pull Request

Change m_processes to use weak pointers instead of raw pointers. Use lock() to pin on use.

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

Existing tests

Copilot AI review requested due to automatic review settings June 16, 2026 20:46

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 addresses a use-after-free risk during container exec-process teardown by changing WSLCContainerImpl’s tracking of exec process controls from raw pointers to std::weak_ptr, and pinning controls with lock() before invoking teardown callbacks.

Changes:

  • Replace m_processes storage from std::vector<DockerExecProcessControl*> to std::vector<std::weak_ptr<DockerExecProcessControl>>.
  • Update exec creation to use std::make_shared<DockerExecProcessControl>() and store weak references in m_processes.
  • Remove WSLCContainerImpl::OnProcessReleased() and update teardown paths (ReleaseProcesses(), container destructor) to snapshot the weak list and safely notify live controls via weak_ptr::lock().

Reviewed changes

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

File Description
src/windows/wslcsession/WSLCProcessControl.cpp Removes destructor callback into the container, relying on weak tracking to avoid UAF during teardown.
src/windows/wslcsession/WSLCContainer.h Updates m_processes to store std::weak_ptr<DockerExecProcessControl> instead of raw pointers.
src/windows/wslcsession/WSLCContainer.cpp Switches exec control creation to shared_ptr, stores weak refs, and pins controls with lock() during teardown notifications.

@yao-msft
yao-msft marked this pull request as ready for review June 16, 2026 22:30
@yao-msft
yao-msft requested a review from a team as a code owner June 16, 2026 22:30
OneBlue
Blue (OneBlue) previously approved these changes Jun 17, 2026

@OneBlue Blue (OneBlue) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM ! It's wild that we never hit this during testing

{
std::lock_guard processesLock{m_processesLock};
// Snapshot under the lock, then notify outside it, pinning each control via lock() first
// (avoids the use after free, and avoids calling back under m_processesLock).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: I'd recommend removing the avoids the use after free since once merged, that comment will refer to an issue that won't exist anymore

@@ -118,11 +118,7 @@ DockerExecProcessControl::DockerExecProcessControl(

DockerExecProcessControl::~DockerExecProcessControl()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: This destructor can be removed since it's not doing anything

@yao-msft
yao-msft merged commit 8cb27f1 into master Jun 17, 2026
11 checks passed
@yao-msft
yao-msft deleted the user/yaosun/secb1 branch June 17, 2026 19:18
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