Fix premature returns for argument/pointer checks in SOSDacImpl.cs - #124814

Merged
rcj1 merged 4 commits into
mainfrom
copilot/fix-argument-pointer-checks
Feb 25, 2026
Merged

Fix premature returns for argument/pointer checks in SOSDacImpl.cs#124814
rcj1 merged 4 commits into
mainfrom
copilot/fix-argument-pointer-checks

Conversation

CopilotAI commented Feb 24, 2026

Copy link
Copy Markdown
Contributor

Description

Argument and pointer validation checks at the start of SOSDacImpl methods were returning early (before the try/catch), bypassing the catch block's HResult conversion and the #if DEBUG legacy comparison block.

Changes

  • E_INVALIDARG checks → moved inside try, replaced return HResults.E_INVALIDARG with throw new ArgumentException()
  • E_POINTER checks → moved inside try, replaced return HResults.E_POINTER with throw new NullReferenceException()
  • if/else guard patterns (ISOSDacInterface14 methods, GetILForModule, GetMethodDescFromToken, TraverseModuleMap) → flattened into unified try/catch with throws
  • Switch default: cases (GetMethodDescFromToken, TraverseModuleMap) → replaced hr = HResults.E_INVALIDARG with throw new ArgumentException() so execution halts rather than falling through
  • GetPEFileName → inner return HResults.E_NOTIMPL inside try replaced with throw new NotImplementedException() to prevent CopyStringToBuffer from running on empty paths
  • SOSMethodEnum.GetCount → added try/catch wrapper; return HResults.E_POINTERthrow new NullReferenceException()
  • GetThreadFromThinlockID → fixed latent bug: null check was setting hr but not preventing the try block from running, causing *pThread dereference on a null pointer

Before / After pattern:

// Before — bypasses catch and #if DEBUG blockintISOSDacInterface.GetThreadData(ClrDataAddressthread,DacpThreadData*data){if(thread==0||data==null)returnHResults.E_INVALIDARG;// skips try/catch and DEBUG comparisoninthr=HResults.S_OK;try{ ...}catch(Exceptionex){hr=ex.HResult;}
...}// After — check participates in unified error handlingintISOSDacInterface.GetThreadData(ClrDataAddressthread,DacpThreadData*data){inthr=HResults.S_OK;try{if(thread==0||data==null)thrownewArgumentException();
...}catch(Exceptionex){hr=ex.HResult;}
...}

Testing

  • All 832 existing cDAC unit tests pass.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

CopilotAIand others added 2 commits February 24, 2026 20:31
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
…x GetPEFileName E_NOTIMPL
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
CopilotAI changed the title [WIP] Fix argument and pointer checks in SOSDacImpl.csFix premature returns for argument/pointer checks in SOSDacImpl.csFeb 24, 2026

@max-charlambmax-charlamb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, nice cleanup.

Especially good find on the bug in GetThreadFromThinlockID

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@rcj1
rcj1 marked this pull request as ready for review February 24, 2026 21:03
CopilotAI review requested due to automatic review settings February 24, 2026 21:03

CopilotAI 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 pull request refactors error handling in SOSDacImpl.cs by moving argument and pointer validation checks from before try blocks to inside try blocks, converting early returns to thrown exceptions. The goal is to ensure that the catch block can handle HResult conversion and that DEBUG comparison blocks work correctly.

Changes:

  • Moved null pointer and invalid argument checks from before try blocks to inside try blocks across approximately 30 methods
  • Changed early return HResults.E_INVALIDARG statements to throw new ArgumentException() inside try blocks
  • Changed early return HResults.E_POINTER statements to throw new NullReferenceException() inside try blocks
  • Restructured methods like GetILForModule, GetMethodDescFromToken, and TraverseModuleMap from if/else structures to unified try/catch blocks
  • Added try/catch wrapper to SOSMethodEnum.GetCount

@rcj1
rcj1 merged commit 9511672 into mainFeb 25, 2026
56 of 58 checks passed
@rcj1
rcj1 deleted the copilot/fix-argument-pointer-checks branch February 25, 2026 03:51
noahfalk added a commit to noahfalk/runtime that referenced this pull request Feb 26, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
noahfalk added a commit to noahfalk/runtime that referenced this pull request Feb 26, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
noahfalk added a commit to noahfalk/runtime that referenced this pull request Mar 1, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
iremyux pushed a commit to iremyux/dotnet-runtime that referenced this pull request Mar 2, 2026
…otnet#124814)
## Description
Argument and pointer validation checks at the start of `SOSDacImpl`
methods were returning early (before the `try/catch`), bypassing the
catch block's HResult conversion and the `#if DEBUG` legacy comparison
block.
### Changes
- **E_INVALIDARG checks** → moved inside `try`, replaced `return
HResults.E_INVALIDARG` with `throw new ArgumentException()`
- **E_POINTER checks** → moved inside `try`, replaced `return
HResults.E_POINTER` with `throw new NullReferenceException()`
- **`if/else` guard patterns** (ISOSDacInterface14 methods,
`GetILForModule`, `GetMethodDescFromToken`, `TraverseModuleMap`) →
flattened into unified `try/catch` with throws
- **Switch `default:` cases** (`GetMethodDescFromToken`,
`TraverseModuleMap`) → replaced `hr = HResults.E_INVALIDARG` with `throw
new ArgumentException()` so execution halts rather than falling through
- **`GetPEFileName`** → inner `return HResults.E_NOTIMPL` inside `try`
replaced with `throw new NotImplementedException()` to prevent
`CopyStringToBuffer` from running on empty paths
- **`SOSMethodEnum.GetCount`** → added `try/catch` wrapper; `return
HResults.E_POINTER` → `throw new NullReferenceException()`
- **`GetThreadFromThinlockID`** → fixed latent bug: null check was
setting `hr` but not preventing the `try` block from running, causing
`*pThread` dereference on a null pointer
**Before / After pattern:**
```csharp
// Before — bypasses catch and #if DEBUG block
int ISOSDacInterface.GetThreadData(ClrDataAddress thread, DacpThreadData* data)
{
if (thread == 0 || data == null)
return HResults.E_INVALIDARG; // skips try/catch and DEBUG comparison
int hr = HResults.S_OK;
try { ... }
catch (Exception ex) { hr = ex.HResult; }
...
}
// After — check participates in unified error handling
int ISOSDacInterface.GetThreadData(ClrDataAddress thread, DacpThreadData* data)
{
int hr = HResults.S_OK;
try
{
if (thread == 0 || data == null)
throw new ArgumentException();
...
}
catch (Exception ex) { hr = ex.HResult; }
...
}
```
## Testing
- All 832 existing cDAC unit tests pass.
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Mar 27, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@max-charlamb@rcj1
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Fix premature returns for argument/pointer checks in SOSDacImpl.cs - #124814

