Validate addresses before calling DAC to avoid crashes on older runtimes#1380
Validate addresses before calling DAC to avoid crashes on older runtimes#1380leculver merged 3 commits intomicrosoft:mainfrom
Conversation
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>
There was a problem hiding this comment.
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_INVALIDARGfor HResult-returning methods,nullfor string-returning methods - Updated one existing guard (
GetMethodTableData) to also check for zero address alongside the existing TypeHandle check
| if (module == 0) | ||
| return HResult.E_INVALIDARG; |
There was a problem hiding this comment.
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.
| if (heap == 0) | ||
| return HResult.E_INVALIDARG; |
There was a problem hiding this comment.
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.
|
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. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
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>
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).