Fix TypeLoadException in GetMarshalAs when SafeArray has zero-length user-defined type name#124355
Fix TypeLoadException in GetMarshalAs when SafeArray has zero-length user-defined type name#124355
Conversation
…with empty SafeArray type name When a MarshalAs blob contains NATIVE_TYPE_SAFEARRAY with a zero-length user-defined type name, the native GetMarshalAs returned a non-null pointer despite having zero bytes. The managed code then used CreateReadOnlySpanFromNullTerminated to decode this pointer, reading past the blob into garbage memory, producing a garbled string that failed type resolution with TypeLoadException. Fix: Return NULL from native code when byte count is 0 for safeArrayUserDefinedSubType, marshalType, and marshalCookie. Also add try/catch around safeArrayUserDefinedType resolution for defense-in-depth, matching the existing pattern for marshalTypeRef. Fixes #115953 Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/area-system-reflection |
|
@copilot Please add a test for this scenario. It should be possible to add validation for the parsing of MarshalAs attributes in under the src/libraries/System.Runtime.InteropServices test suite. |
Verifies that accessing CustomAttributes on a parameter annotated with MarshalAs(UnmanagedType.SafeArray) without SafeArrayUserDefinedSubType does not throw TypeLoadException. Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
Added in 5478b0d. The test ( |
....Runtime.InteropServices.UnitTests/System/Runtime/InteropServices/MarshalAsAttributeTests.cs
Outdated
Show resolved
Hide resolved
| [Theory] | ||
| [InlineData(nameof(ISafeArrayMarshallingTest.Method1))] | ||
| [InlineData(nameof(ISafeArrayMarshallingTest.Method2))] | ||
| public void SafeArrayParameter_NoUserDefinedSubType_CustomAttributesDoNotThrow(string methodName) | ||
| { | ||
| MethodInfo method = typeof(ISafeArrayMarshallingTest).GetMethod(methodName); | ||
| ParameterInfo parameter = method.GetParameters().Single(); | ||
|
|
||
| // Accessing CustomAttributes should not throw TypeLoadException | ||
| // when SafeArrayUserDefinedSubType is not specified. | ||
| var attributes = parameter.CustomAttributes.ToList(); | ||
|
|
||
| MarshalAsAttribute marshalAs = (MarshalAsAttribute)Attribute.GetCustomAttribute(parameter, typeof(MarshalAsAttribute)); | ||
| Assert.NotNull(marshalAs); | ||
| Assert.Equal(UnmanagedType.SafeArray, marshalAs.Value); | ||
| Assert.Null(marshalAs.SafeArrayUserDefinedSubType); | ||
| } | ||
|
|
||
| [ComImport] | ||
| [Guid("1FC06EAF-2B18-4D54-B7D4-E654A8BEEF5B")] | ||
| [InterfaceType(ComInterfaceType.InterfaceIsIDispatch)] | ||
| private interface ISafeArrayMarshallingTest | ||
| { | ||
| void Method1([MarshalAs(UnmanagedType.SafeArray, SafeArraySubType = VarEnum.VT_DISPATCH)] object[] args); | ||
| void Method2([MarshalAs(UnmanagedType.SafeArray, SafeArraySubType = VarEnum.VT_UNKNOWN)] object[] args); | ||
| } |
There was a problem hiding this comment.
I was curious about the metadata shapes this generates so I tried this in a .NET10-windows project. The test passes as-is, without this fix. Is that expected?
There was a problem hiding this comment.
Boo. I was afraid of that. The issue is ParseNativeTypeInfo which is in mlinfo.cpp. It reads into the middle of the data stream and sets the pointer also sets the length. So the pointer may be non-null, but the size is 0. The OP's example fails as expected, but I'm not sure how to force the metadata into that state to be honest.
There was a problem hiding this comment.
Yes, that's expected — the C# compiler omits the zero-length string from the FieldMarshal blob entirely, so the native NATIVE_TYPE_SAFEARRAY parsing never sets m_strSafeArrayUserDefTypeName. The bug only triggers when the blob contains the explicit 0x00 compressed-string-length byte that tlbimp produces (3 bytes: 1D 09 00 vs the 2 bytes 1D 09 from the C# compiler).
Replaced the test in fe17a0b with one that uses PersistedAssemblyBuilder to create a valid PE, then injects the exact problematic FieldMarshal blob bytes [0x1D, 0x09, 0x00] via MetadataBuilder.AddMarshallingDescriptor.
There was a problem hiding this comment.
@copilot and did you confirm that this new test fails without the fix and passes with it? From the session log it looks like you may not have run it without the fix. Please do.
There was a problem hiding this comment.
@copilot I tried the test locally and it does not fail without the fix on Windows. This, however, does:
// Builds a PE with a FieldMarshal blob containing NATIVE_TYPE_SAFEARRAY
// with a zero-length user-defined sub type name followed by poison bytes.
// Without the native fix, the FCALL returns a dangling pointer into the
// poison bytes; the managed side reads "X" and throws TypeLoadException.
using System.Reflection;
using System.Reflection.Emit;
using System.Reflection.Metadata;
using System.Reflection.Metadata.Ecma335;
using System.Reflection.PortableExecutable;
using System.Runtime.Loader;
byte[] peImage = CreatePEWithSafeArrayFieldMarshal();
AssemblyLoadContext alc = new("TestALC", isCollectible: true);
try
{
Assembly asm = alc.LoadFromStream(new MemoryStream(peImage));
Type iface = asm.GetType("TestInterface")!;
MethodInfo method = iface.GetMethod("TestMethod")!;
ParameterInfo param = method.GetParameters()[0];
// Without the fix, this throws TypeLoadException because the native FCALL
// returns a non-null dangling pointer for the zero-length type name string,
// and CreateReadOnlySpanFromNullTerminated reads the poison "X" from it.
_ = param.GetCustomAttributes(false);
Console.WriteLine("PASS: No exception thrown.");
}
catch (TypeLoadException ex)
{
Console.WriteLine($"FAIL: TypeLoadException: {ex.Message}");
}
finally
{
alc.Unload();
}
static byte[] CreatePEWithSafeArrayFieldMarshal()
{
PersistedAssemblyBuilder ab = new(new AssemblyName("SafeArrayTestAsm"), typeof(object).Assembly);
ModuleBuilder moduleDef = ab.DefineDynamicModule("SafeArrayTestAsm.dll");
TypeBuilder typeDef = moduleDef.DefineType("TestInterface",
TypeAttributes.Interface | TypeAttributes.Abstract | TypeAttributes.Public);
MethodBuilder methodDef = typeDef.DefineMethod("TestMethod",
MethodAttributes.Public | MethodAttributes.Abstract | MethodAttributes.Virtual |
MethodAttributes.NewSlot | MethodAttributes.HideBySig,
typeof(void), [typeof(object[])]);
methodDef.DefineParameter(1, ParameterAttributes.HasFieldMarshal, "args");
typeDef.CreateType();
MetadataBuilder mdBuilder = ab.GenerateMetadata(out BlobBuilder ilStream, out _);
// Inject a FieldMarshal blob that reproduces the native bug:
// 0x1D NATIVE_TYPE_SAFEARRAY
// 0x09 VT_DISPATCH (compressed uint)
// 0x00 compressed string length = 0 (empty user-defined type name)
// 0x58 'X' — poison byte, NOT consumed by the parser (it read length 0)
// 0x00 null terminator for CreateReadOnlySpanFromNullTerminated
//
// The native parser sets m_strSafeArrayUserDefTypeName to the current
// position after reading the zero string length, which points at the 'X'.
// m_cSafeArrayUserDefTypeNameBytes is 0. Without the native fix, the
// FCALL returns that non-null pointer; the managed side calls
// CreateReadOnlySpanFromNullTerminated on it, reads "X", and then
// TypeNameResolver throws TypeLoadException trying to resolve it.
//
// With the native fix (return NULL when byte count == 0), the managed
// side sees null and skips type resolution entirely.
//
// Plain {0x1D, 0x09, 0x00} doesn't reliably repro because the blob
// lands near the end of the #Blob heap where null padding follows,
// so the dangling pointer hits 0x00 immediately → empty string → no throw.
BlobBuilder marshalDescriptor = new();
marshalDescriptor.WriteBytes(new byte[] { 0x1D, 0x09, 0x00, 0x58, 0x00 });
mdBuilder.AddMarshallingDescriptor(
MetadataTokens.ParameterHandle(1),
mdBuilder.GetOrAddBlob(marshalDescriptor));
ManagedPEBuilder peAssembler = new(
PEHeaderBuilder.CreateLibraryHeader(),
new MetadataRootBuilder(mdBuilder),
ilStream,
flags: CorFlags.ILOnly);
BlobBuilder peOutput = new();
peAssembler.Serialize(peOutput);
return peOutput.ToArray();
}
There was a problem hiding this comment.
Pull request overview
This PR fixes a TypeLoadException that occurs when accessing CustomAttributes on parameters with [MarshalAs(UnmanagedType.SafeArray)] attributes that don't specify a SafeArrayUserDefinedSubType. The regression was introduced in commit a3dc133 when the native code was changed to return raw byte pointers instead of managed strings.
Changes:
- Fixed native code in
managedmdimport.cppto returnNULLinstead of dangling pointers when string byte counts are zero - Added defensive error handling in
MdImport.csto catchTypeLoadExceptionwhen resolvingsafeArrayUserDefinedType - Added regression test to verify that accessing
CustomAttributesdoesn't throw whenSafeArrayUserDefinedSubTypeis not specified - Fixed typo in test method name from
Ctor_UmanagedTyetoCtor_UnmanagedType
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/coreclr/vm/managedmdimport.cpp |
Added null checks for zero-length strings before returning pointers to prevent dangling pointer issues |
src/coreclr/System.Private.CoreLib/src/System/Reflection/MdImport.cs |
Added try-catch for TypeLoadException when resolving safeArrayUserDefinedType as defense-in-depth |
src/libraries/System.Runtime.InteropServices/tests/System.Runtime.InteropServices.UnitTests/System/Runtime/InteropServices/MarshalAsAttributeTests.cs |
Added regression test and fixed typo in existing test method name |
…blob The C# compiler never generates a zero-length user-defined type name in the FieldMarshal blob - it omits the length byte entirely. The previous test using C# attributes therefore did not exercise the bug. This new test uses PersistedAssemblyBuilder to create a valid PE and then injects a FieldMarshal blob with the exact bytes that tlbimp produces: 0x1D (NATIVE_TYPE_SAFEARRAY) | 0x09 (VT_DISPATCH) | 0x00 (string len 0) The trailing zero-length string is the trigger for the bug. Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
| try | ||
| { | ||
| safeArrayUserDefinedType = string.IsNullOrEmpty(safeArrayUserDefinedTypeName) ? null : | ||
| TypeNameResolver.GetTypeReferencedByCustomAttribute(safeArrayUserDefinedTypeName, scope); |
There was a problem hiding this comment.
Update marshalTypeName resolution below to use IsNullOrEmpty for consistency?
|
@copilot Can you address the comments? |
|
@copilot Can you address the comments? |
|
CCA got into a bad state on this PR. I'm replacing it with #124406. |
Description
MetadataImport.GetMarshalAsreturns raw byte pointers to managed code without their associated lengths. When a MarshalAs blob containsNATIVE_TYPE_SAFEARRAYwith a zero-length user-defined type name (common in tlbimp-generated COM interop assemblies), the native code returned a non-null pointer pointing past the blob data. The managed code then usedCreateReadOnlySpanFromNullTerminatedon this pointer, reading garbage memory until hitting a null byte, producing a garbled string that failed type resolution withTypeLoadException.Regression introduced by commit a3dc133 which switched from returning managed strings to returning raw byte pointers.
Native fix (
managedmdimport.cpp): ReturnNULLwhen byte count is 0 forsafeArrayUserDefinedSubType,marshalType, andmarshalCookie:Managed defense-in-depth (
MdImport.cs): WrappedsafeArrayUserDefinedTyperesolution intry/catch(TypeLoadException), matching the existing pattern formarshalTypeRef.Regression test (
MarshalAsAttributeTests.cs): TheSafeArrayParameter_ZeroLengthUserDefinedSubType_DoesNotThrowtest usesPersistedAssemblyBuilderto create a PE assembly and then injects a FieldMarshal blob with the exact bytes that tlbimp produces:0x1D(NATIVE_TYPE_SAFEARRAY) |0x09(VT_DISPATCH) |0x00(compressed string length 0). The trailing zero-length string is what distinguishes this from blobs the C# compiler produces (which omit the length byte entirely), and is the specific trigger for the bug. The test loads the PE, reflects on the parameter, and verifies thatGetCustomAttributesdoes not throwTypeLoadExceptionand thatSafeArrayUserDefinedSubTypeis null.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.