Merged
rcj1 merged 4 commits into
mainfrom
copilot/fix-argument-pointer-checks
Feb 25, 2026
Merged

Fix premature returns for argument/pointer checks in SOSDacImpl.cs#124814
rcj1 merged 4 commits into
mainfrom
copilot/fix-argument-pointer-checks

Conversation

CopilotAI commented Feb 24, 2026

Copy link
Copy Markdown
Contributor

Description

Argument and pointer validation checks at the start of SOSDacImpl methods were returning early (before the try/catch), bypassing the catch block's HResult conversion and the #if DEBUG legacy comparison block.

Changes

  • E_INVALIDARG checks → moved inside try, replaced return HResults.E_INVALIDARG with throw new ArgumentException()
  • E_POINTER checks → moved inside try, replaced return HResults.E_POINTER with throw new NullReferenceException()
  • if/else guard patterns (ISOSDacInterface14 methods, GetILForModule, GetMethodDescFromToken, TraverseModuleMap) → flattened into unified try/catch with throws
  • Switch default: cases (GetMethodDescFromToken, TraverseModuleMap) → replaced hr = HResults.E_INVALIDARG with throw new ArgumentException() so execution halts rather than falling through
  • GetPEFileName → inner return HResults.E_NOTIMPL inside try replaced with throw new NotImplementedException() to prevent CopyStringToBuffer from running on empty paths
  • SOSMethodEnum.GetCount → added try/catch wrapper; return HResults.E_POINTERthrow new NullReferenceException()
  • GetThreadFromThinlockID → fixed latent bug: null check was setting hr but not preventing the try block from running, causing *pThread dereference on a null pointer

Before / After pattern:

// Before — bypasses catch and #if DEBUG blockintISOSDacInterface.GetThreadData(ClrDataAddressthread,DacpThreadData*data){if(thread==0||data==null)returnHResults.E_INVALIDARG;// skips try/catch and DEBUG comparisoninthr=HResults.S_OK;try{ ...}catch(Exceptionex){hr=ex.HResult;}
...}// After — check participates in unified error handlingintISOSDacInterface.GetThreadData(ClrDataAddressthread,DacpThreadData*data){inthr=HResults.S_OK;try{if(thread==0||data==null)thrownewArgumentException();
...}catch(Exceptionex){hr=ex.HResult;}
...}

Testing

  • All 832 existing cDAC unit tests pass.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

CopilotAIand others added 2 commits February 24, 2026 20:31
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
…x GetPEFileName E_NOTIMPL
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
CopilotAI changed the title [WIP] Fix argument and pointer checks in SOSDacImpl.csFix premature returns for argument/pointer checks in SOSDacImpl.csFeb 24, 2026

@max-charlambmax-charlamb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, nice cleanup.

Especially good find on the bug in GetThreadFromThinlockID

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@rcj1
rcj1 marked this pull request as ready for review February 24, 2026 21:03
CopilotAI review requested due to automatic review settings February 24, 2026 21:03

CopilotAI 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 pull request refactors error handling in SOSDacImpl.cs by moving argument and pointer validation checks from before try blocks to inside try blocks, converting early returns to thrown exceptions. The goal is to ensure that the catch block can handle HResult conversion and that DEBUG comparison blocks work correctly.

Changes:

  • Moved null pointer and invalid argument checks from before try blocks to inside try blocks across approximately 30 methods
  • Changed early return HResults.E_INVALIDARG statements to throw new ArgumentException() inside try blocks
  • Changed early return HResults.E_POINTER statements to throw new NullReferenceException() inside try blocks
  • Restructured methods like GetILForModule, GetMethodDescFromToken, and TraverseModuleMap from if/else structures to unified try/catch blocks
  • Added try/catch wrapper to SOSMethodEnum.GetCount

@rcj1
rcj1 merged commit 9511672 into mainFeb 25, 2026
56 of 58 checks passed
@rcj1
rcj1 deleted the copilot/fix-argument-pointer-checks branch February 25, 2026 03:51
noahfalk added a commit to noahfalk/runtime that referenced this pull request Feb 26, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
noahfalk added a commit to noahfalk/runtime that referenced this pull request Feb 26, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
noahfalk added a commit to noahfalk/runtime that referenced this pull request Mar 1, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
iremyux pushed a commit to iremyux/dotnet-runtime that referenced this pull request Mar 2, 2026
…otnet#124814)
## Description
Argument and pointer validation checks at the start of `SOSDacImpl`
methods were returning early (before the `try/catch`), bypassing the
catch block's HResult conversion and the `#if DEBUG` legacy comparison
block.
### Changes
- **E_INVALIDARG checks** → moved inside `try`, replaced `return
HResults.E_INVALIDARG` with `throw new ArgumentException()`
- **E_POINTER checks** → moved inside `try`, replaced `return
HResults.E_POINTER` with `throw new NullReferenceException()`
- **`if/else` guard patterns** (ISOSDacInterface14 methods,
`GetILForModule`, `GetMethodDescFromToken`, `TraverseModuleMap`) →
flattened into unified `try/catch` with throws
- **Switch `default:` cases** (`GetMethodDescFromToken`,
`TraverseModuleMap`) → replaced `hr = HResults.E_INVALIDARG` with `throw
new ArgumentException()` so execution halts rather than falling through
- **`GetPEFileName`** → inner `return HResults.E_NOTIMPL` inside `try`
replaced with `throw new NotImplementedException()` to prevent
`CopyStringToBuffer` from running on empty paths
- **`SOSMethodEnum.GetCount`** → added `try/catch` wrapper; `return
HResults.E_POINTER` → `throw new NullReferenceException()`
- **`GetThreadFromThinlockID`** → fixed latent bug: null check was
setting `hr` but not preventing the `try` block from running, causing
`*pThread` dereference on a null pointer
**Before / After pattern:**
```csharp
// Before — bypasses catch and #if DEBUG block
int ISOSDacInterface.GetThreadData(ClrDataAddress thread, DacpThreadData* data)
{
if (thread == 0 || data == null)
return HResults.E_INVALIDARG; // skips try/catch and DEBUG comparison
int hr = HResults.S_OK;
try { ... }
catch (Exception ex) { hr = ex.HResult; }
...
}
// After — check participates in unified error handling
int ISOSDacInterface.GetThreadData(ClrDataAddress thread, DacpThreadData* data)
{
int hr = HResults.S_OK;
try
{
if (thread == 0 || data == null)
throw new ArgumentException();
...
}
catch (Exception ex) { hr = ex.HResult; }
...
}
```
## Testing
- All 832 existing cDAC unit tests pass.
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Mar 27, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@max-charlamb@rcj1
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix premature returns for argument/pointer checks in SOSDacImpl.cs - #124814

