Fix CET debugger stepping over CALL instructions #108809
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tions from out of process
…ft side, and resume threads on the right side before the left side is notified
…otePatch/RemoveRemotePatch for patch writing
This reverts commit 874c465.
Contributor
|
Tagging subscribers to this area: @tommcdon |
3 tasks
Member
|
[celebrate] Mikelle Rogers reacted to your message:
…________________________________
From: Tom McDonald ***@***.***>
Sent: Friday, October 11, 2024 10:14:52 PM
To: dotnet/runtime ***@***.***>
Cc: Mikelle Rogers ***@***.***>; Review requested ***@***.***>
Subject: [dotnet/runtime] Fix CET debugger stepping over CALL instructions (PR #108809)
This PR fixes a scenario where we fail when resuming from a CALL instruction that is currently patched with a breakpoint. The fix will only be enabled when executing on Windows X64 when resuming from a breakpoint on a CALL instruction and the Out-of-Process SetThreadContext debugger feature is enabled. Out of process SetThreadContext is only enabled when 1) CET is enabled, -OR- 2) DOTNET_OutOfProcessSetContext=1 environment variable is set.
Background
Debugger support for CET was added in .NET 7 on #7357<#7357>. In .NET 9, support for CET<#47309> has been enabled by default on Windows X64. When CET is detected, the .NET debugger alters the thread context from out of process instead of in-process. In general non-optimized debugging works with this fix; however, we have found that resuming from a breakpoint on a CALL instruction triggers a CET failure (often seen as 0xc0000409 or Fail Fast). This can be reproduced by attempting to step through optimized code, such as a Ready-To-Run method.
The debugger currently copies the patched instruction into a special buffer. When program execution resumes from a breakpoint, it will set thread context into the patch buffer, single-step over the instruction, then resume execution back in the original program execution stream. When we single-step over a CALL instruction, the return address on the stack points back to the patch buffer (instead of the original program stream). To account for this, we adjust the return address on the stack to point back to the instruction immediately after the CALL instruction. Unfortunately, if CET Is enabled, the shadow stack will not match the actual stack, resulting in a failure.
This PR changes fixes the shadow stack issue for resuming from CALL instructions by performing an in-place single-step over the CALL instruction in the normal program stream (not the patch buffer). To address concerns around slipping breakpoints (as we are now replacing the original instruction into the program stream when resuming from a breakpoint over a CALL instruction), we suspend all threads during the replace/single-step/re-patch sequence of events. There is new code in CordbProcess to coordinate this extra sequence of events.
We will only perform an in-place single-step when Out-of-Process Execution Control is enabled -and- we resume from breakpoints on CALL instructions.
While testing, I also found and fixed an issue with Thread::ReadyForAsyncException caused by a non-initialized CONTEXT and Windows attempting to retrieve the extended XSTATE data in the context.
________________________________
You can view, comment on, or merge this pull request online at:
#108809
Commit Summary
* 7ff15c7<7ff15c7> Initial changes to support in-place single-stepping over call instructions from out of process
* 81070ee<81070ee> ThreadSuspension WIP
* 877ab0e<877ab0e> Change how in-place single step is tracked
* fa1529b<fa1529b> Pass fSSCompleted to the right side
* 6c87a98<6c87a98> Only suspend other threads for in-place single step
* 232c7d5<232c7d5> Add thread resume in HandleSetThreadContextNeeded
* e70d2a9<e70d2a9> ScanForTriggers should return DebuggerPatchSkip
* 8b6413c<8b6413c> Refactor out of proc thread suspension into CordbThread
* 44431bd<44431bd> Avoid using DebuggerPatchSkip for tracking in-place single step on left side, and resume threads on the right side before the left side is notified
* e8b4128<e8b4128> Track InplaceSteppingThreads at the process level on the RS, ApplyRemotePatch/RemoveRemotePatch for patch writing
* 83a3043<83a3043> Block detach if m_inplaceSteppingThreads is not empty
* 961d07c<961d07c> Refactor InplaceSteppingThreads impl
* 7942742<7942742> Track thread create and exit for thread suspension
* 657102b<657102b> Change comments to minimize size of change
* 826721d<826721d> Fix blank space diff
* 331d4fd<331d4fd> Keep track of single step thread suspension count at the process level
* 141eb8c<141eb8c> Cleanup and correctly pass bool from LS to RS
* dc0ae8c<dc0ae8c> Workaround crash caused by SSP restore on fake context
* 983616f<983616f> Change page protections when clearing or writing breakpoints
* 79a1b71<79a1b71> Remove debug output and perform some cleanup
* 2169333<2169333> Track unmanaged threads with SHash instead of unordered_map
* 190dfab<190dfab> Revert "Workaround crash caused by SSP restore on fake context"
* 2feac80<2feac80> Fix func-eval abort with minimal code change
File Changes
(9 files<https://github.com/dotnet/runtime/pull/108809/files>)
* M src/coreclr/debug/di/process.cpp<https://github.com/dotnet/runtime/pull/108809/files#diff-9306f20ec74ebce567046e23510aa7f795c5a705412976ee691732b0a41d9147> (296)
* M src/coreclr/debug/di/rspriv.h<https://github.com/dotnet/runtime/pull/108809/files#diff-33cfff7c468dcb720bd40d6f730a674798433a5683db6e0aa0ede41dbd8f1dfa> (51)
* M src/coreclr/debug/di/shimprocess.cpp<https://github.com/dotnet/runtime/pull/108809/files#diff-211e69e30222b42b96f69dd6f3e764f6a64d0f11d7169b7f3ec299079df01c4a> (8)
* M src/coreclr/debug/ee/controller.cpp<https://github.com/dotnet/runtime/pull/108809/files#diff-56c927cca69b678fd1fdbeab66e14e38a36fc6f913cb3d30af7ac37859ac2d3f> (111)
* M src/coreclr/debug/ee/controller.h<https://github.com/dotnet/runtime/pull/108809/files#diff-848473b66cfd0f9aa17a37b9eb554a008f618b3a87b08c4904c5afd72f623993> (52)
* M src/coreclr/debug/ee/debugger.cpp<https://github.com/dotnet/runtime/pull/108809/files#diff-929895306dfa5d9f87b16fc8db526518977126da720f701299dc2bb96d5f7f4e> (22)
* M src/coreclr/debug/ee/debugger.h<https://github.com/dotnet/runtime/pull/108809/files#diff-69682c039e91f5562b6396a8d82e5c80d20af9a365e99d9ffd5d3b6dd5d79734> (6)
* M src/coreclr/vm/dbginterface.h<https://github.com/dotnet/runtime/pull/108809/files#diff-e382026434dc3f275026f18ff097fc46676244feb9b654ae2dc3a112a8d973a4> (6)
* M src/coreclr/vm/threadsuspend.cpp<https://github.com/dotnet/runtime/pull/108809/files#diff-c2b6a152f03d56992f672d77bd2fd94fc91b2bff527fa281fad9aa44d53e0c90> (1)
Patch Links:
* https://github.com/dotnet/runtime/pull/108809.patch
* https://github.com/dotnet/runtime/pull/108809.diff
—
Reply to this email directly, view it on GitHub<#108809>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKXP3D66HIETIKOBKMDUWSDZ3BEVZAVCNFSM6AAAAABPZXEOQ6VHI2DSMVQWIX3LMV43ASLTON2WKOZSGU4DEMRWGM4DAOA>.
You are receiving this because your review was requested.Message ID: ***@***.***>
|
noahfalk
approved these changes
Oct 14, 2024
Member
noahfalk
left a comment
There was a problem hiding this comment.
LGTM. All my suggestions are stylistic so none of them are necessary if you need to get this merged quickly to meet checkin deadlines.
Co-authored-by: Noah Falk <noahfalk@users.noreply.github.com>
4 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a scenario where we fail when resuming from a CALL instruction that is currently patched with a breakpoint. The fix will only be enabled when executing on Windows X64 when resuming from a breakpoint on a CALL instruction and the Out-of-Process SetThreadContext debugger feature is enabled. Out of process SetThreadContext is only enabled when 1) CET is enabled, -OR- 2)
DOTNET_OutOfProcessSetContext=1environment variable is set.Background
Debugger support for CET was added in .NET 7 on #7357. In .NET 9, support for CET has been enabled by default on Windows X64. When CET is detected, the .NET debugger alters the thread context from out of process instead of in-process. In general non-optimized debugging works with this fix; however, we have found that resuming from a breakpoint on a CALL instruction triggers a CET failure (often seen as 0xc0000409 or Fail Fast). This can be reproduced by attempting to step through optimized code, such as a Ready-To-Run method.
The debugger currently copies the patched instruction into a special buffer. When program execution resumes from a breakpoint, it will set thread context into the patch buffer, single-step over the instruction, then resume execution back in the original program execution stream. When we single-step over a CALL instruction, the return address on the stack points back to the patch buffer (instead of the original program stream). To account for this, we adjust the return address on the stack to point back to the instruction immediately after the CALL instruction. Unfortunately, if CET Is enabled, the shadow stack will not match the actual stack, resulting in a failure.
This PR changes fixes the shadow stack issue for resuming from CALL instructions by performing an in-place single-step over the CALL instruction in the normal program stream (not the patch buffer). To address concerns around slipping breakpoints (as we are now replacing the original instruction into the program stream when resuming from a breakpoint over a CALL instruction), we suspend all threads during the replace/single-step/re-patch sequence of events. There is new code in CordbProcess to coordinate this extra sequence of events.
We will only perform an in-place single-step when Out-of-Process Execution Control is enabled -and- we resume from breakpoints on CALL instructions.
While testing, I also found and fixed an issue with
Thread::ReadyForAsyncExceptioncaused by a non-initialized CONTEXT and Windows attempting to retrieve the extended XSTATE data in the context.