Uh oh!
There was an error while loading. Please reload this page.
Release debugger thread tracker on the win32 event thread - #119776
Release debugger thread tracker on the win32 event thread#119776tommcdon wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a thread safety issue in the CoreCLR debugger by moving the cleanup of unmanaged thread trackers from the CordbProcess::Neuter() method to the win32 event thread during process exit. The change prevents potential crashes that could occur when neutering the process while win32 events are still being processed in parallel.
Key changes:
- Moved thread tracker cleanup logic from
Neuter()to win32 event thread execution - Added a new
ClearThreadTrackers()method to encapsulate the cleanup logic - Ensured thread-safe access to the unmanaged thread hash table
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/debug/di/rspriv.h | Added declaration for new ClearThreadTrackers() method |
| src/coreclr/debug/di/process.cpp | Removed thread tracker cleanup from Neuter(), added new method implementation, and called it from win32 event thread |
| m_pProcess->m_exiting = true; | ||
| #ifdef OUT_OF_PROCESS_SETTHREADCONTEXT | ||
| m_pProcess->ClearThreadTrackers(); // we must release the thread trackers on the Win32EventThread |
There was a problem hiding this comment.
CordbProcess::Neuter ensured that threads were resumed and let go from the tracker. Now, on detach we won't clean them out. Could this leave threads in suspended state on detach?
There was a problem hiding this comment.
For process exit, the threads are gone anyway. I have a separate PR solves the problem for detach. #118849
xtqqczze
commented
Sep 16, 2025
Build failures were resolved by 579bd79. |
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "a3f00614b9eb05422e61f2ebc8eb18cc736d8e80",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "3fc2c4afb24f6b005cf91bfdc3d1ed06e956563e",
"last_reviewed_commit": "a3f00614b9eb05422e61f2ebc8eb18cc736d8e80",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "3fc2c4afb24f6b005cf91bfdc3d1ed06e956563e",
"last_recorded_worker_run_id": "29673722851",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "a3f00614b9eb05422e61f2ebc8eb18cc736d8e80",
"review_id": 4730106436
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: m_unmanagedThreadHashTable on CordbProcess tracks unmanaged threads used for out-of-process SetThreadContext single-stepping. It is normally accessed on the Win32 event thread. Previously CordbProcess::Neuter() iterated the table and deleted the UnmanagedThreadTracker objects, but Neuter() can run on a different thread while the Win32 event thread is still processing native debug events, creating a data race that can corrupt these objects or crash the debugger.
Approach: The deletion logic is extracted from Neuter() into a new CordbProcess::ClearThreadTrackers() method, which is now invoked from CordbWin32EventThread::ExitProcess() under the process lock, guaranteeing it runs on the Win32 event thread. The moved code is otherwise identical (Close + delete each tracker, RemoveAll(), reset m_dwOutOfProcessStepping), and both the old and new sites are guarded by OUT_OF_PROCESS_SETTHREADCONTEXT. The header declares the new method inside the same conditional block.
Summary: The change is minimal, correct, and directly addresses the described race. The cleanup now happens on the owning thread and under m_pProcess->Lock(), which is the right synchronization boundary. I found no correctness or resource-management regressions in the changed lines. One non-blocking observation: ExitProcess() has early-return paths (the DebugActiveProcessStop failure path and the EP(detach) after EP(exit) path) that occur before ClearThreadTrackers() is reached. The detach-failure early return leaves the process alive so no leak results there, and the second case is a duplicate call whose first invocation already cleared the table, so neither introduces a leak in practice. Verdict: LGTM.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 48.1 AIC · ⌖ 10.4 AIC · ⊞ 10K
The m_unmanagedThreadHashTable in CordbProcess is used to track unmanaged threads for out-of-process setthreadcontext, suspending/resuming threads for single-stepping over call instructions. The table is accessed view the win32 event thread. When neutering CordbProcess, it's still possible to be receiving win32 events in parallel, however, we currently iterate and release objects in m_unmanagedThreadHashTable on a separate thread. This could cause issues with the objects, or even a crash. This change moves the deallocation of the objects in m_unmanagedThreadHashTable to the win32 event thread, eliminating thread unsafeness.