Merged
rcj1 merged 4 commits into
mainfrom
copilot/fix-argument-pointer-checks
Feb 25, 2026
Merged

Fix premature returns for argument/pointer checks in SOSDacImpl.cs#124814
rcj1 merged 4 commits into
mainfrom
copilot/fix-argument-pointer-checks

Conversation

CopilotAI commented Feb 24, 2026

Copy link
Copy Markdown
Contributor

Description

Argument and pointer validation checks at the start of SOSDacImpl methods were returning early (before the try/catch), bypassing the catch block's HResult conversion and the #if DEBUG legacy comparison block.

Changes

  • E_INVALIDARG checks → moved inside try, replaced return HResults.E_INVALIDARG with throw new ArgumentException()
  • E_POINTER checks → moved inside try, replaced return HResults.E_POINTER with throw new NullReferenceException()
  • if/else guard patterns (ISOSDacInterface14 methods, GetILForModule, GetMethodDescFromToken, TraverseModuleMap) → flattened into unified try/catch with throws
  • Switch default: cases (GetMethodDescFromToken, TraverseModuleMap) → replaced hr = HResults.E_INVALIDARG with throw new ArgumentException() so execution halts rather than falling through
  • GetPEFileName → inner return HResults.E_NOTIMPL inside try replaced with throw new NotImplementedException() to prevent CopyStringToBuffer from running on empty paths
  • SOSMethodEnum.GetCount → added try/catch wrapper; return HResults.E_POINTERthrow new NullReferenceException()
  • GetThreadFromThinlockID → fixed latent bug: null check was setting hr but not preventing the try block from running, causing *pThread dereference on a null pointer

Before / After pattern:

// Before — bypasses catch and #if DEBUG blockintISOSDacInterface.GetThreadData(ClrDataAddressthread,DacpThreadData*data){if(thread==0||data==null)returnHResults.E_INVALIDARG;// skips try/catch and DEBUG comparisoninthr=HResults.S_OK;try{ ...}catch(Exceptionex){hr=ex.HResult;}
...}// After — check participates in unified error handlingintISOSDacInterface.GetThreadData(ClrDataAddressthread,DacpThreadData*data){inthr=HResults.S_OK;try{if(thread==0||data==null)thrownewArgumentException();
...}catch(Exceptionex){hr=ex.HResult;}
...}

Testing

  • All 832 existing cDAC unit tests pass.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

CopilotAIand others added 2 commits February 24, 2026 20:31
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
…x GetPEFileName E_NOTIMPL
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
CopilotAI changed the title [WIP] Fix argument and pointer checks in SOSDacImpl.csFix premature returns for argument/pointer checks in SOSDacImpl.csFeb 24, 2026

@max-charlambmax-charlamb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, nice cleanup.

Especially good find on the bug in GetThreadFromThinlockID

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@rcj1
rcj1 marked this pull request as ready for review February 24, 2026 21:03
CopilotAI review requested due to automatic review settings February 24, 2026 21:03

CopilotAI 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 pull request refactors error handling in SOSDacImpl.cs by moving argument and pointer validation checks from before try blocks to inside try blocks, converting early returns to thrown exceptions. The goal is to ensure that the catch block can handle HResult conversion and that DEBUG comparison blocks work correctly.

Changes:

  • Moved null pointer and invalid argument checks from before try blocks to inside try blocks across approximately 30 methods
  • Changed early return HResults.E_INVALIDARG statements to throw new ArgumentException() inside try blocks
  • Changed early return HResults.E_POINTER statements to throw new NullReferenceException() inside try blocks
  • Restructured methods like GetILForModule, GetMethodDescFromToken, and TraverseModuleMap from if/else structures to unified try/catch blocks
  • Added try/catch wrapper to SOSMethodEnum.GetCount

@rcj1
rcj1 merged commit 9511672 into mainFeb 25, 2026
56 of 58 checks passed
@rcj1
rcj1 deleted the copilot/fix-argument-pointer-checks branch February 25, 2026 03:51
noahfalk added a commit to noahfalk/runtime that referenced this pull request Feb 26, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
noahfalk added a commit to noahfalk/runtime that referenced this pull request Feb 26, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
noahfalk added a commit to noahfalk/runtime that referenced this pull request Mar 1, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
iremyux pushed a commit to iremyux/dotnet-runtime that referenced this pull request Mar 2, 2026
…otnet#124814)
## Description
Argument and pointer validation checks at the start of `SOSDacImpl`
methods were returning early (before the `try/catch`), bypassing the
catch block's HResult conversion and the `#if DEBUG` legacy comparison
block.
### Changes
- **E_INVALIDARG checks** → moved inside `try`, replaced `return
HResults.E_INVALIDARG` with `throw new ArgumentException()`
- **E_POINTER checks** → moved inside `try`, replaced `return
HResults.E_POINTER` with `throw new NullReferenceException()`
- **`if/else` guard patterns** (ISOSDacInterface14 methods,
`GetILForModule`, `GetMethodDescFromToken`, `TraverseModuleMap`) →
flattened into unified `try/catch` with throws
- **Switch `default:` cases** (`GetMethodDescFromToken`,
`TraverseModuleMap`) → replaced `hr = HResults.E_INVALIDARG` with `throw
new ArgumentException()` so execution halts rather than falling through
- **`GetPEFileName`** → inner `return HResults.E_NOTIMPL` inside `try`
replaced with `throw new NotImplementedException()` to prevent
`CopyStringToBuffer` from running on empty paths
- **`SOSMethodEnum.GetCount`** → added `try/catch` wrapper; `return
HResults.E_POINTER` → `throw new NullReferenceException()`
- **`GetThreadFromThinlockID`** → fixed latent bug: null check was
setting `hr` but not preventing the `try` block from running, causing
`*pThread` dereference on a null pointer
**Before / After pattern:**
```csharp
// Before — bypasses catch and #if DEBUG block
int ISOSDacInterface.GetThreadData(ClrDataAddress thread, DacpThreadData* data)
{
if (thread == 0 || data == null)
return HResults.E_INVALIDARG; // skips try/catch and DEBUG comparison
int hr = HResults.S_OK;
try { ... }
catch (Exception ex) { hr = ex.HResult; }
...
}
// After — check participates in unified error handling
int ISOSDacInterface.GetThreadData(ClrDataAddress thread, DacpThreadData* data)
{
int hr = HResults.S_OK;
try
{
if (thread == 0 || data == null)
throw new ArgumentException();
...
}
catch (Exception ex) { hr = ex.HResult; }
...
}
```
## Testing
- All 832 existing cDAC unit tests pass.
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Mar 27, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@max-charlamb@rcj1
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix premature returns for argument/pointer checks in SOSDacImpl.cs - #124814

