Skip to content

Comments

Validate addresses before calling DAC to avoid crashes on older runtimes#1380

Merged
leculver merged 3 commits intomicrosoft:mainfrom
leculver:issue_124640
Feb 24, 2026
Merged

Validate addresses before calling DAC to avoid crashes on older runtimes#1380
leculver merged 3 commits intomicrosoft:mainfrom
leculver:issue_124640

Conversation

@leculver
Copy link
Contributor

Add zero-address guards to GetAppDomainName, TraverseModuleMap, TraverseStubHeap, GetFieldInfo, and GetMethodTableData. These prevent passing null/zero to DAC methods that crash without null checks on older runtimes (dotnet/runtime#124640).

Add zero-address guards to GetAppDomainName, TraverseModuleMap,
TraverseStubHeap, GetFieldInfo, and GetMethodTableData. These prevent
passing null/zero to DAC methods that crash without null checks on
older runtimes (dotnet/runtime#124640).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@leculver leculver marked this pull request as draft February 20, 2026 21:47
@leculver leculver marked this pull request as ready for review February 20, 2026 21:48
@leculver leculver requested a review from Copilot February 20, 2026 21:48
Copy link
Contributor

Copilot AI left a comment

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 adds defensive null/zero-address checks to five DAC interface methods to prevent crashes on older .NET runtimes that lack built-in null checks (per dotnet/runtime#124640). The changes ensure that ClrMD gracefully handles zero addresses before passing them to potentially unsafe DAC methods.

Changes:

  • Added zero-address guards to five DAC interface methods that crash on older runtimes when passed null/zero pointers
  • Methods return appropriate error values: HResult.E_INVALIDARG for HResult-returning methods, null for string-returning methods
  • Updated one existing guard (GetMethodTableData) to also check for zero address alongside the existing TypeHandle check

Comment on lines +674 to +675
if (module == 0)
return HResult.E_INVALIDARG;
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

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

Inconsistent brace usage. According to .editorconfig line 275, braces are preferred (csharp_prefer_braces = true:warning). This simple return statement doesn't use braces, but GetFieldInfo (lines 298-302) uses braces for the same pattern. For consistency, consider either using braces here and in GetAppDomainName, or removing braces from GetFieldInfo.

Copilot uses AI. Check for mistakes.
Comment on lines +703 to +704
if (heap == 0)
return HResult.E_INVALIDARG;
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

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

Inconsistent brace usage. According to .editorconfig line 275, braces are preferred (csharp_prefer_braces = true:warning). This simple return statement doesn't use braces, but GetFieldInfo (lines 298-302) uses braces for the same pattern. For consistency, consider either using braces here and in GetAppDomainName and TraverseModuleMap, or removing braces from GetFieldInfo.

Copilot uses AI. Check for mistakes.
@timcassell
Copy link
Contributor

There were also crashes due to -1 addresses in older runtimes (net7 and earlier). In BDN we just filter the entire invalid address range. Not sure if that strategy makes sense for every API, but maybe worth considering.

https://github.com/dotnet/BenchmarkDotNet/blob/f54f764266722c6eba4e6f6ded8c808a4012a34e/src/BenchmarkDotNet/Disassemblers/ClrMdDisassembler.cs#L19-L44

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@leculver
Copy link
Contributor Author

I'm trying to sweep the dac for these kinds of failures: https://github.com/leculver/dac-tests/ dotnet/runtime#124686

The real issue is that 20 years ago the design/architecture was that throwing any exception (including accessviolation) could be swallowed by a try/catch block and ignored in debugging. Ignoring how obviously a bad idea that was, it simply doesn't work on Linux/OSX, can't "handle" a sigsegv. The design is broken all the way down.

Right now these fixes are just trying to clean up the worst of them/low hanging fruit. The new cDac effort in the runtime is making huge progress, and one day we should be able to just use a better interface.

…tnet/runtime#124640)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@leculver leculver merged commit 73dca27 into microsoft:main Feb 24, 2026
8 checks passed
@leculver leculver deleted the issue_124640 branch February 24, 2026 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants