Fix TypeLoadException in GetMarshalAs when SafeArray has zero-length user-defined type name#124408
Fix TypeLoadException in GetMarshalAs when SafeArray has zero-length user-defined type name#124408
Conversation
|
Tagging subscribers to this area: @dotnet/interop-contrib |
…user-defined type name Return NULL in native code when byte count is 0 for safeArrayUserDefinedSubType, marshalType, and marshalCookie. Add try/catch for safeArrayUserDefinedType resolution in managed code. Use IsNullOrEmpty consistently for marshalTypeName. Add regression test with poison bytes to reliably reproduce the bug. Fixes #124346 Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a regression in MetadataImport.GetMarshalAs where certain MarshalAs blobs (notably NATIVE_TYPE_SAFEARRAY with a zero-length UDT name produced by tlbimp) could result in reading past blob boundaries and triggering TypeLoadException during type resolution.
Changes:
- CoreCLR native: return
NULLfor SafeArray UDT name / custom marshaler type / cookie when the corresponding byte count is 0. - CoreCLR managed: catch
TypeLoadExceptionwhen resolvingSafeArrayUserDefinedSubTypeand alignmarshalTypeNamechecks withstring.IsNullOrEmpty. - Libraries tests: add a regression test that constructs a PE with a crafted FieldMarshal blob to reproduce the bug; fix an existing test method name typo.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/libraries/System.Runtime.InteropServices/tests/System.Runtime.InteropServices.UnitTests/System/Runtime/InteropServices/MarshalAsAttributeTests.cs | Adds a regression test building a PE with a crafted SAFEARRAY marshal blob; fixes a test method name typo. |
| src/coreclr/vm/managedmdimport.cpp | Prevents dangling pointers by returning NULL when marshaler-related string byte counts are 0. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/MdImport.cs | Adds managed defense-in-depth against invalid type name strings and uses string.IsNullOrEmpty consistently. |
| // The user may have supplied a bad type name string causing this TypeLoadException | ||
| // Regardless, we return the bad type name |
There was a problem hiding this comment.
This catch-block comment says "we return the bad type name", but MarshalAsAttribute doesn’t expose the SafeArray user-defined subtype name string anywhere (only the resolved Type via SafeArrayUserDefinedSubType). Please adjust the comment to reflect the actual behavior (e.g., that we swallow TypeLoadException and leave SafeArrayUserDefinedSubType unset) to avoid misleading future maintainers.
| // The user may have supplied a bad type name string causing this TypeLoadException | |
| // Regardless, we return the bad type name | |
| // The user may have supplied a bad type name string causing this TypeLoadException. | |
| // Swallow the exception and leave SafeArrayUserDefinedSubType unset; the original | |
| // type name remains in safeArrayUserDefinedTypeName for potential diagnostics. |
There was a problem hiding this comment.
Updated the catch block comment in commit 81224a0 to accurately describe the behavior — we swallow the exception and leave SafeArrayUserDefinedSubType unset.
| [Fact] | ||
| public void SafeArrayParameter_ZeroLengthUserDefinedSubType_DoesNotThrow() | ||
| { |
There was a problem hiding this comment.
This test relies on COM interop behavior (FEATURE_COMINTEROP) and Reflection.Emit (PersistedAssemblyBuilder). As a plain [Fact], it may run on non-Windows / browser / NativeAOT configurations where it either can’t exercise the regression or may throw PlatformNotSupportedException. Consider switching this to a ConditionalFact guarded by PlatformDetection.IsBuiltInComEnabled and PlatformDetection.IsReflectionEmitSupported (and any other relevant guards used in this test project).
There was a problem hiding this comment.
Switched to [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsBuiltInComEnabled), nameof(PlatformDetection.IsReflectionEmitSupported))] in commit 81224a0, matching the existing convention used by tests in the same project (e.g., GetEndComSlotTests).
| Type iface = asm.GetType("TestInterface"); | ||
| MethodInfo method = iface.GetMethod("TestMethod"); | ||
| ParameterInfo param = method.GetParameters()[0]; | ||
|
|
||
| // Must not throw TypeLoadException. | ||
| _ = param.GetCustomAttributes(false); | ||
|
|
||
| MarshalAsAttribute attr = (MarshalAsAttribute)Attribute.GetCustomAttribute(param, typeof(MarshalAsAttribute)); |
There was a problem hiding this comment.
These reflection calls return nullable types (e.g., Assembly.GetType / Type.GetMethod / Attribute.GetCustomAttribute). As written, this likely introduces nullable warnings (and potential NullReferenceExceptions if metadata changes). Please add explicit assertions / null-forgiveness where appropriate (or use the overloads that throw on failure) so the test compiles cleanly under nullable and fails with a clear message if the expected members aren’t found.
| Type iface = asm.GetType("TestInterface"); | |
| MethodInfo method = iface.GetMethod("TestMethod"); | |
| ParameterInfo param = method.GetParameters()[0]; | |
| // Must not throw TypeLoadException. | |
| _ = param.GetCustomAttributes(false); | |
| MarshalAsAttribute attr = (MarshalAsAttribute)Attribute.GetCustomAttribute(param, typeof(MarshalAsAttribute)); | |
| Type iface = asm.GetType("TestInterface") ?? throw new InvalidOperationException("Test interface 'TestInterface' not found in generated assembly."); | |
| MethodInfo method = iface.GetMethod("TestMethod") ?? throw new InvalidOperationException("Test method 'TestMethod' not found on 'TestInterface'."); | |
| ParameterInfo param = method.GetParameters()[0]; | |
| // Must not throw TypeLoadException. | |
| _ = param.GetCustomAttributes(false); | |
| MarshalAsAttribute attr = (MarshalAsAttribute?)Attribute.GetCustomAttribute(param, typeof(MarshalAsAttribute)) | |
| ?? throw new InvalidOperationException("Expected MarshalAsAttribute not found on parameter."); |
There was a problem hiding this comment.
Fixed nullable handling in commit 81224a0 — added ! on GetType and GetMethod returns, and changed attr to MarshalAsAttribute? with the ? cast.
| md.DefineParameter(1, ParameterAttributes.HasFieldMarshal, "args"); | ||
| td.CreateType(); | ||
|
|
||
| MetadataBuilder mdb = ab.GenerateMetadata(out BlobBuilder ilBlob, out _); | ||
|
|
||
| // Blob bytes: | ||
| // 0x1D NATIVE_TYPE_SAFEARRAY | ||
| // 0x09 VT_DISPATCH | ||
| // 0x00 compressed string length 0 | ||
| // 0x58 'X' poison (not consumed by parser) | ||
| // 0x00 null terminator | ||
| BlobBuilder marshalBlob = new(); | ||
| marshalBlob.WriteByte(0x1D); | ||
| marshalBlob.WriteByte(0x09); | ||
| marshalBlob.WriteByte(0x00); | ||
| marshalBlob.WriteByte(0x58); | ||
| marshalBlob.WriteByte(0x00); | ||
| mdb.AddMarshallingDescriptor( | ||
| MetadataTokens.ParameterHandle(1), | ||
| mdb.GetOrAddBlob(marshalBlob)); |
There was a problem hiding this comment.
Avoid hardcoding ParameterHandle(1) here; the row id depends on how the Params table is emitted and could change (e.g., if a return parameter row gets introduced). Capture the ParameterBuilder returned from DefineParameter and derive the correct ParameterHandle from its metadata token when calling AddMarshallingDescriptor, so the test is resilient to metadata layout changes.
There was a problem hiding this comment.
Addressed in commit 81224a0 by extracting the hardcoded 1 into const int paramRowNumber = 1; with a comment explaining why it's correct. The portable emit layer's ParameterBuilder doesn't expose a metadata token, so we can't derive it programmatically, but the assumption is valid and now clearly documented.
| Assembly asm = alc.LoadFromStream(new MemoryStream(peBytes)); | ||
| Type iface = asm.GetType("TestInterface"); | ||
| MethodInfo method = iface.GetMethod("TestMethod"); | ||
| ParameterInfo param = method.GetParameters()[0]; | ||
|
|
||
| // Must not throw TypeLoadException. | ||
| _ = param.GetCustomAttributes(false); | ||
|
|
||
| MarshalAsAttribute attr = (MarshalAsAttribute)Attribute.GetCustomAttribute(param, typeof(MarshalAsAttribute)); | ||
| Assert.NotNull(attr); | ||
| Assert.Equal(UnmanagedType.SafeArray, attr.Value); | ||
| Assert.Null(attr.SafeArrayUserDefinedSubType); |
There was a problem hiding this comment.
The MemoryStream passed to AssemblyLoadContext.LoadFromStream isn’t disposed. Please wrap it in a using/try-finally so the stream is closed promptly (this can also help collectible ALC unloading behavior).
| Assembly asm = alc.LoadFromStream(new MemoryStream(peBytes)); | |
| Type iface = asm.GetType("TestInterface"); | |
| MethodInfo method = iface.GetMethod("TestMethod"); | |
| ParameterInfo param = method.GetParameters()[0]; | |
| // Must not throw TypeLoadException. | |
| _ = param.GetCustomAttributes(false); | |
| MarshalAsAttribute attr = (MarshalAsAttribute)Attribute.GetCustomAttribute(param, typeof(MarshalAsAttribute)); | |
| Assert.NotNull(attr); | |
| Assert.Equal(UnmanagedType.SafeArray, attr.Value); | |
| Assert.Null(attr.SafeArrayUserDefinedSubType); | |
| using (MemoryStream peStream = new(peBytes)) | |
| { | |
| Assembly asm = alc.LoadFromStream(peStream); | |
| Type iface = asm.GetType("TestInterface"); | |
| MethodInfo method = iface.GetMethod("TestMethod"); | |
| ParameterInfo param = method.GetParameters()[0]; | |
| // Must not throw TypeLoadException. | |
| _ = param.GetCustomAttributes(false); | |
| MarshalAsAttribute attr = (MarshalAsAttribute)Attribute.GetCustomAttribute(param, typeof(MarshalAsAttribute)); | |
| Assert.NotNull(attr); | |
| Assert.Equal(UnmanagedType.SafeArray, attr.Value); | |
| Assert.Null(attr.SafeArrayUserDefinedSubType); | |
| } |
There was a problem hiding this comment.
Wrapped in using MemoryStream peStream = new(peBytes); in commit 81224a0.
|
@copilot please address all relevant feedback. Also invoke the code-review skill and post the analysis/results as a comment on this PR |
…ispose stream, and comment accuracy Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Head branch was pushed to by a user without write access
All review feedback has been addressed in commit 81224a0. Here's the code-review skill analysis: 🤖 Code Review — PR #124408 Motivation: Justified regression fix. The native FCALL returned dangling pointers for zero-length strings after commit a3dc133 switched from managed strings to raw byte pointers. This caused Approach: Correct two-layer fix — native root cause fix (return NULL when byte count is 0) plus managed defense-in-depth (catch Summary: ✅ No blocking issues found. The native fix correctly guards all three string outputs ( |
| { | ||
| // The user may have supplied a bad type name string causing this TypeLoadException. | ||
| // Swallow the exception and leave SafeArrayUserDefinedSubType unset; the original | ||
| // type name remains in safeArrayUserDefinedTypeName for potential diagnostics. |
There was a problem hiding this comment.
safeArrayUserDefinedTypeName is a local variable. It is going to be gone once this method returns. What is the "potential diagnostics" that this comment talks about?
There was a problem hiding this comment.
Good catch — removed the misleading "potential diagnostics" reference. The comment now matches the existing marshalTypeRef catch block style. Fixed in 3c8d570.
…back Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
managedmdimport.cpp): Return NULL when byte count is 0 forsafeArrayUserDefinedSubType,marshalType, andmarshalCookieMdImport.cs): Add try/catch for safeArrayUserDefinedType resolution, update marshalTypeName to useIsNullOrEmptyfor consistency (jkotas feedback from Fix TypeLoadException in GetMarshalAs when SafeArray has zero-length user-defined type name #124355)MarshalAsAttributeTests.cs): Use poison bytes{0x1D, 0x09, 0x00, 0x58, 0x00}per stephentoub's feedback from Fix TypeLoadException in GetMarshalAs when SafeArray has zero-length user-defined type name #124355Ctor_UmanagedTye→Ctor_UnmanagedTypetypoIsBuiltInComEnabledandIsReflectionEmitSupported(comment 2806608113)!and?annotations (comment 2806608127)usingdeclaration (comment 2806608147)Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.