Merged
rcj1 merged 4 commits into
mainfrom
copilot/fix-argument-pointer-checks
Feb 25, 2026
Merged

Fix premature returns for argument/pointer checks in SOSDacImpl.cs#124814
rcj1 merged 4 commits into
mainfrom
copilot/fix-argument-pointer-checks

Conversation

CopilotAI commented Feb 24, 2026

Copy link
Copy Markdown
Contributor

Description

Argument and pointer validation checks at the start of SOSDacImpl methods were returning early (before the try/catch), bypassing the catch block's HResult conversion and the #if DEBUG legacy comparison block.

Changes

  • E_INVALIDARG checks → moved inside try, replaced return HResults.E_INVALIDARG with throw new ArgumentException()
  • E_POINTER checks → moved inside try, replaced return HResults.E_POINTER with throw new NullReferenceException()
  • if/else guard patterns (ISOSDacInterface14 methods, GetILForModule, GetMethodDescFromToken, TraverseModuleMap) → flattened into unified try/catch with throws
  • Switch default: cases (GetMethodDescFromToken, TraverseModuleMap) → replaced hr = HResults.E_INVALIDARG with throw new ArgumentException() so execution halts rather than falling through
  • GetPEFileName → inner return HResults.E_NOTIMPL inside try replaced with throw new NotImplementedException() to prevent CopyStringToBuffer from running on empty paths
  • SOSMethodEnum.GetCount → added try/catch wrapper; return HResults.E_POINTERthrow new NullReferenceException()
  • GetThreadFromThinlockID → fixed latent bug: null check was setting hr but not preventing the try block from running, causing *pThread dereference on a null pointer

Before / After pattern:

// Before — bypasses catch and #if DEBUG blockintISOSDacInterface.GetThreadData(ClrDataAddressthread,DacpThreadData*data){if(thread==0||data==null)returnHResults.E_INVALIDARG;// skips try/catch and DEBUG comparisoninthr=HResults.S_OK;try{ ...}catch(Exceptionex){hr=ex.HResult;}
...}// After — check participates in unified error handlingintISOSDacInterface.GetThreadData(ClrDataAddressthread,DacpThreadData*data){inthr=HResults.S_OK;try{if(thread==0||data==null)thrownewArgumentException();
...}catch(Exceptionex){hr=ex.HResult;}
...}

Testing

  • All 832 existing cDAC unit tests pass.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

CopilotAIand others added 2 commits February 24, 2026 20:31
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
…x GetPEFileName E_NOTIMPL
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
CopilotAI changed the title [WIP] Fix argument and pointer checks in SOSDacImpl.csFix premature returns for argument/pointer checks in SOSDacImpl.csFeb 24, 2026

@max-charlambmax-charlamb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, nice cleanup.

Especially good find on the bug in GetThreadFromThinlockID

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@rcj1
rcj1 marked this pull request as ready for review February 24, 2026 21:03
CopilotAI review requested due to automatic review settings February 24, 2026 21:03

CopilotAI 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 pull request refactors error handling in SOSDacImpl.cs by moving argument and pointer validation checks from before try blocks to inside try blocks, converting early returns to thrown exceptions. The goal is to ensure that the catch block can handle HResult conversion and that DEBUG comparison blocks work correctly.

Changes:

  • Moved null pointer and invalid argument checks from before try blocks to inside try blocks across approximately 30 methods
  • Changed early return HResults.E_INVALIDARG statements to throw new ArgumentException() inside try blocks
  • Changed early return HResults.E_POINTER statements to throw new NullReferenceException() inside try blocks
  • Restructured methods like GetILForModule, GetMethodDescFromToken, and TraverseModuleMap from if/else structures to unified try/catch blocks
  • Added try/catch wrapper to SOSMethodEnum.GetCount

@rcj1
rcj1 merged commit 9511672 into mainFeb 25, 2026
56 of 58 checks passed
@rcj1
rcj1 deleted the copilot/fix-argument-pointer-checks branch February 25, 2026 03:51
noahfalk added a commit to noahfalk/runtime that referenced this pull request Feb 26, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
noahfalk added a commit to noahfalk/runtime that referenced this pull request Feb 26, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
noahfalk added a commit to noahfalk/runtime that referenced this pull request Mar 1, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
iremyux pushed a commit to iremyux/dotnet-runtime that referenced this pull request Mar 2, 2026
…otnet#124814)
## Description
Argument and pointer validation checks at the start of `SOSDacImpl`
methods were returning early (before the `try/catch`), bypassing the
catch block's HResult conversion and the `#if DEBUG` legacy comparison
block.
### Changes
- **E_INVALIDARG checks** → moved inside `try`, replaced `return
HResults.E_INVALIDARG` with `throw new ArgumentException()`
- **E_POINTER checks** → moved inside `try`, replaced `return
HResults.E_POINTER` with `throw new NullReferenceException()`
- **`if/else` guard patterns** (ISOSDacInterface14 methods,
`GetILForModule`, `GetMethodDescFromToken`, `TraverseModuleMap`) →
flattened into unified `try/catch` with throws
- **Switch `default:` cases** (`GetMethodDescFromToken`,
`TraverseModuleMap`) → replaced `hr = HResults.E_INVALIDARG` with `throw
new ArgumentException()` so execution halts rather than falling through
- **`GetPEFileName`** → inner `return HResults.E_NOTIMPL` inside `try`
replaced with `throw new NotImplementedException()` to prevent
`CopyStringToBuffer` from running on empty paths
- **`SOSMethodEnum.GetCount`** → added `try/catch` wrapper; `return
HResults.E_POINTER` → `throw new NullReferenceException()`
- **`GetThreadFromThinlockID`** → fixed latent bug: null check was
setting `hr` but not preventing the `try` block from running, causing
`*pThread` dereference on a null pointer
**Before / After pattern:**
```csharp
// Before — bypasses catch and #if DEBUG block
int ISOSDacInterface.GetThreadData(ClrDataAddress thread, DacpThreadData* data)
{
if (thread == 0 || data == null)
return HResults.E_INVALIDARG; // skips try/catch and DEBUG comparison
int hr = HResults.S_OK;
try { ... }
catch (Exception ex) { hr = ex.HResult; }
...
}
// After — check participates in unified error handling
int ISOSDacInterface.GetThreadData(ClrDataAddress thread, DacpThreadData* data)
{
int hr = HResults.S_OK;
try
{
if (thread == 0 || data == null)
throw new ArgumentException();
...
}
catch (Exception ex) { hr = ex.HResult; }
...
}
```
## Testing
- All 832 existing cDAC unit tests pass.
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Mar 27, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@max-charlamb@rcj1
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Fix premature returns for argument/pointer checks in SOSDacImpl.cs - #124814

