Size the RESOLVE_UPDATE_METADATA events in DbgTransportSession::GetEventSize - #134717
Open
pcshrosbree wants to merge 1 commit into
Open
pcshrosbree wants to merge 1 commit into
pcshrosbree wants to merge 1 commit into
Conversation
On Unix, DbgTransportSession::GetEventSize had no case for the four DB_IPCE_RESOLVE_UPDATE_METADATA_* events, so release builds sent them header-only and the receiver read stale bytes in place of the MetadataUpdateRequest payload. Requesting the locals of a method that a profiler had given new locals could then crash the debuggee. Size _1, _1_RESULT and _2 as MetadataUpdateRequest, and add _2_RESULT, which carries only the header hr, to the zero-payload group. Fix dotnet#134714
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
On Unix,
DbgTransportSession::GetEventSizehas no case for the fourDB_IPCE_RESOLVE_UPDATE_METADATA_*events. They fall todefault:, so release builds send only the event header, and checked builds assert"Unknown debugger event type"in the sending process. The receiver then reads stale bytes where theMetadataUpdateRequestpayload should be.The debugger starts this exchange when it meets a local-signature token that is not in its cached metadata, for example after a profiler has given a method new locals with
SetILFunctionBody. Requesting that method's locals then can crash the debuggee (SIGSEGV inDebugger::SerializeModuleMetaData). Details, a minimal repro and results on 8.0 to 11.0 are in the linked issue.This change sizes the three events that carry
MetadataUpdateRequest, and adds_2_RESULT, which carries only the headerhr, to the existing zero-payload group:_1vmModule_1_RESULTpMetadataStart,nMetadataSize_2pMetadataStart(the buffer to free)_2_RESULTGetEventSizeis compiled into bothmscordbiandcoreclr(debug/di/shared.cpp,debug/ee/shared.cpp), and each side sizes the events it sends, so both binaries need the change.Validation
With the minimal repro from the issue (a small C profiler that adds one unused local, and netcoredbg requesting locals at an exception stop), 3 runs per cell unless noted:
GetEventSize, 3/3mainatc29faec90e46, linux-x64 (VS Code; release, and checked for "after"¹)¹ The checked
mainbuild also swaps the DAC, JIT and CoreLib.Within each release configuration, only
libcoreclr.soandlibmscordbi.sodiffer between the before and after builds. macOS shares the transport and was not tested. The controls (no profiler; an unchanged body; a new MemberRef without a new local) pass on every build where they were run.Tests
I did not find a test in this repo that exercises the debugger transport (debugger tests appear to live in dotnet/diagnostics), so this PR adds no test, as with #97476 and #110122. The repro is attached to the issue. #134715 proposes a checked-build check that every declared event type is sized. I am happy to add a test if you can point me at the right place.
Servicing
The omission is present on every supported branch. I'd suggest this for
release/11.0, ideally before GA; per-branch patches for 10.0, 9.0 and 8.0 are attached to the issue in case you judge those worth servicing. If you do service it, the text below may save time:Servicing template
Customer Impact
On Linux, when a profiler has given a method new locals with
SetILFunctionBody, a debugger that requests that method's locals (an IDE's Variables pane can do so automatically, e.g. at an exception stop) can crash the debuggee. macOS uses the same transport and was not tested; Windows does not use it.Regression
Present since at least 6.0 (checked from
v6.0.36).Testing
The minimal repro in #134714: release and checked 10.0.12 builds (linux-x64), release (linux-arm64), the 11.0 RC1 tag and
main(linux-x64, VS Code).Risk
Low. Cases are added to a size switch for events that were sent header-only. Only the sent length of these three events grows, to include their payload; no other event, struct or framing changes. The debugger (
libmscordbi.so) and the runtime (libcoreclr.so) should ship together: a fixedmscordbiwith an unpatchedcoreclrcan send a stale pointer back in_2for the runtime to free (see the issue). Both are in the runtime package.Resolves #134714
Note
This change and description were prepared with AI assistance (Anthropic Claude and OpenAI Codex), under my direction. I reviewed them before posting.