From f43b2299157212205b18090fcf69562b9beb245f Mon Sep 17 00:00:00 2001 From: Max Charlamb <44248479+max-charlamb@users.noreply.github.com> Date: Thu, 26 Feb 2026 11:52:01 -0500 Subject: [PATCH] update type validation --- .../TypeValidation.cs | 34 ++++++ ...ostics.DataContractReader.DumpTests.csproj | 1 + .../tests/DumpTests/VarargPInvokeDumpTests.cs | 5 +- .../managed/cdac/tests/MethodTableTests.cs | 112 ++++++++++++++++++ src/native/managed/cdac/tests/TestHelpers.cs | 31 +++++ 5 files changed, 181 insertions(+), 2 deletions(-) create mode 100644 src/native/managed/cdac/tests/TestHelpers.cs diff --git a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/TypeValidation.cs b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/TypeValidation.cs index 7b96dacd78c849..8c53ac0bfe07d8 100644 --- a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/TypeValidation.cs +++ b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/TypeValidation.cs @@ -67,6 +67,8 @@ internal TargetPointer CanonMT } } } + + internal readonly bool ValidateReadable() => ValidateDataReadable(_target, Address); } internal struct NonValidatedEEClass @@ -84,6 +86,8 @@ internal NonValidatedEEClass(Target target, TargetPointer eeClassPointer) } internal TargetPointer MethodTable => _target.ReadPointer(Address + (ulong)_type.Fields[nameof(MethodTable)].Offset); + + internal readonly bool ValidateReadable() => ValidateDataReadable(_target, Address); } internal static NonValidatedMethodTable GetMethodTableData(Target target, TargetPointer methodTablePointer) @@ -108,6 +112,11 @@ private bool ValidateMethodTablePointer(NonValidatedMethodTable umt) { try { + // Make sure that we can read the method table's data. + if (!umt.ValidateReadable()) + { + return false; + } if (!ValidateThrowing(umt)) { return false; @@ -146,6 +155,10 @@ private bool ValidateThrowing(NonValidatedMethodTable methodTable) if (eeClassPtr != TargetPointer.Null) { NonValidatedEEClass eeClass = GetEEClassData(_target, eeClassPtr); + if (!eeClass.ValidateReadable()) + { + return false; + } TargetPointer methodTablePtrFromClass = eeClass.MethodTable; if (methodTable.Address == methodTablePtrFromClass) { @@ -154,6 +167,10 @@ private bool ValidateThrowing(NonValidatedMethodTable methodTable) if (methodTable.Flags.HasInstantiation || methodTable.Flags.IsArray) { NonValidatedMethodTable methodTableFromClass = GetMethodTableData(_target, methodTablePtrFromClass); + if (!methodTableFromClass.ValidateReadable()) + { + return false; + } TargetPointer classFromMethodTable = GetClassThrowing(methodTableFromClass); return classFromMethodTable == eeClassPtr; } @@ -174,6 +191,19 @@ private bool ValidateMethodTableAdHoc(NonValidatedMethodTable methodTable) return true; } + private static bool ValidateDataReadable(Target target, TargetPointer dataAddress) where T : IData + { + try + { + T dataClass = T.Create(target, dataAddress); + return true; + } + catch (VirtualReadException) + { + return false; + } + } + private TargetPointer GetClassThrowing(NonValidatedMethodTable methodTable) { TargetPointer eeClassOrCanonMT = methodTable.EEClassOrCanonMT; @@ -186,6 +216,10 @@ private TargetPointer GetClassThrowing(NonValidatedMethodTable methodTable) { TargetPointer canonicalMethodTablePtr = methodTable.CanonMT; NonValidatedMethodTable umt = GetMethodTableData(_target, canonicalMethodTablePtr); + if (!umt.ValidateReadable()) + { + throw new InvalidOperationException("canon MT is not readable"); + } return umt.EEClass; } } diff --git a/src/native/managed/cdac/tests/DumpTests/Microsoft.Diagnostics.DataContractReader.DumpTests.csproj b/src/native/managed/cdac/tests/DumpTests/Microsoft.Diagnostics.DataContractReader.DumpTests.csproj index 805897bf3c817a..a9f6a176af4d19 100644 --- a/src/native/managed/cdac/tests/DumpTests/Microsoft.Diagnostics.DataContractReader.DumpTests.csproj +++ b/src/native/managed/cdac/tests/DumpTests/Microsoft.Diagnostics.DataContractReader.DumpTests.csproj @@ -26,6 +26,7 @@ + diff --git a/src/native/managed/cdac/tests/DumpTests/VarargPInvokeDumpTests.cs b/src/native/managed/cdac/tests/DumpTests/VarargPInvokeDumpTests.cs index 92ffac5eacba7c..a1ec6c60be2fdc 100644 --- a/src/native/managed/cdac/tests/DumpTests/VarargPInvokeDumpTests.cs +++ b/src/native/managed/cdac/tests/DumpTests/VarargPInvokeDumpTests.cs @@ -7,6 +7,7 @@ using Microsoft.Diagnostics.DataContractReader.Contracts; using Microsoft.Diagnostics.DataContractReader.Legacy; using Xunit; +using static Microsoft.Diagnostics.DataContractReader.Tests.TestHelpers; namespace Microsoft.Diagnostics.DataContractReader.DumpTests; @@ -95,7 +96,7 @@ public unsafe void VarargPInvoke_GetMethodTableDataForILStubFrame(TestConfigurat TargetPointer mt = rts.GetMethodTable(mdHandle); DacpMethodTableData mtData; int hr = sosDac.GetMethodTableData(new ClrDataAddress(mt), &mtData); - Assert.Equal(HResults.S_OK, hr); + AssertHResult(HResults.S_OK, hr); return; } @@ -135,7 +136,7 @@ public unsafe void VarargPInvoke_GetILAddressMapForILStub_ReturnsEFail(TestConfi Target, mdHandle, TargetPointer.Null, legacyImpl: null); uint mapNeeded; int hr = methodInstance.GetILAddressMap(0, &mapNeeded, null); - Assert.Equal(HResults.E_FAIL, hr); + AssertHResult(HResults.E_FAIL, hr); return; } diff --git a/src/native/managed/cdac/tests/MethodTableTests.cs b/src/native/managed/cdac/tests/MethodTableTests.cs index a3e52a592e6de2..245882b82bf75a 100644 --- a/src/native/managed/cdac/tests/MethodTableTests.cs +++ b/src/native/managed/cdac/tests/MethodTableTests.cs @@ -3,9 +3,11 @@ using System; using Microsoft.Diagnostics.DataContractReader.Contracts; +using Microsoft.Diagnostics.DataContractReader.Legacy; using Microsoft.Diagnostics.DataContractReader.RuntimeTypeSystemHelpers; using Moq; using Xunit; +using static Microsoft.Diagnostics.DataContractReader.Tests.TestHelpers; namespace Microsoft.Diagnostics.DataContractReader.Tests; @@ -226,4 +228,114 @@ public void ValidateArrayInstMethodTable(MockTarget.Architecture arch) }); } + + [Theory] + [ClassData(typeof(MockTarget.StdArch))] + public unsafe void GetMethodTableDataReturnsEInvalidArgWhenEEClassPartiallyReadable(MockTarget.Architecture arch) + { + // Reproduces the HResult mismatch from dotnet/diagnostics CI where the cDAC returned + // CORDBG_E_READVIRTUAL_FAILURE (0x80131c49) instead of E_INVALIDARG (0x80070057) + // when GetMethodTableData was called with an address whose MethodTable validation + // passes but subsequent EEClass reads fail. + TargetPointer methodTablePtr = default; + + RTSContractHelper(arch, + (rtsBuilder) => + { + TargetTestHelpers helpers = rtsBuilder.Builder.TargetTestHelpers; + + // Create a full MethodTable that will pass validation + methodTablePtr = rtsBuilder.AddMethodTable("PartialEEClass MT", + mtflags: default, mtflags2: default, baseSize: helpers.ObjectBaseSize, + module: TargetPointer.Null, parentMethodTable: TargetPointer.Null, + numInterfaces: 0, numVirtuals: 3); + + // Create a tiny EEClass fragment — only the MethodTable pointer field. + // Previously, validation only read NonValidatedEEClass.MethodTable (at offset 0), + // so this minimal fragment was sufficient for validation to pass while later + // EEClass reads (for MethodDescChunk, NumMethods, etc.) at subsequent offsets + // would fail with VirtualReadException. After the TypeValidation fix, the full + // EEClass is eagerly read during validation, so this now correctly reports + // E_INVALIDARG instead of surfacing VirtualReadException from those later reads. + int pointerSize = helpers.PointerSize; + MockMemorySpace.HeapFragment tinyEEClass = rtsBuilder.TypeSystemAllocator.Allocate( + (ulong)pointerSize, "Tiny EEClass (MethodTable field only)"); + helpers.WritePointer(tinyEEClass.Data, methodTablePtr); + rtsBuilder.Builder.AddHeapFragment(tinyEEClass); + + // Point the MethodTable's EEClassOrCanonMT at the tiny EEClass + rtsBuilder.SetMethodTableEEClassOrCanonMTRaw(methodTablePtr, tinyEEClass.Address); + }, + (target) => + { + ISOSDacInterface sosDac = new SOSDacImpl(target, legacyObj: null); + + DacpMethodTableData mtData = default; + int hr = sosDac.GetMethodTableData(new ClrDataAddress(methodTablePtr), &mtData); + + // Should return E_INVALIDARG to match legacy DAC behavior + AssertHResult(HResults.E_INVALIDARG, hr); + }); + } + + [Theory] + [ClassData(typeof(MockTarget.StdArch))] + public unsafe void GetMethodTableDataReturnsEInvalidArgWhenMethodTablePartiallyReadable(MockTarget.Architecture arch) + { + // Analogous to the EEClass test above but for the MethodTable itself. + // If a MethodTable is only partially readable (enough for validation fields + // like MTFlags, BaseSize, MTFlags2, EEClassOrCanonMT) but not the full + // structure (Module, ParentMethodTable, etc.), validation currently passes + // because NonValidatedMethodTable reads fields lazily. The subsequent + // Data.MethodTable construction then fails with VirtualReadException, + // surfacing CORDBG_E_READVIRTUAL_FAILURE instead of E_INVALIDARG. + TargetPointer tinyMethodTableAddr = default; + + RTSContractHelper(arch, + (rtsBuilder) => + { + TargetTestHelpers helpers = rtsBuilder.Builder.TargetTestHelpers; + Target.TypeInfo mtTypeInfo = rtsBuilder.Types[DataType.MethodTable]; + + // Create a valid EEClass that will point back to our tiny MethodTable + TargetPointer eeClassPtr = rtsBuilder.AddEEClass("PartialMT EEClass", + attr: 0, numMethods: 0, numNonVirtualSlots: 0); + + // Allocate a tiny MethodTable fragment — only enough for the fields that + // validation reads (MTFlags, BaseSize, MTFlags2, EEClassOrCanonMT) but + // not the full MethodTable. Fields like Module, ParentMethodTable, etc. + // at subsequent offsets will be unreadable. + int eeClassOrCanonMTOffset = mtTypeInfo.Fields[nameof(Data.MethodTable.EEClassOrCanonMT)].Offset; + ulong partialSize = (ulong)(eeClassOrCanonMTOffset + helpers.PointerSize); + MockMemorySpace.HeapFragment tinyMT = rtsBuilder.TypeSystemAllocator.Allocate( + partialSize, "Tiny MethodTable (validation fields only)"); + + Span dest = tinyMT.Data; + helpers.Write(dest.Slice(mtTypeInfo.Fields[nameof(Data.MethodTable.MTFlags)].Offset), (uint)0); + helpers.Write(dest.Slice(mtTypeInfo.Fields[nameof(Data.MethodTable.BaseSize)].Offset), (uint)helpers.ObjectBaseSize); + helpers.Write(dest.Slice(mtTypeInfo.Fields[nameof(Data.MethodTable.MTFlags2)].Offset), (uint)0); + helpers.WritePointer(dest.Slice(eeClassOrCanonMTOffset), eeClassPtr); + + rtsBuilder.Builder.AddHeapFragment(tinyMT); + tinyMethodTableAddr = tinyMT.Address; + + // Point the EEClass back at the tiny MethodTable to pass validation + Target.TypeInfo eeClassTypeInfo = rtsBuilder.Types[DataType.EEClass]; + Span eeClassBytes = rtsBuilder.Builder.BorrowAddressRange( + eeClassPtr, (int)eeClassTypeInfo.Size.Value); + helpers.WritePointer( + eeClassBytes.Slice(eeClassTypeInfo.Fields[nameof(Data.EEClass.MethodTable)].Offset, + helpers.PointerSize), tinyMethodTableAddr); + }, + (target) => + { + ISOSDacInterface sosDac = new SOSDacImpl(target, legacyObj: null); + + DacpMethodTableData mtData = default; + int hr = sosDac.GetMethodTableData(new ClrDataAddress(tinyMethodTableAddr), &mtData); + + // Should return E_INVALIDARG to match legacy DAC behavior + AssertHResult(HResults.E_INVALIDARG, hr); + }); + } } diff --git a/src/native/managed/cdac/tests/TestHelpers.cs b/src/native/managed/cdac/tests/TestHelpers.cs new file mode 100644 index 00000000000000..ffadf6fb55bb95 --- /dev/null +++ b/src/native/managed/cdac/tests/TestHelpers.cs @@ -0,0 +1,31 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Reflection; +using Xunit; + +namespace Microsoft.Diagnostics.DataContractReader.Tests; + +internal static class TestHelpers +{ + internal static string FormatHResult(int hr) + { + string hex = $"0x{unchecked((uint)hr):X8}"; + foreach (Type type in new[] { typeof(HResults), typeof(CorDbgHResults) }) + { + foreach (FieldInfo field in type.GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static)) + { + if (field.FieldType == typeof(int) && (int)field.GetValue(null)! == hr) + return $"{field.Name} ({hex})"; + } + } + return hex; + } + + internal static void AssertHResult(int expected, int actual) + { + Assert.True(expected == actual, + $"Expected: {FormatHResult(expected)}, Actual: {FormatHResult(actual)}"); + } +}