Merged
rcj1 merged 4 commits into
mainfrom
copilot/fix-argument-pointer-checks
Feb 25, 2026
Merged

Fix premature returns for argument/pointer checks in SOSDacImpl.cs#124814
rcj1 merged 4 commits into
mainfrom
copilot/fix-argument-pointer-checks

Conversation

CopilotAI commented Feb 24, 2026

Copy link
Copy Markdown
Contributor

Description

Argument and pointer validation checks at the start of SOSDacImpl methods were returning early (before the try/catch), bypassing the catch block's HResult conversion and the #if DEBUG legacy comparison block.

Changes

  • E_INVALIDARG checks → moved inside try, replaced return HResults.E_INVALIDARG with throw new ArgumentException()
  • E_POINTER checks → moved inside try, replaced return HResults.E_POINTER with throw new NullReferenceException()
  • if/else guard patterns (ISOSDacInterface14 methods, GetILForModule, GetMethodDescFromToken, TraverseModuleMap) → flattened into unified try/catch with throws
  • Switch default: cases (GetMethodDescFromToken, TraverseModuleMap) → replaced hr = HResults.E_INVALIDARG with throw new ArgumentException() so execution halts rather than falling through
  • GetPEFileName → inner return HResults.E_NOTIMPL inside try replaced with throw new NotImplementedException() to prevent CopyStringToBuffer from running on empty paths
  • SOSMethodEnum.GetCount → added try/catch wrapper; return HResults.E_POINTERthrow new NullReferenceException()
  • GetThreadFromThinlockID → fixed latent bug: null check was setting hr but not preventing the try block from running, causing *pThread dereference on a null pointer

Before / After pattern:

// Before — bypasses catch and #if DEBUG blockintISOSDacInterface.GetThreadData(ClrDataAddressthread,DacpThreadData*data){if(thread==0||data==null)returnHResults.E_INVALIDARG;// skips try/catch and DEBUG comparisoninthr=HResults.S_OK;try{ ...}catch(Exceptionex){hr=ex.HResult;}
...}// After — check participates in unified error handlingintISOSDacInterface.GetThreadData(ClrDataAddressthread,DacpThreadData*data){inthr=HResults.S_OK;try{if(thread==0||data==null)thrownewArgumentException();
...}catch(Exceptionex){hr=ex.HResult;}
...}

Testing

  • All 832 existing cDAC unit tests pass.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

CopilotAIand others added 2 commits February 24, 2026 20:31
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
…x GetPEFileName E_NOTIMPL
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
CopilotAI changed the title [WIP] Fix argument and pointer checks in SOSDacImpl.csFix premature returns for argument/pointer checks in SOSDacImpl.csFeb 24, 2026

@max-charlambmax-charlamb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, nice cleanup.

Especially good find on the bug in GetThreadFromThinlockID

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@rcj1
rcj1 marked this pull request as ready for review February 24, 2026 21:03
CopilotAI review requested due to automatic review settings February 24, 2026 21:03

CopilotAI 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 pull request refactors error handling in SOSDacImpl.cs by moving argument and pointer validation checks from before try blocks to inside try blocks, converting early returns to thrown exceptions. The goal is to ensure that the catch block can handle HResult conversion and that DEBUG comparison blocks work correctly.

Changes:

  • Moved null pointer and invalid argument checks from before try blocks to inside try blocks across approximately 30 methods
  • Changed early return HResults.E_INVALIDARG statements to throw new ArgumentException() inside try blocks
  • Changed early return HResults.E_POINTER statements to throw new NullReferenceException() inside try blocks
  • Restructured methods like GetILForModule, GetMethodDescFromToken, and TraverseModuleMap from if/else structures to unified try/catch blocks
  • Added try/catch wrapper to SOSMethodEnum.GetCount

@rcj1
rcj1 merged commit 9511672 into mainFeb 25, 2026
56 of 58 checks passed
@rcj1
rcj1 deleted the copilot/fix-argument-pointer-checks branch February 25, 2026 03:51
noahfalk added a commit to noahfalk/runtime that referenced this pull request Feb 26, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
noahfalk added a commit to noahfalk/runtime that referenced this pull request Feb 26, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
noahfalk added a commit to noahfalk/runtime that referenced this pull request Mar 1, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
iremyux pushed a commit to iremyux/dotnet-runtime that referenced this pull request Mar 2, 2026
…otnet#124814)
## Description
Argument and pointer validation checks at the start of `SOSDacImpl`
methods were returning early (before the `try/catch`), bypassing the
catch block's HResult conversion and the `#if DEBUG` legacy comparison
block.
### Changes
- **E_INVALIDARG checks** → moved inside `try`, replaced `return
HResults.E_INVALIDARG` with `throw new ArgumentException()`
- **E_POINTER checks** → moved inside `try`, replaced `return
HResults.E_POINTER` with `throw new NullReferenceException()`
- **`if/else` guard patterns** (ISOSDacInterface14 methods,
`GetILForModule`, `GetMethodDescFromToken`, `TraverseModuleMap`) →
flattened into unified `try/catch` with throws
- **Switch `default:` cases** (`GetMethodDescFromToken`,
`TraverseModuleMap`) → replaced `hr = HResults.E_INVALIDARG` with `throw
new ArgumentException()` so execution halts rather than falling through
- **`GetPEFileName`** → inner `return HResults.E_NOTIMPL` inside `try`
replaced with `throw new NotImplementedException()` to prevent
`CopyStringToBuffer` from running on empty paths
- **`SOSMethodEnum.GetCount`** → added `try/catch` wrapper; `return
HResults.E_POINTER` → `throw new NullReferenceException()`
- **`GetThreadFromThinlockID`** → fixed latent bug: null check was
setting `hr` but not preventing the `try` block from running, causing
`*pThread` dereference on a null pointer
**Before / After pattern:**
```csharp
// Before — bypasses catch and #if DEBUG block
int ISOSDacInterface.GetThreadData(ClrDataAddress thread, DacpThreadData* data)
{
if (thread == 0 || data == null)
return HResults.E_INVALIDARG; // skips try/catch and DEBUG comparison
int hr = HResults.S_OK;
try { ... }
catch (Exception ex) { hr = ex.HResult; }
...
}
// After — check participates in unified error handling
int ISOSDacInterface.GetThreadData(ClrDataAddress thread, DacpThreadData* data)
{
int hr = HResults.S_OK;
try
{
if (thread == 0 || data == null)
throw new ArgumentException();
...
}
catch (Exception ex) { hr = ex.HResult; }
...
}
```
## Testing
- All 832 existing cDAC unit tests pass.
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Mar 27, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@max-charlamb@rcj1
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix premature returns for argument/pointer checks in SOSDacImpl.cs - #124814

