Repository navigation
[cDAC] Make AMD64 unwinding breakpoint-aware - #15
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: dotnet#131896 Source head: d8a4f9c
✅ Shipwright · ApproveRecommendation: approve PR #15 · Tier
Findings (5)
Fireworks usage: 20,516 input · 802 output · 21,318 total tokens · $0.0050 · 14s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/in/4490927?s=60&v=4){kind=small}
| TargetPointer patchTablePointerAddress = target.ReadGlobalPointer(Constants.Globals.DebuggerPatchTable); | ||
| TargetPointer patchTableAddress = target.ReadPointer(patchTablePointerAddress); | ||
| Data.DebuggerPatchTable patchTable = target.ProcessedData.GetOrAdd<Data.DebuggerPatchTable>(patchTableAddress); | ||
| int opcodeOffset = target.GetTypeInfo(DataType.DebuggerControllerPatch).Fields["Opcode"].Offset; |
There was a problem hiding this comment.
Shipwright · HIGH
The test ReadInstructionByte_RefreshesCachedPatchesAfterFlush writes the new opcode at patchTable.Entries.Value + opcodeOffset, which only works because the first entry is the only
Impact: The test ReadInstructionByte_RefreshesCachedPatchesAfterFlush writes the new opcode at patchTable.Entries.Value + opcodeOffset, which only works because the first entry is the only entry and the opcode field is at that offset within the first entry. A future test with multiple entries or a different layout would silently write to the wrong location. The test should compute the entry address as Entries + i * patchSiz…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| return []; | ||
|
|
||
| Data.DebuggerPatchTable patchTable = _target.ProcessedData.GetOrAdd<Data.DebuggerPatchTable>(patchTableAddress); | ||
| if (patchTable.Entries == TargetPointer.Null) |
There was a problem hiding this comment.
Shipwright · HIGH
ReadPatches() iterates patchTable.Count entries and reads each DebuggerControllerPatch from target memory without validating Count against the actual backing array size or checking
Impact: ReadPatches() iterates patchTable.Count entries and reads each DebuggerControllerPatch from target memory without validating Count against the actual backing array size or checking for integer overflow in patchAddress = Entries + i * patchSize. A corrupt or malicious dump with a huge Count (uint32 max) will cause an unbounded loop and repeated target reads, potentially hanging or exhausting resources. The contract t…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| return HijackKind.None; | ||
| } | ||
|
|
||
| byte IDebugger.ReadInstructionByte(TargetPointer address) |
There was a problem hiding this comment.
Shipwright · HIGH
ReadInstructionByte catches VirtualReadException around patch metadata lookup and silently falls back to reading the instruction directly.
Impact: ReadInstructionByte catches VirtualReadException around patch metadata lookup and silently falls back to reading the instruction directly. If the patch table is unreadable due to corruption, the unwinder will decode the breakpoint opcode (0xCC) as a real instruction, producing incorrect stack walks. This is a correctness/security boundary issue: the fallback masks a failure that should be surfaced or at least logged…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| Dictionary<TargetPointer, byte> patches = []; | ||
| uint patchSize = Data.DebuggerControllerPatch.GetSize(_target); | ||
|
|
||
| for (uint i = 0; i < patchTable.Count; i++) |
There was a problem hiding this comment.
Shipwright · LOW
ReadPatches() uses patch.Opcode.Value != 0 to decide whether to include a patch.
Impact: ReadPatches() uses patch.Opcode.Value != 0 to decide whether to include a patch. If a legitimate original opcode is 0x00 (e.g., add [rax], al), the patch will be skipped and the unwinder will read the breakpoint byte instead. The condition should be based on CodeAddress != Null alone, or the contract should document that opcode 0 is never a valid patched instruction.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| TargetPointer patchTablePointerAddress = target.ReadGlobalPointer(Constants.Globals.DebuggerPatchTable); | ||
| TargetPointer patchTableAddress = target.ReadPointer(patchTablePointerAddress); | ||
| Data.DebuggerPatchTable patchTable = target.ProcessedData.GetOrAdd<Data.DebuggerPatchTable>(patchTableAddress); | ||
| int opcodeOffset = target.GetTypeInfo(DataType.DebuggerControllerPatch).Fields["Opcode"].Offset; |
There was a problem hiding this comment.
Shipwright · LOW
The test 'ReadInstructionByte_RefreshesCachedPatchesAfterFlush' writes the new opcode at 'patchTable.Entries.Value + opcodeOffset', but the opcode field is at 'opcodeOffset' within
Impact: The test 'ReadInstructionByte_RefreshesCachedPatchesAfterFlush' writes the new opcode at 'patchTable.Entries.Value + opcodeOffset', but the opcode field is at 'opcodeOffset' within each 'DebuggerControllerPatch' entry. Since the first entry starts at 'Entries', this happens to target the first entry's opcode, which is the only entry in the test. This is fragile but not a functional failure for the current test. Howe…
Suggested fix: Fix the review finding before release.
1 participant
Fixes dotnet#112273.
The AMD64 unwinder disassembles the current instruction stream to detect and emulate version 1 epilogues. When a debugger breakpoint replaces an epilogue instruction with
int3, cDAC could fail to recognize the epilogue and produce an incomplete stack walk.This change:
The end-to-end regression test added internally.
Validation:
build.cmd tools.cdac -rc releaseDebuggerTestsNote
This PR description was generated with GitHub Copilot.
Source merge-base:
ca4ed7d4a265c32e5240863c6b8ff45121339cc4Source head:
d8a4f9c796cf05e71826405c581e3c74e8c101f1