From 13b9c3e0c512ebe8a43da36af7d6f639facb5f33 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 1 Aug 2025 16:51:49 +0200 Subject: [PATCH 1/4] add a failing test that was super hard to extract from the repro --- .../tests/EdgeCaseTests.cs | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/libraries/System.Formats.Nrbf/tests/EdgeCaseTests.cs b/src/libraries/System.Formats.Nrbf/tests/EdgeCaseTests.cs index f091d47ded8c5f..1d14993a0890c3 100644 --- a/src/libraries/System.Formats.Nrbf/tests/EdgeCaseTests.cs +++ b/src/libraries/System.Formats.Nrbf/tests/EdgeCaseTests.cs @@ -144,4 +144,48 @@ public void CanReadAllKindsOfDateTimes_DateTimeIsMemberOfTheRootRecord(DateTime Assert.Equal(input.Ticks, classRecord.GetDateTime(nameof(ClassWithDateTime.Value)).Ticks); Assert.Equal(input.Kind, classRecord.GetDateTime(nameof(ClassWithDateTime.Value)).Kind); } + + [Fact] + public void CanReadUserClassStoredAsSystemClass() + { + // For the following data, BinaryFormatter serializes the ClassWithNullableStructField class + // as a record with a single field called "NullableField" with BinaryType.SystemClass (!!!) + // and TypeName being System.Nullable`1[[SampleStruct, $AssemblyName]]. + // It most likely does so, because it's System.Nullable<$NonSystemStruct>. + // But later it serializes the SampleStruct as a ClassWithMembersAndTypes record, + // not SystemClassWithMembersAndTypes. + // It does so, only when the payload contains at least one class with the nullable field being null. + + using MemoryStream stream = Serialize( + new ClassWithNullableStructField[] + { + new ClassWithNullableStructField() { NullableField = null }, // having a null here is crucial for the test + new ClassWithNullableStructField() { NullableField = new ClassWithNullableStructField.SampleStruct() { Value = 42 } } + } + ); + + SZArrayRecord arrayRecord = (SZArrayRecord)NrbfDecoder.Decode(stream); + SerializationRecord[] records = arrayRecord.GetArray(); + Assert.Equal(2, arrayRecord.Length); + Assert.All(records, record => Assert.True(record.TypeNameMatches(typeof(ClassWithNullableStructField)))); + Assert.Null(((ClassRecord)records[0]).GetClassRecord(nameof(ClassWithNullableStructField.NullableField))); + + ClassRecord? notNullRecord = ((ClassRecord)records[1]).GetClassRecord(nameof(ClassWithNullableStructField.NullableField)); + Assert.NotNull(notNullRecord); + Assert.Equal(42, notNullRecord.GetInt32(nameof(ClassWithNullableStructField.SampleStruct.Value))); + } + + [Serializable] + public class ClassWithNullableStructField + { +#pragma warning disable IDE0001 // Simplify names + public System.Nullable NullableField; +#pragma warning restore IDE0001 + + [Serializable] + public struct SampleStruct + { + public int Value; + } + } } From f1a1696918f19e3211371c9515d034ec1dc9647a Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 1 Aug 2025 16:56:05 +0200 Subject: [PATCH 2/4] more permissive fix: allow any SystemClass to be represented as ClassWithMembersAndTypes in the payload --- .../src/System/Formats/Nrbf/MemberTypeInfo.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs index 84c1073b0ef67a..c46d13f443e119 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs @@ -91,8 +91,10 @@ internal static MemberTypeInfo Decode(BinaryReader reader, int count, PayloadOpt const AllowedRecordTypes SystemClass = Classes | AllowedRecordTypes.SystemClassWithMembersAndTypes // All primitive types can be stored by using one of the interfaces they implement. // Example: `new IEnumerable[1] { "hello" }` or `new IComparable[1] { int.MaxValue }`. - | AllowedRecordTypes.BinaryObjectString | AllowedRecordTypes.MemberPrimitiveTyped; - const AllowedRecordTypes NonSystemClass = Classes | AllowedRecordTypes.ClassWithMembersAndTypes; + | AllowedRecordTypes.BinaryObjectString | AllowedRecordTypes.MemberPrimitiveTyped + // System.Nullable is a special case of SystemClassWithMembersAndTypes + | AllowedRecordTypes.ClassWithMembersAndTypes; + const AllowedRecordTypes NonSystemClass = Classes | AllowedRecordTypes.ClassWithMembersAndTypes; return binaryType switch { From 205f91a043421bbb1583490d757af22946da0822 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 1 Aug 2025 17:13:56 +0200 Subject: [PATCH 3/4] a fix that allows for only one particular edge case scenario --- .../src/System/Formats/Nrbf/MemberTypeInfo.cs | 23 ++++++++++++++++--- .../Formats/Nrbf/Utils/TypeNameHelpers.cs | 5 +++- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs index c46d13f443e119..84fa240cee2d66 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs @@ -91,9 +91,7 @@ internal static MemberTypeInfo Decode(BinaryReader reader, int count, PayloadOpt const AllowedRecordTypes SystemClass = Classes | AllowedRecordTypes.SystemClassWithMembersAndTypes // All primitive types can be stored by using one of the interfaces they implement. // Example: `new IEnumerable[1] { "hello" }` or `new IComparable[1] { int.MaxValue }`. - | AllowedRecordTypes.BinaryObjectString | AllowedRecordTypes.MemberPrimitiveTyped - // System.Nullable is a special case of SystemClassWithMembersAndTypes - | AllowedRecordTypes.ClassWithMembersAndTypes; + | AllowedRecordTypes.BinaryObjectString | AllowedRecordTypes.MemberPrimitiveTyped; const AllowedRecordTypes NonSystemClass = Classes | AllowedRecordTypes.ClassWithMembersAndTypes; return binaryType switch @@ -104,10 +102,29 @@ internal static MemberTypeInfo Decode(BinaryReader reader, int count, PayloadOpt BinaryType.StringArray => (StringArray, default), BinaryType.PrimitiveArray => (PrimitiveArray, default), BinaryType.Class => (NonSystemClass, default), + BinaryType.SystemClass when IsNullableUserTypeRepresentedAsSystemType((TypeName)additionalInfo!) => (SystemClass | AllowedRecordTypes.ClassWithMembersAndTypes, default), BinaryType.SystemClass => (SystemClass, default), BinaryType.ObjectArray => (ObjectArray, default), _ => throw new InvalidOperationException() }; + + static bool IsNullableUserTypeRepresentedAsSystemType(TypeName typeName) + { + if (!typeName.IsConstructedGenericType || typeName.Name != typeof(Nullable<>).Name) + { + return false; + } + + var genericArgs = typeName.GetGenericArguments(); + if (genericArgs.Length != 1) + { + return false; + } + + // If the generic argument is not from CoreLib, then it is a user type. + var assemblyName = genericArgs[0].AssemblyName; + return assemblyName is not null && !assemblyName.FullName.Equals(TypeNameHelpers.CoreLibAssemblyName.FullName, StringComparison.Ordinal); + } } internal TypeName GetArrayTypeName(ArrayInfo arrayInfo) diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/TypeNameHelpers.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/TypeNameHelpers.cs index a2fba1b52ecbc9..26327cc013324d 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/TypeNameHelpers.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/TypeNameHelpers.cs @@ -21,6 +21,9 @@ internal static class TypeNameHelpers private static readonly TypeName?[] s_primitiveSZArrayTypeNames = new TypeName?[(int)UIntPtrPrimitiveType + 1]; private static AssemblyNameInfo? s_coreLibAssemblyName; + internal static AssemblyNameInfo CoreLibAssemblyName + => s_coreLibAssemblyName ??= AssemblyNameInfo.Parse("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089".AsSpan()); + internal static TypeName GetPrimitiveTypeName(PrimitiveType primitiveType) { TypeName? typeName = s_primitiveTypeNames[(int)primitiveType]; @@ -155,7 +158,7 @@ internal static TypeName ParseSystemRecordTypeName(this string rawName, PayloadO .WithCoreLibAssemblyName(); // We know it's a System Record, so we set the LibraryName to CoreLib internal static TypeName WithCoreLibAssemblyName(this TypeName systemType) - => systemType.With(s_coreLibAssemblyName ??= AssemblyNameInfo.Parse("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089".AsSpan())); + => systemType.With(CoreLibAssemblyName); private static TypeName With(this TypeName typeName, AssemblyNameInfo assemblyName) { From ddbe9a71461386d10436359ab7fb43ed452b1c84 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 1 Aug 2025 17:35:33 +0200 Subject: [PATCH 4/4] Revert "a fix that allows for only one particular edge case scenario" This reverts commit 3636ebe09b0d72df99bb8fea9196a80538852f4d. --- .../src/System/Formats/Nrbf/MemberTypeInfo.cs | 23 +++---------------- .../Formats/Nrbf/Utils/TypeNameHelpers.cs | 5 +--- 2 files changed, 4 insertions(+), 24 deletions(-) diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs index 84fa240cee2d66..c46d13f443e119 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs @@ -91,7 +91,9 @@ internal static MemberTypeInfo Decode(BinaryReader reader, int count, PayloadOpt const AllowedRecordTypes SystemClass = Classes | AllowedRecordTypes.SystemClassWithMembersAndTypes // All primitive types can be stored by using one of the interfaces they implement. // Example: `new IEnumerable[1] { "hello" }` or `new IComparable[1] { int.MaxValue }`. - | AllowedRecordTypes.BinaryObjectString | AllowedRecordTypes.MemberPrimitiveTyped; + | AllowedRecordTypes.BinaryObjectString | AllowedRecordTypes.MemberPrimitiveTyped + // System.Nullable is a special case of SystemClassWithMembersAndTypes + | AllowedRecordTypes.ClassWithMembersAndTypes; const AllowedRecordTypes NonSystemClass = Classes | AllowedRecordTypes.ClassWithMembersAndTypes; return binaryType switch @@ -102,29 +104,10 @@ internal static MemberTypeInfo Decode(BinaryReader reader, int count, PayloadOpt BinaryType.StringArray => (StringArray, default), BinaryType.PrimitiveArray => (PrimitiveArray, default), BinaryType.Class => (NonSystemClass, default), - BinaryType.SystemClass when IsNullableUserTypeRepresentedAsSystemType((TypeName)additionalInfo!) => (SystemClass | AllowedRecordTypes.ClassWithMembersAndTypes, default), BinaryType.SystemClass => (SystemClass, default), BinaryType.ObjectArray => (ObjectArray, default), _ => throw new InvalidOperationException() }; - - static bool IsNullableUserTypeRepresentedAsSystemType(TypeName typeName) - { - if (!typeName.IsConstructedGenericType || typeName.Name != typeof(Nullable<>).Name) - { - return false; - } - - var genericArgs = typeName.GetGenericArguments(); - if (genericArgs.Length != 1) - { - return false; - } - - // If the generic argument is not from CoreLib, then it is a user type. - var assemblyName = genericArgs[0].AssemblyName; - return assemblyName is not null && !assemblyName.FullName.Equals(TypeNameHelpers.CoreLibAssemblyName.FullName, StringComparison.Ordinal); - } } internal TypeName GetArrayTypeName(ArrayInfo arrayInfo) diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/TypeNameHelpers.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/TypeNameHelpers.cs index 26327cc013324d..a2fba1b52ecbc9 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/TypeNameHelpers.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/TypeNameHelpers.cs @@ -21,9 +21,6 @@ internal static class TypeNameHelpers private static readonly TypeName?[] s_primitiveSZArrayTypeNames = new TypeName?[(int)UIntPtrPrimitiveType + 1]; private static AssemblyNameInfo? s_coreLibAssemblyName; - internal static AssemblyNameInfo CoreLibAssemblyName - => s_coreLibAssemblyName ??= AssemblyNameInfo.Parse("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089".AsSpan()); - internal static TypeName GetPrimitiveTypeName(PrimitiveType primitiveType) { TypeName? typeName = s_primitiveTypeNames[(int)primitiveType]; @@ -158,7 +155,7 @@ internal static TypeName ParseSystemRecordTypeName(this string rawName, PayloadO .WithCoreLibAssemblyName(); // We know it's a System Record, so we set the LibraryName to CoreLib internal static TypeName WithCoreLibAssemblyName(this TypeName systemType) - => systemType.With(CoreLibAssemblyName); + => systemType.With(s_coreLibAssemblyName ??= AssemblyNameInfo.Parse("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089".AsSpan())); private static TypeName With(this TypeName typeName, AssemblyNameInfo assemblyName) {