Merged
rcj1 merged 4 commits into
mainfrom
copilot/fix-argument-pointer-checks
Feb 25, 2026
Merged

Fix premature returns for argument/pointer checks in SOSDacImpl.cs#124814
rcj1 merged 4 commits into
mainfrom
copilot/fix-argument-pointer-checks

Conversation

CopilotAI commented Feb 24, 2026

Copy link
Copy Markdown
Contributor

Description

Argument and pointer validation checks at the start of SOSDacImpl methods were returning early (before the try/catch), bypassing the catch block's HResult conversion and the #if DEBUG legacy comparison block.

Changes

  • E_INVALIDARG checks → moved inside try, replaced return HResults.E_INVALIDARG with throw new ArgumentException()
  • E_POINTER checks → moved inside try, replaced return HResults.E_POINTER with throw new NullReferenceException()
  • if/else guard patterns (ISOSDacInterface14 methods, GetILForModule, GetMethodDescFromToken, TraverseModuleMap) → flattened into unified try/catch with throws
  • Switch default: cases (GetMethodDescFromToken, TraverseModuleMap) → replaced hr = HResults.E_INVALIDARG with throw new ArgumentException() so execution halts rather than falling through
  • GetPEFileName → inner return HResults.E_NOTIMPL inside try replaced with throw new NotImplementedException() to prevent CopyStringToBuffer from running on empty paths
  • SOSMethodEnum.GetCount → added try/catch wrapper; return HResults.E_POINTERthrow new NullReferenceException()
  • GetThreadFromThinlockID → fixed latent bug: null check was setting hr but not preventing the try block from running, causing *pThread dereference on a null pointer

Before / After pattern:

// Before — bypasses catch and #if DEBUG blockintISOSDacInterface.GetThreadData(ClrDataAddressthread,DacpThreadData*data){if(thread==0||data==null)returnHResults.E_INVALIDARG;// skips try/catch and DEBUG comparisoninthr=HResults.S_OK;try{ ...}catch(Exceptionex){hr=ex.HResult;}
...}// After — check participates in unified error handlingintISOSDacInterface.GetThreadData(ClrDataAddressthread,DacpThreadData*data){inthr=HResults.S_OK;try{if(thread==0||data==null)thrownewArgumentException();
...}catch(Exceptionex){hr=ex.HResult;}
...}

Testing

  • All 832 existing cDAC unit tests pass.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

CopilotAIand others added 2 commits February 24, 2026 20:31
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
…x GetPEFileName E_NOTIMPL
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
CopilotAI changed the title [WIP] Fix argument and pointer checks in SOSDacImpl.csFix premature returns for argument/pointer checks in SOSDacImpl.csFeb 24, 2026

@max-charlambmax-charlamb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, nice cleanup.

Especially good find on the bug in GetThreadFromThinlockID

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@rcj1
rcj1 marked this pull request as ready for review February 24, 2026 21:03
CopilotAI review requested due to automatic review settings February 24, 2026 21:03

CopilotAI 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 pull request refactors error handling in SOSDacImpl.cs by moving argument and pointer validation checks from before try blocks to inside try blocks, converting early returns to thrown exceptions. The goal is to ensure that the catch block can handle HResult conversion and that DEBUG comparison blocks work correctly.

Changes:

  • Moved null pointer and invalid argument checks from before try blocks to inside try blocks across approximately 30 methods
  • Changed early return HResults.E_INVALIDARG statements to throw new ArgumentException() inside try blocks
  • Changed early return HResults.E_POINTER statements to throw new NullReferenceException() inside try blocks
  • Restructured methods like GetILForModule, GetMethodDescFromToken, and TraverseModuleMap from if/else structures to unified try/catch blocks
  • Added try/catch wrapper to SOSMethodEnum.GetCount

@rcj1
rcj1 merged commit 9511672 into mainFeb 25, 2026
56 of 58 checks passed
@rcj1
rcj1 deleted the copilot/fix-argument-pointer-checks branch February 25, 2026 03:51
noahfalk added a commit to noahfalk/runtime that referenced this pull request Feb 26, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
noahfalk added a commit to noahfalk/runtime that referenced this pull request Feb 26, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
noahfalk added a commit to noahfalk/runtime that referenced this pull request Mar 1, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
iremyux pushed a commit to iremyux/dotnet-runtime that referenced this pull request Mar 2, 2026
…otnet#124814)
## Description
Argument and pointer validation checks at the start of `SOSDacImpl`
methods were returning early (before the `try/catch`), bypassing the
catch block's HResult conversion and the `#if DEBUG` legacy comparison
block.
### Changes
- **E_INVALIDARG checks** → moved inside `try`, replaced `return
HResults.E_INVALIDARG` with `throw new ArgumentException()`
- **E_POINTER checks** → moved inside `try`, replaced `return
HResults.E_POINTER` with `throw new NullReferenceException()`
- **`if/else` guard patterns** (ISOSDacInterface14 methods,
`GetILForModule`, `GetMethodDescFromToken`, `TraverseModuleMap`) →
flattened into unified `try/catch` with throws
- **Switch `default:` cases** (`GetMethodDescFromToken`,
`TraverseModuleMap`) → replaced `hr = HResults.E_INVALIDARG` with `throw
new ArgumentException()` so execution halts rather than falling through
- **`GetPEFileName`** → inner `return HResults.E_NOTIMPL` inside `try`
replaced with `throw new NotImplementedException()` to prevent
`CopyStringToBuffer` from running on empty paths
- **`SOSMethodEnum.GetCount`** → added `try/catch` wrapper; `return
HResults.E_POINTER` → `throw new NullReferenceException()`
- **`GetThreadFromThinlockID`** → fixed latent bug: null check was
setting `hr` but not preventing the `try` block from running, causing
`*pThread` dereference on a null pointer
**Before / After pattern:**
```csharp
// Before — bypasses catch and #if DEBUG block
int ISOSDacInterface.GetThreadData(ClrDataAddress thread, DacpThreadData* data)
{
if (thread == 0 || data == null)
return HResults.E_INVALIDARG; // skips try/catch and DEBUG comparison
int hr = HResults.S_OK;
try { ... }
catch (Exception ex) { hr = ex.HResult; }
...
}
// After — check participates in unified error handling
int ISOSDacInterface.GetThreadData(ClrDataAddress thread, DacpThreadData* data)
{
int hr = HResults.S_OK;
try
{
if (thread == 0 || data == null)
throw new ArgumentException();
...
}
catch (Exception ex) { hr = ex.HResult; }
...
}
```
## Testing
- All 832 existing cDAC unit tests pass.
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Mar 27, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@max-charlamb@rcj1
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix premature returns for argument/pointer checks in SOSDacImpl.cs - #124814

Merged
rcj1 merged 4 commits into
mainfrom
copilot/fix-argument-pointer-checks
Feb 25, 2026
Merged

Fix premature returns for argument/pointer checks in SOSDacImpl.cs#124814
rcj1 merged 4 commits into
mainfrom
copilot/fix-argument-pointer-checks

Conversation

CopilotAI commented Feb 24, 2026

Copy link
Copy Markdown
Contributor

Description

Argument and pointer validation checks at the start of SOSDacImpl methods were returning early (before the try/catch), bypassing the catch block's HResult conversion and the #if DEBUG legacy comparison block.

Changes

  • E_INVALIDARG checks → moved inside try, replaced return HResults.E_INVALIDARG with throw new ArgumentException()
  • E_POINTER checks → moved inside try, replaced return HResults.E_POINTER with throw new NullReferenceException()
  • if/else guard patterns (ISOSDacInterface14 methods, GetILForModule, GetMethodDescFromToken, TraverseModuleMap) → flattened into unified try/catch with throws
  • Switch default: cases (GetMethodDescFromToken, TraverseModuleMap) → replaced hr = HResults.E_INVALIDARG with throw new ArgumentException() so execution halts rather than falling through
  • GetPEFileName → inner return HResults.E_NOTIMPL inside try replaced with throw new NotImplementedException() to prevent CopyStringToBuffer from running on empty paths
  • SOSMethodEnum.GetCount → added try/catch wrapper; return HResults.E_POINTERthrow new NullReferenceException()
  • GetThreadFromThinlockID → fixed latent bug: null check was setting hr but not preventing the try block from running, causing *pThread dereference on a null pointer

Before / After pattern:

// Before — bypasses catch and #if DEBUG blockintISOSDacInterface.GetThreadData(ClrDataAddressthread,DacpThreadData*data){if(thread==0||data==null)returnHResults.E_INVALIDARG;// skips try/catch and DEBUG comparisoninthr=HResults.S_OK;try{ ...}catch(Exceptionex){hr=ex.HResult;}
...}// After — check participates in unified error handlingintISOSDacInterface.GetThreadData(ClrDataAddressthread,DacpThreadData*data){inthr=HResults.S_OK;try{if(thread==0||data==null)thrownewArgumentException();
...}catch(Exceptionex){hr=ex.HResult;}
...}

Testing

  • All 832 existing cDAC unit tests pass.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

CopilotAIand others added 2 commits February 24, 2026 20:31
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
…x GetPEFileName E_NOTIMPL
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
CopilotAI changed the title [WIP] Fix argument and pointer checks in SOSDacImpl.csFix premature returns for argument/pointer checks in SOSDacImpl.csFeb 24, 2026

@max-charlambmax-charlamb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, nice cleanup.

Especially good find on the bug in GetThreadFromThinlockID

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@rcj1
rcj1 marked this pull request as ready for review February 24, 2026 21:03
CopilotAI review requested due to automatic review settings February 24, 2026 21:03

CopilotAI 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 pull request refactors error handling in SOSDacImpl.cs by moving argument and pointer validation checks from before try blocks to inside try blocks, converting early returns to thrown exceptions. The goal is to ensure that the catch block can handle HResult conversion and that DEBUG comparison blocks work correctly.

Changes:

  • Moved null pointer and invalid argument checks from before try blocks to inside try blocks across approximately 30 methods
  • Changed early return HResults.E_INVALIDARG statements to throw new ArgumentException() inside try blocks
  • Changed early return HResults.E_POINTER statements to throw new NullReferenceException() inside try blocks
  • Restructured methods like GetILForModule, GetMethodDescFromToken, and TraverseModuleMap from if/else structures to unified try/catch blocks
  • Added try/catch wrapper to SOSMethodEnum.GetCount

@rcj1
rcj1 merged commit 9511672 into mainFeb 25, 2026
56 of 58 checks passed
@rcj1
rcj1 deleted the copilot/fix-argument-pointer-checks branch February 25, 2026 03:51
noahfalk added a commit to noahfalk/runtime that referenced this pull request Feb 26, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
noahfalk added a commit to noahfalk/runtime that referenced this pull request Feb 26, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
noahfalk added a commit to noahfalk/runtime that referenced this pull request Mar 1, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
iremyux pushed a commit to iremyux/dotnet-runtime that referenced this pull request Mar 2, 2026
…otnet#124814)
## Description
Argument and pointer validation checks at the start of `SOSDacImpl`
methods were returning early (before the `try/catch`), bypassing the
catch block's HResult conversion and the `#if DEBUG` legacy comparison
block.
### Changes
- **E_INVALIDARG checks** → moved inside `try`, replaced `return
HResults.E_INVALIDARG` with `throw new ArgumentException()`
- **E_POINTER checks** → moved inside `try`, replaced `return
HResults.E_POINTER` with `throw new NullReferenceException()`
- **`if/else` guard patterns** (ISOSDacInterface14 methods,
`GetILForModule`, `GetMethodDescFromToken`, `TraverseModuleMap`) →
flattened into unified `try/catch` with throws
- **Switch `default:` cases** (`GetMethodDescFromToken`,
`TraverseModuleMap`) → replaced `hr = HResults.E_INVALIDARG` with `throw
new ArgumentException()` so execution halts rather than falling through
- **`GetPEFileName`** → inner `return HResults.E_NOTIMPL` inside `try`
replaced with `throw new NotImplementedException()` to prevent
`CopyStringToBuffer` from running on empty paths
- **`SOSMethodEnum.GetCount`** → added `try/catch` wrapper; `return
HResults.E_POINTER` → `throw new NullReferenceException()`
- **`GetThreadFromThinlockID`** → fixed latent bug: null check was
setting `hr` but not preventing the `try` block from running, causing
`*pThread` dereference on a null pointer
**Before / After pattern:**
```csharp
// Before — bypasses catch and #if DEBUG block
int ISOSDacInterface.GetThreadData(ClrDataAddress thread, DacpThreadData* data)
{
if (thread == 0 || data == null)
return HResults.E_INVALIDARG; // skips try/catch and DEBUG comparison
int hr = HResults.S_OK;
try { ... }
catch (Exception ex) { hr = ex.HResult; }
...
}
// After — check participates in unified error handling
int ISOSDacInterface.GetThreadData(ClrDataAddress thread, DacpThreadData* data)
{
int hr = HResults.S_OK;
try
{
if (thread == 0 || data == null)
throw new ArgumentException();
...
}
catch (Exception ex) { hr = ex.HResult; }
...
}
```
## Testing
- All 832 existing cDAC unit tests pass.
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Mar 27, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@max-charlamb@rcj1
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Fix premature returns for argument/pointer checks in SOSDacImpl.cs - #124814

Merged
rcj1 merged 4 commits into
mainfrom
copilot/fix-argument-pointer-checks
Feb 25, 2026
Merged

Fix premature returns for argument/pointer checks in SOSDacImpl.cs#124814
rcj1 merged 4 commits into
mainfrom
copilot/fix-argument-pointer-checks

Conversation

CopilotAI commented Feb 24, 2026

Copy link
Copy Markdown
Contributor

Description

Argument and pointer validation checks at the start of SOSDacImpl methods were returning early (before the try/catch), bypassing the catch block's HResult conversion and the #if DEBUG legacy comparison block.

Changes

  • E_INVALIDARG checks → moved inside try, replaced return HResults.E_INVALIDARG with throw new ArgumentException()
  • E_POINTER checks → moved inside try, replaced return HResults.E_POINTER with throw new NullReferenceException()
  • if/else guard patterns (ISOSDacInterface14 methods, GetILForModule, GetMethodDescFromToken, TraverseModuleMap) → flattened into unified try/catch with throws
  • Switch default: cases (GetMethodDescFromToken, TraverseModuleMap) → replaced hr = HResults.E_INVALIDARG with throw new ArgumentException() so execution halts rather than falling through
  • GetPEFileName → inner return HResults.E_NOTIMPL inside try replaced with throw new NotImplementedException() to prevent CopyStringToBuffer from running on empty paths
  • SOSMethodEnum.GetCount → added try/catch wrapper; return HResults.E_POINTERthrow new NullReferenceException()
  • GetThreadFromThinlockID → fixed latent bug: null check was setting hr but not preventing the try block from running, causing *pThread dereference on a null pointer

Before / After pattern:

// Before — bypasses catch and #if DEBUG blockintISOSDacInterface.GetThreadData(ClrDataAddressthread,DacpThreadData*data){if(thread==0||data==null)returnHResults.E_INVALIDARG;// skips try/catch and DEBUG comparisoninthr=HResults.S_OK;try{ ...}catch(Exceptionex){hr=ex.HResult;}
...}// After — check participates in unified error handlingintISOSDacInterface.GetThreadData(ClrDataAddressthread,DacpThreadData*data){inthr=HResults.S_OK;try{if(thread==0||data==null)thrownewArgumentException();
...}catch(Exceptionex){hr=ex.HResult;}
...}

Testing

  • All 832 existing cDAC unit tests pass.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

CopilotAIand others added 2 commits February 24, 2026 20:31
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
…x GetPEFileName E_NOTIMPL
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
CopilotAI changed the title [WIP] Fix argument and pointer checks in SOSDacImpl.csFix premature returns for argument/pointer checks in SOSDacImpl.csFeb 24, 2026

@max-charlambmax-charlamb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, nice cleanup.

Especially good find on the bug in GetThreadFromThinlockID

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@rcj1
rcj1 marked this pull request as ready for review February 24, 2026 21:03
CopilotAI review requested due to automatic review settings February 24, 2026 21:03

CopilotAI 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 pull request refactors error handling in SOSDacImpl.cs by moving argument and pointer validation checks from before try blocks to inside try blocks, converting early returns to thrown exceptions. The goal is to ensure that the catch block can handle HResult conversion and that DEBUG comparison blocks work correctly.

Changes:

  • Moved null pointer and invalid argument checks from before try blocks to inside try blocks across approximately 30 methods
  • Changed early return HResults.E_INVALIDARG statements to throw new ArgumentException() inside try blocks
  • Changed early return HResults.E_POINTER statements to throw new NullReferenceException() inside try blocks
  • Restructured methods like GetILForModule, GetMethodDescFromToken, and TraverseModuleMap from if/else structures to unified try/catch blocks
  • Added try/catch wrapper to SOSMethodEnum.GetCount

@rcj1
rcj1 merged commit 9511672 into mainFeb 25, 2026
56 of 58 checks passed
@rcj1
rcj1 deleted the copilot/fix-argument-pointer-checks branch February 25, 2026 03:51
noahfalk added a commit to noahfalk/runtime that referenced this pull request Feb 26, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
noahfalk added a commit to noahfalk/runtime that referenced this pull request Feb 26, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
noahfalk added a commit to noahfalk/runtime that referenced this pull request Mar 1, 2026
…atch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods
- Move null/validation checks inside try/catch blocks per PR dotnet#124814 pattern
- Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count
- Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data
- Use 'is null'/'is not null' instead of '== null'/'!= null'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
iremyux pushed a commit to iremyux/dotnet-runtime that referenced this pull request Mar 2, 2026
…otnet#124814)
## Description
Argument and pointer validation checks at the start of `SOSDacImpl`
methods were returning early (before the `try/catch`), bypassing the
catch block's HResult conversion and the `#if DEBUG` legacy comparison
block.
### Changes
- **E_INVALIDARG checks** → moved inside `try`, replaced `return
HResults.E_INVALIDARG` with `throw new ArgumentException()`
- **E_POINTER checks** → moved inside `try`, replaced `return
HResults.E_POINTER` with `throw new NullReferenceException()`
- **`if/else` guard patterns** (ISOSDacInterface14 methods,
`GetILForModule`, `GetMethodDescFromToken`, `TraverseModuleMap`) →
flattened into unified `try/catch` with throws
- **Switch `default:` cases** (`GetMethodDescFromToken`,
`TraverseModuleMap`) → replaced `hr = HResults.E_INVALIDARG` with `throw
new ArgumentException()` so execution halts rather than falling through
- **`GetPEFileName`** → inner `return HResults.E_NOTIMPL` inside `try`
replaced with `throw new NotImplementedException()` to prevent
`CopyStringToBuffer` from running on empty paths
- **`SOSMethodEnum.GetCount`** → added `try/catch` wrapper; `return
HResults.E_POINTER` → `throw new NullReferenceException()`
- **`GetThreadFromThinlockID`** → fixed latent bug: null check was
setting `hr` but not preventing the `try` block from running, causing
`*pThread` dereference on a null pointer
**Before / After pattern:**
```csharp
// Before — bypasses catch and #if DEBUG block
int ISOSDacInterface.GetThreadData(ClrDataAddress thread, DacpThreadData* data)
{
if (thread == 0 || data == null)
return HResults.E_INVALIDARG; // skips try/catch and DEBUG comparison
int hr = HResults.S_OK;
try { ... }
catch (Exception ex) { hr = ex.HResult; }
...
}
// After — check participates in unified error handling
int ISOSDacInterface.GetThreadData(ClrDataAddress thread, DacpThreadData* data)
{
int hr = HResults.S_OK;
try
{
if (thread == 0 || data == null)
throw new ArgumentException();
...
}
catch (Exception ex) { hr = ex.HResult; }
...
}
```
## Testing
- All 832 existing cDAC unit tests pass.
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Mar 27, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@max-charlamb@rcj1