From d5addcb8aa7c54ce3c2d106d671716e843208ef4 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 19 Aug 2024 14:22:25 +0200 Subject: [PATCH 01/11] introduce ArrayRecord.TotalElementsCount --- .../ref/System.Formats.Nrbf.cs | 1 + .../src/System/Formats/Nrbf/ArrayRecord.cs | 6 ++ .../System/Formats/Nrbf/BinaryArrayRecord.cs | 58 +++++++++++++++++++ .../tests/ArraySinglePrimitiveRecordTests.cs | 1 + .../tests/JaggedArraysTests.cs | 20 +++++++ .../tests/RectangularArraysTests.cs | 3 + 6 files changed, 89 insertions(+) diff --git a/src/libraries/System.Formats.Nrbf/ref/System.Formats.Nrbf.cs b/src/libraries/System.Formats.Nrbf/ref/System.Formats.Nrbf.cs index 8e12cf7c3712f..cb91110f6c8c9 100644 --- a/src/libraries/System.Formats.Nrbf/ref/System.Formats.Nrbf.cs +++ b/src/libraries/System.Formats.Nrbf/ref/System.Formats.Nrbf.cs @@ -11,6 +11,7 @@ public abstract partial class ArrayRecord : System.Formats.Nrbf.SerializationRec internal ArrayRecord() { } public override System.Formats.Nrbf.SerializationRecordId Id { get { throw null; } } public abstract System.ReadOnlySpan Lengths { get; } + public virtual long TotalElementsCount { get; } public int Rank { get { throw null; } } [System.Diagnostics.CodeAnalysis.RequiresDynamicCode("The code for an array of the specified type might not be available.")] public System.Array GetArray(System.Type expectedArrayType, bool allowNulls = true) { throw null; } diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs index ddfd91a29fb1a..4072df7e2d28e 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs @@ -27,6 +27,12 @@ private protected ArrayRecord(ArrayInfo arrayInfo) /// A buffer of integers that represent the number of elements in every dimension. public abstract ReadOnlySpan Lengths { get; } + /// + /// When overridden in a derived class, gets the total number of all elements in every dimension. + /// + /// A number that represent the total number of all elements in every dimension. + public virtual long TotalElementsCount => ArrayInfo.TotalElementsCount; + /// /// Gets the rank of the array. /// diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs index 0c7e04e840a48..bfc6471a6a060 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs @@ -6,6 +6,7 @@ using System.IO; using System.Reflection.Metadata; using System.Formats.Nrbf.Utils; +using System.Diagnostics; namespace System.Formats.Nrbf; @@ -27,12 +28,16 @@ internal sealed class BinaryArrayRecord : ArrayRecord ]; private TypeName? _typeName; + private long _totalElementsCount; private BinaryArrayRecord(ArrayInfo arrayInfo, MemberTypeInfo memberTypeInfo) : base(arrayInfo) { MemberTypeInfo = memberTypeInfo; Values = []; + + // We need to parse all elements of the jagged array to obtain total elements count; + _totalElementsCount = arrayInfo.ArrayType != BinaryArrayType.Jagged ? arrayInfo.TotalElementsCount : -1; } public override SerializationRecordType RecordType => SerializationRecordType.BinaryArray; @@ -40,6 +45,59 @@ private BinaryArrayRecord(ArrayInfo arrayInfo, MemberTypeInfo memberTypeInfo) /// public override ReadOnlySpan Lengths => new int[1] { Length }; + /// + public override long TotalElementsCount + { + get + { + if (_totalElementsCount >= 0) + { + return _totalElementsCount; + } + + Debug.Assert(ArrayInfo.ArrayType == BinaryArrayType.Jagged); + + long result = 0; + foreach (object value in Values) + { + object item = value is MemberReferenceRecord referenceRecord + ? referenceRecord.GetReferencedRecord() + : value; + + if (item is not SerializationRecord record) + { + result++; + continue; + } + + switch (record.RecordType) + { + case SerializationRecordType.BinaryArray: + case SerializationRecordType.ArraySinglePrimitive: + case SerializationRecordType.ArraySingleObject: + case SerializationRecordType.ArraySingleString: + ArrayRecord nestedArrayRecord = (ArrayRecord)record; + // This may be a recursive call! + result += nestedArrayRecord.TotalElementsCount; + break; + case SerializationRecordType.ObjectNull: + case SerializationRecordType.ObjectNullMultiple256: + case SerializationRecordType.ObjectNullMultiple: + // Null Records nested inside jagged array do not increase total elements count. + // Example: "int[][] input = [[1, 2, 3], null]" is just 3 elements in total. + break; + default: + result++; + break; + } + } + + _totalElementsCount = result; + + return result; + } + } + public override TypeName TypeName => _typeName ??= MemberTypeInfo.GetArrayTypeName(ArrayInfo); diff --git a/src/libraries/System.Formats.Nrbf/tests/ArraySinglePrimitiveRecordTests.cs b/src/libraries/System.Formats.Nrbf/tests/ArraySinglePrimitiveRecordTests.cs index d4a4b5b3c690a..56091bf0182d2 100644 --- a/src/libraries/System.Formats.Nrbf/tests/ArraySinglePrimitiveRecordTests.cs +++ b/src/libraries/System.Formats.Nrbf/tests/ArraySinglePrimitiveRecordTests.cs @@ -94,6 +94,7 @@ private void Test(int size, bool canSeek) SZArrayRecord arrayRecord = (SZArrayRecord)NrbfDecoder.Decode(stream); Assert.Equal(size, arrayRecord.Length); + Assert.Equal(size, arrayRecord.TotalElementsCount); T?[] output = arrayRecord.GetArray(); Assert.Equal(input, output); Assert.Same(output, arrayRecord.GetArray()); diff --git a/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs b/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs index a72c3227c1eec..1973e0c6c2116 100644 --- a/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs +++ b/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs @@ -19,6 +19,19 @@ public void CanReadJaggedArraysOfPrimitiveTypes_2D() Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); + Assert.Equal(input.Length * 3, arrayRecord.TotalElementsCount); + } + + [Fact] + public void TotalElementsCountDoesNotIncludeNullArrays() + { + int[][] input = [[1, 2, 3], null]; + + var arrayRecord = (ArrayRecord)NrbfDecoder.Decode(Serialize(input)); + + Verify(input, arrayRecord); + Assert.Equal(input, arrayRecord.GetArray(input.GetType())); + Assert.Equal(3, arrayRecord.TotalElementsCount); } [Fact] @@ -36,6 +49,7 @@ public void CanReadJaggedArraysOfPrimitiveTypes_3D() Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); Assert.Equal(1, arrayRecord.Rank); + Assert.Equal(input.Length * 1 * 3, arrayRecord.TotalElementsCount); } [Fact] @@ -60,6 +74,7 @@ public void CanReadJaggedArrayOfRectangularArrays() Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); Assert.Equal(1, arrayRecord.Rank); + Assert.Equal(input.Length * 3 * 3, arrayRecord.TotalElementsCount); } [Fact] @@ -75,6 +90,7 @@ public void CanReadJaggedArraysOfStrings() Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); + Assert.Equal(input.Length * 3, arrayRecord.TotalElementsCount); } [Fact] @@ -90,6 +106,7 @@ public void CanReadJaggedArraysOfObjects() Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); + Assert.Equal(input.Length * 3, arrayRecord.TotalElementsCount); } [Serializable] @@ -102,14 +119,17 @@ public class ComplexType public void CanReadJaggedArraysOfComplexTypes() { ComplexType[][] input = new ComplexType[3][]; + long totalElementsCount = 0; for (int i = 0; i < input.Length; i++) { input[i] = Enumerable.Range(0, i + 1).Select(j => new ComplexType { SomeField = j }).ToArray(); + totalElementsCount += input[i].Length; } var arrayRecord = (ArrayRecord)NrbfDecoder.Decode(Serialize(input)); Verify(input, arrayRecord); + Assert.Equal(totalElementsCount, arrayRecord.TotalElementsCount); var output = (ClassRecord?[][])arrayRecord.GetArray(input.GetType()); for (int i = 0; i < input.Length; i++) { diff --git a/src/libraries/System.Formats.Nrbf/tests/RectangularArraysTests.cs b/src/libraries/System.Formats.Nrbf/tests/RectangularArraysTests.cs index 25e7bb5a4d533..f3e7671c6b20c 100644 --- a/src/libraries/System.Formats.Nrbf/tests/RectangularArraysTests.cs +++ b/src/libraries/System.Formats.Nrbf/tests/RectangularArraysTests.cs @@ -223,10 +223,13 @@ public void CanReadRectangularArraysOfComplexTypes_3D() internal static void Verify(Array input, ArrayRecord arrayRecord) { Assert.Equal(input.Rank, arrayRecord.Lengths.Length); + long totalElementsCount = 1; for (int i = 0; i < input.Rank; i++) { Assert.Equal(input.GetLength(i), arrayRecord.Lengths[i]); + totalElementsCount *= input.GetLength(i); } + Assert.Equal(totalElementsCount, arrayRecord.TotalElementsCount); Assert.Equal(input.GetType().FullName, arrayRecord.TypeName.FullName); Assert.Equal(input.GetType().GetAssemblyNameIncludingTypeForwards(), arrayRecord.TypeName.AssemblyName!.FullName); } From 38fb5f8f115dc3b658d58d484c1e80f42a308fa5 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 19 Aug 2024 15:06:07 +0200 Subject: [PATCH 02/11] remove recursion --- .../src/System/Formats/Nrbf/ArrayRecord.cs | 2 +- .../System/Formats/Nrbf/BinaryArrayRecord.cs | 40 ++++++++++++++----- 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs index 4072df7e2d28e..24cfcc216fcbd 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs @@ -50,7 +50,7 @@ private protected ArrayRecord(ArrayInfo arrayInfo) internal long ValuesToRead { get; private protected set; } - private protected ArrayInfo ArrayInfo { get; } + internal ArrayInfo ArrayInfo { get; } /// /// Allocates an array and fills it with the data provided in the serialized records (in case of primitive types like or ) or the serialized records themselves. diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs index bfc6471a6a060..c5d1fc1c38823 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs @@ -50,15 +50,29 @@ public override long TotalElementsCount { get { - if (_totalElementsCount >= 0) + if (_totalElementsCount < 0) { - return _totalElementsCount; + Debug.Assert(ArrayInfo.ArrayType == BinaryArrayType.Jagged); + + Stack jaggedArrayRecords = new(); + jaggedArrayRecords.Push(this); + + _totalElementsCount = GetTotalElementsCount(jaggedArrayRecords); } - Debug.Assert(ArrayInfo.ArrayType == BinaryArrayType.Jagged); + return _totalElementsCount; + } + } + + private static long GetTotalElementsCount(Stack jaggedArrayRecords) + { + long result = 0; + while (jaggedArrayRecords.Count != 0) + { + BinaryArrayRecord current = jaggedArrayRecords.Pop(); + Debug.Assert(current.ArrayInfo.ArrayType == BinaryArrayType.Jagged); - long result = 0; - foreach (object value in Values) + foreach (object value in current.Values) { object item = value is MemberReferenceRecord referenceRecord ? referenceRecord.GetReferencedRecord() @@ -77,8 +91,14 @@ public override long TotalElementsCount case SerializationRecordType.ArraySingleObject: case SerializationRecordType.ArraySingleString: ArrayRecord nestedArrayRecord = (ArrayRecord)record; - // This may be a recursive call! - result += nestedArrayRecord.TotalElementsCount; + if (nestedArrayRecord.ArrayInfo.ArrayType == BinaryArrayType.Jagged) + { + jaggedArrayRecords.Push((BinaryArrayRecord)nestedArrayRecord); + } + else + { + result += nestedArrayRecord.TotalElementsCount; + } break; case SerializationRecordType.ObjectNull: case SerializationRecordType.ObjectNullMultiple256: @@ -91,11 +111,9 @@ public override long TotalElementsCount break; } } - - _totalElementsCount = result; - - return result; } + + return result; } public override TypeName TypeName From f89177c8b9edd88c72d73ef86dbfc49486872284 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 19 Aug 2024 15:14:25 +0200 Subject: [PATCH 03/11] don't allocate the Stack unless it's necessary --- .../System/Formats/Nrbf/BinaryArrayRecord.cs | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs index c5d1fc1c38823..503d24ae96d98 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs @@ -52,27 +52,28 @@ public override long TotalElementsCount { if (_totalElementsCount < 0) { - Debug.Assert(ArrayInfo.ArrayType == BinaryArrayType.Jagged); - - Stack jaggedArrayRecords = new(); - jaggedArrayRecords.Push(this); - - _totalElementsCount = GetTotalElementsCount(jaggedArrayRecords); + _totalElementsCount = GetJaggedArrayTotalElementsCount(this); } return _totalElementsCount; } } - private static long GetTotalElementsCount(Stack jaggedArrayRecords) + private static long GetJaggedArrayTotalElementsCount(BinaryArrayRecord jaggedArrayRecord) { long result = 0; - while (jaggedArrayRecords.Count != 0) + Stack? jaggedArrayRecords = null; + + do { - BinaryArrayRecord current = jaggedArrayRecords.Pop(); - Debug.Assert(current.ArrayInfo.ArrayType == BinaryArrayType.Jagged); + if (jaggedArrayRecords is not null) + { + jaggedArrayRecord = jaggedArrayRecords.Pop(); + } + + Debug.Assert(jaggedArrayRecord.ArrayInfo.ArrayType == BinaryArrayType.Jagged); - foreach (object value in current.Values) + foreach (object value in jaggedArrayRecord.Values) { object item = value is MemberReferenceRecord referenceRecord ? referenceRecord.GetReferencedRecord() @@ -93,7 +94,7 @@ private static long GetTotalElementsCount(Stack jaggedArrayRe ArrayRecord nestedArrayRecord = (ArrayRecord)record; if (nestedArrayRecord.ArrayInfo.ArrayType == BinaryArrayType.Jagged) { - jaggedArrayRecords.Push((BinaryArrayRecord)nestedArrayRecord); + (jaggedArrayRecords ??= new()).Push((BinaryArrayRecord)nestedArrayRecord); } else { @@ -112,6 +113,7 @@ private static long GetTotalElementsCount(Stack jaggedArrayRe } } } + while (jaggedArrayRecords is not null && jaggedArrayRecords.Count > 0); return result; } From 77088c2db0b5a32ff3385f4b541634042a8fc096 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 19 Aug 2024 15:16:19 +0200 Subject: [PATCH 04/11] use Queue (both are fine as the order does not really matter here) --- .../src/System/Formats/Nrbf/BinaryArrayRecord.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs index 503d24ae96d98..96494c2e0393b 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs @@ -62,13 +62,13 @@ public override long TotalElementsCount private static long GetJaggedArrayTotalElementsCount(BinaryArrayRecord jaggedArrayRecord) { long result = 0; - Stack? jaggedArrayRecords = null; + Queue? jaggedArrayRecords = null; do { if (jaggedArrayRecords is not null) { - jaggedArrayRecord = jaggedArrayRecords.Pop(); + jaggedArrayRecord = jaggedArrayRecords.Dequeue(); } Debug.Assert(jaggedArrayRecord.ArrayInfo.ArrayType == BinaryArrayType.Jagged); @@ -94,7 +94,7 @@ private static long GetJaggedArrayTotalElementsCount(BinaryArrayRecord jaggedArr ArrayRecord nestedArrayRecord = (ArrayRecord)record; if (nestedArrayRecord.ArrayInfo.ArrayType == BinaryArrayType.Jagged) { - (jaggedArrayRecords ??= new()).Push((BinaryArrayRecord)nestedArrayRecord); + (jaggedArrayRecords ??= new()).Enqueue((BinaryArrayRecord)nestedArrayRecord); } else { From 13306fc1bef73498db6310ff1fd7d56e3e4001e8 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 19 Aug 2024 15:26:28 +0200 Subject: [PATCH 05/11] do not include invalid Type or Assembly names in the exception messages, as it's most likely corrupted/tampered/malicious data and could be used as a vector of attack. --- .../System.Formats.Nrbf/src/Resources/Strings.resx | 6 +++--- .../src/System/Formats/Nrbf/BinaryLibraryRecord.cs | 2 +- .../src/System/Formats/Nrbf/Utils/ThrowHelper.cs | 10 ++++++---- .../src/System/Formats/Nrbf/Utils/TypeNameHelpers.cs | 4 ++-- 4 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/libraries/System.Formats.Nrbf/src/Resources/Strings.resx b/src/libraries/System.Formats.Nrbf/src/Resources/Strings.resx index 349405150c65e..14eb97bd1db12 100644 --- a/src/libraries/System.Formats.Nrbf/src/Resources/Strings.resx +++ b/src/libraries/System.Formats.Nrbf/src/Resources/Strings.resx @@ -136,13 +136,13 @@ Member reference was pointing to a record of unexpected type. - Invalid type name: `{0}`. + Invalid type name. Expected the array to be of type {0}, but its element type was {1}. - Invalid type or assembly name: `{0},{1}`. + Invalid type or assembly name. Duplicate member name: `{0}`. @@ -160,6 +160,6 @@ Only arrays with zero offsets are supported. - Invalid assembly name: `{0}`. + Invalid assembly name. \ No newline at end of file diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryLibraryRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryLibraryRecord.cs index ccd39922e23fb..51daf174c51a1 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryLibraryRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryLibraryRecord.cs @@ -57,7 +57,7 @@ internal static BinaryLibraryRecord Decode(BinaryReader reader, PayloadOptions o } else if (!options.UndoTruncatedTypeNames) { - ThrowHelper.ThrowInvalidAssemblyName(rawName); + ThrowHelper.ThrowInvalidAssemblyName(); } return new BinaryLibraryRecord(id, rawName); diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/ThrowHelper.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/ThrowHelper.cs index f096bfc736098..275b70aca5742 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/ThrowHelper.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/ThrowHelper.cs @@ -6,6 +6,8 @@ namespace System.Formats.Nrbf.Utils; +// The exception messages do not contain type or assembly names on purpose, +// as it's most likely corrupted/tampered/malicious data. internal static class ThrowHelper { internal static void ThrowInvalidValue(object value) @@ -14,8 +16,8 @@ internal static void ThrowInvalidValue(object value) internal static void ThrowInvalidReference() => throw new SerializationException(SR.Serialization_InvalidReference); - internal static void ThrowInvalidTypeName(string name) - => throw new SerializationException(SR.Format(SR.Serialization_InvalidTypeName, name)); + internal static void ThrowInvalidTypeName() + => throw new SerializationException(SR.Serialization_InvalidTypeName); internal static void ThrowUnexpectedNullRecordCount() => throw new SerializationException(SR.Serialization_UnexpectedNullRecordCount); @@ -26,8 +28,8 @@ internal static void ThrowMaxArrayLength(long limit, long actual) internal static void ThrowArrayContainedNulls() => throw new SerializationException(SR.Serialization_ArrayContainedNulls); - internal static void ThrowInvalidAssemblyName(string rawName) - => throw new SerializationException(SR.Format(SR.Serialization_InvalidAssemblyName, rawName)); + internal static void ThrowInvalidAssemblyName() + => throw new SerializationException(SR.Serialization_InvalidAssemblyName); internal static void ThrowEndOfStreamException() => throw new EndOfStreamException(); 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 97c3b4e42f68b..8a378cc384a49 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 @@ -125,7 +125,7 @@ internal static TypeName ParseNonSystemClassRecordTypeName(this string rawName, if (typeName is null) { - throw new SerializationException(SR.Format(SR.Serialization_InvalidTypeOrAssemblyName, rawName, libraryRecord.RawLibraryName)); + throw new SerializationException(SR.Serialization_InvalidTypeOrAssemblyName); } if (typeName.AssemblyName is null) @@ -169,7 +169,7 @@ private static TypeName With(this TypeName typeName, AssemblyNameInfo assemblyNa else { // BinaryFormatter can not serialize pointers or references. - ThrowHelper.ThrowInvalidTypeName(typeName.FullName); + ThrowHelper.ThrowInvalidTypeName(); } } From 297aa4bbea5cbf4e47ea792b200b0bf6a56d13f5 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 19 Aug 2024 17:32:09 +0200 Subject: [PATCH 06/11] It is possible to have binary array records have an element type of array without being marked as jagged --- .../src/System/Formats/Nrbf/ArrayRecord.cs | 7 +- .../System/Formats/Nrbf/BinaryArrayRecord.cs | 127 +++++++++--------- .../tests/JaggedArraysTests.cs | 33 +++++ 3 files changed, 103 insertions(+), 64 deletions(-) diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs index 24cfcc216fcbd..0f760c1da0373 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs @@ -50,7 +50,12 @@ private protected ArrayRecord(ArrayInfo arrayInfo) internal long ValuesToRead { get; private protected set; } - internal ArrayInfo ArrayInfo { get; } + private protected ArrayInfo ArrayInfo { get; } + + internal bool IsJagged + => ArrayInfo.ArrayType == BinaryArrayType.Jagged + // It is possible to have binary array records have an element type of array without being marked as jagged. + || TypeName.GetElementType().IsArray; /// /// Allocates an array and fills it with the data provided in the serialized records (in case of primitive types like or ) or the serialized records themselves. diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs index 96494c2e0393b..d5c31c30be53d 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs @@ -35,9 +35,8 @@ private BinaryArrayRecord(ArrayInfo arrayInfo, MemberTypeInfo memberTypeInfo) { MemberTypeInfo = memberTypeInfo; Values = []; - - // We need to parse all elements of the jagged array to obtain total elements count; - _totalElementsCount = arrayInfo.ArrayType != BinaryArrayType.Jagged ? arrayInfo.TotalElementsCount : -1; + // We need to parse all elements of the jagged array to obtain total elements count. + _totalElementsCount = -1; } public override SerializationRecordType RecordType => SerializationRecordType.BinaryArray; @@ -52,72 +51,15 @@ public override long TotalElementsCount { if (_totalElementsCount < 0) { - _totalElementsCount = GetJaggedArrayTotalElementsCount(this); + _totalElementsCount = IsJagged + ? GetJaggedArrayTotalElementsCount(this) + : ArrayInfo.TotalElementsCount; } return _totalElementsCount; } } - private static long GetJaggedArrayTotalElementsCount(BinaryArrayRecord jaggedArrayRecord) - { - long result = 0; - Queue? jaggedArrayRecords = null; - - do - { - if (jaggedArrayRecords is not null) - { - jaggedArrayRecord = jaggedArrayRecords.Dequeue(); - } - - Debug.Assert(jaggedArrayRecord.ArrayInfo.ArrayType == BinaryArrayType.Jagged); - - foreach (object value in jaggedArrayRecord.Values) - { - object item = value is MemberReferenceRecord referenceRecord - ? referenceRecord.GetReferencedRecord() - : value; - - if (item is not SerializationRecord record) - { - result++; - continue; - } - - switch (record.RecordType) - { - case SerializationRecordType.BinaryArray: - case SerializationRecordType.ArraySinglePrimitive: - case SerializationRecordType.ArraySingleObject: - case SerializationRecordType.ArraySingleString: - ArrayRecord nestedArrayRecord = (ArrayRecord)record; - if (nestedArrayRecord.ArrayInfo.ArrayType == BinaryArrayType.Jagged) - { - (jaggedArrayRecords ??= new()).Enqueue((BinaryArrayRecord)nestedArrayRecord); - } - else - { - result += nestedArrayRecord.TotalElementsCount; - } - break; - case SerializationRecordType.ObjectNull: - case SerializationRecordType.ObjectNullMultiple256: - case SerializationRecordType.ObjectNullMultiple: - // Null Records nested inside jagged array do not increase total elements count. - // Example: "int[][] input = [[1, 2, 3], null]" is just 3 elements in total. - break; - default: - result++; - break; - } - } - } - while (jaggedArrayRecords is not null && jaggedArrayRecords.Count > 0); - - return result; - } - public override TypeName TypeName => _typeName ??= MemberTypeInfo.GetArrayTypeName(ArrayInfo); @@ -235,6 +177,65 @@ internal static ArrayRecord Decode(BinaryReader reader, RecordMap recordMap, Pay : new BinaryArrayRecord(arrayInfo, memberTypeInfo); } + private static long GetJaggedArrayTotalElementsCount(BinaryArrayRecord jaggedArrayRecord) + { + long result = 0; + Queue? jaggedArrayRecords = null; + + do + { + if (jaggedArrayRecords is not null) + { + jaggedArrayRecord = jaggedArrayRecords.Dequeue(); + } + + Debug.Assert(jaggedArrayRecord.IsJagged); + + foreach (object value in jaggedArrayRecord.Values) + { + object item = value is MemberReferenceRecord referenceRecord + ? referenceRecord.GetReferencedRecord() + : value; + + if (item is not SerializationRecord record) + { + result++; + continue; + } + + switch (record.RecordType) + { + case SerializationRecordType.BinaryArray: + case SerializationRecordType.ArraySinglePrimitive: + case SerializationRecordType.ArraySingleObject: + case SerializationRecordType.ArraySingleString: + ArrayRecord nestedArrayRecord = (ArrayRecord)record; + if (nestedArrayRecord.IsJagged) + { + (jaggedArrayRecords ??= new()).Enqueue((BinaryArrayRecord)nestedArrayRecord); + } + else + { + result += nestedArrayRecord.TotalElementsCount; + } + break; + case SerializationRecordType.ObjectNull: + case SerializationRecordType.ObjectNullMultiple256: + case SerializationRecordType.ObjectNullMultiple: + // Null Records nested inside jagged array do not increase total elements count. + // Example: "int[][] input = [[1, 2, 3], null]" is just 3 elements in total. + break; + default: + result++; + break; + } + } + } + while (jaggedArrayRecords is not null && jaggedArrayRecords.Count > 0); + + return result; + } + private protected override void AddValue(object value) => Values.Add(value); internal override (AllowedRecordTypes allowed, PrimitiveType primitiveType) GetAllowedRecordType() diff --git a/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs b/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs index 1973e0c6c2116..e4567c87cd781 100644 --- a/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs +++ b/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs @@ -1,4 +1,5 @@ using System.Formats.Nrbf.Utils; +using System.IO; using System.Linq; using Xunit; @@ -34,6 +35,38 @@ public void TotalElementsCountDoesNotIncludeNullArrays() Assert.Equal(3, arrayRecord.TotalElementsCount); } + [Fact] + public void ItIsPossibleToHaveBinaryArrayRecordsHaveAnElementTypeOfArrayWithoutBeingMarkedAsJagged() + { + int[][][] input = new int[3][][]; + for (int i = 0; i < input.Length; i++) + { + input[i] = new int[4][]; + + for (int j = 0; j < input[i].Length; j++) + { + input[i][j] = [i, j, 0, 1, 2]; + } + } + + byte[] serialized = Serialize(input).ToArray(); + const int ArrayTypeByteIndex = + sizeof(byte) + sizeof(int) * 4 + // stream header + sizeof(byte) + // SerializationRecordType.BinaryArray + sizeof(int); // SerializationRecordId + + Assert.Equal((byte)BinaryArrayType.Jagged, serialized[ArrayTypeByteIndex]); + + // change the reported array type + serialized[ArrayTypeByteIndex] = (byte)BinaryArrayType.Single; + + var arrayRecord = (ArrayRecord)NrbfDecoder.Decode(new MemoryStream(serialized)); + + Verify(input, arrayRecord); + Assert.Equal(input, arrayRecord.GetArray(input.GetType())); + Assert.Equal(3 * 4 * 5, arrayRecord.TotalElementsCount); + } + [Fact] public void CanReadJaggedArraysOfPrimitiveTypes_3D() { From ea50be4c7aea3f8c867e633ccac39d147545b7bd Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 20 Aug 2024 19:44:19 +0200 Subject: [PATCH 07/11] address feedback: - rename TotalElementsCount to FlattenedLength - Ensure lack of recursive call - add a comment explaining null nuances --- .../ref/System.Formats.Nrbf.cs | 2 +- .../src/System/Formats/Nrbf/ArrayInfo.cs | 8 ++++---- .../src/System/Formats/Nrbf/ArrayRecord.cs | 4 ++-- .../System/Formats/Nrbf/BinaryArrayRecord.cs | 17 ++++++++++------- .../Formats/Nrbf/RectangularArrayRecord.cs | 4 ++-- .../tests/ArraySinglePrimitiveRecordTests.cs | 2 +- .../tests/JaggedArraysTests.cs | 18 +++++++++--------- .../tests/RectangularArraysTests.cs | 2 +- 8 files changed, 30 insertions(+), 27 deletions(-) diff --git a/src/libraries/System.Formats.Nrbf/ref/System.Formats.Nrbf.cs b/src/libraries/System.Formats.Nrbf/ref/System.Formats.Nrbf.cs index cb91110f6c8c9..d7a6e01a72352 100644 --- a/src/libraries/System.Formats.Nrbf/ref/System.Formats.Nrbf.cs +++ b/src/libraries/System.Formats.Nrbf/ref/System.Formats.Nrbf.cs @@ -11,7 +11,7 @@ public abstract partial class ArrayRecord : System.Formats.Nrbf.SerializationRec internal ArrayRecord() { } public override System.Formats.Nrbf.SerializationRecordId Id { get { throw null; } } public abstract System.ReadOnlySpan Lengths { get; } - public virtual long TotalElementsCount { get; } + public virtual long FlattenedLength { get; } public int Rank { get { throw null; } } [System.Diagnostics.CodeAnalysis.RequiresDynamicCode("The code for an array of the specified type might not be available.")] public System.Array GetArray(System.Type expectedArrayType, bool allowNulls = true) { throw null; } diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayInfo.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayInfo.cs index e8b28825888e4..3f84437b7d1aa 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayInfo.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayInfo.cs @@ -21,14 +21,14 @@ internal readonly struct ArrayInfo internal ArrayInfo(SerializationRecordId id, long totalElementsCount, BinaryArrayType arrayType = BinaryArrayType.Single, int rank = 1) { Id = id; - TotalElementsCount = totalElementsCount; + FlattenedLength = totalElementsCount; ArrayType = arrayType; Rank = rank; } internal SerializationRecordId Id { get; } - internal long TotalElementsCount { get; } + internal long FlattenedLength { get; } internal BinaryArrayType ArrayType { get; } @@ -36,8 +36,8 @@ internal ArrayInfo(SerializationRecordId id, long totalElementsCount, BinaryArra internal int GetSZArrayLength() { - Debug.Assert(TotalElementsCount <= MaxArrayLength); - return (int)TotalElementsCount; + Debug.Assert(FlattenedLength <= MaxArrayLength); + return (int)FlattenedLength; } internal static ArrayInfo Decode(BinaryReader reader) diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs index 0f760c1da0373..aa2e168c2f0a7 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs @@ -18,7 +18,7 @@ public abstract class ArrayRecord : SerializationRecord private protected ArrayRecord(ArrayInfo arrayInfo) { ArrayInfo = arrayInfo; - ValuesToRead = arrayInfo.TotalElementsCount; + ValuesToRead = arrayInfo.FlattenedLength; } /// @@ -31,7 +31,7 @@ private protected ArrayRecord(ArrayInfo arrayInfo) /// When overridden in a derived class, gets the total number of all elements in every dimension. /// /// A number that represent the total number of all elements in every dimension. - public virtual long TotalElementsCount => ArrayInfo.TotalElementsCount; + public virtual long FlattenedLength => ArrayInfo.FlattenedLength; /// /// Gets the rank of the array. diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs index d5c31c30be53d..ed27b51e3bccb 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs @@ -45,15 +45,15 @@ private BinaryArrayRecord(ArrayInfo arrayInfo, MemberTypeInfo memberTypeInfo) public override ReadOnlySpan Lengths => new int[1] { Length }; /// - public override long TotalElementsCount + public override long FlattenedLength { get { if (_totalElementsCount < 0) { _totalElementsCount = IsJagged - ? GetJaggedArrayTotalElementsCount(this) - : ArrayInfo.TotalElementsCount; + ? GetJaggedArrayFlattenedLength(this) + : ArrayInfo.FlattenedLength; } return _totalElementsCount; @@ -177,7 +177,7 @@ internal static ArrayRecord Decode(BinaryReader reader, RecordMap recordMap, Pay : new BinaryArrayRecord(arrayInfo, memberTypeInfo); } - private static long GetJaggedArrayTotalElementsCount(BinaryArrayRecord jaggedArrayRecord) + private static long GetJaggedArrayFlattenedLength(BinaryArrayRecord jaggedArrayRecord) { long result = 0; Queue? jaggedArrayRecords = null; @@ -216,14 +216,17 @@ private static long GetJaggedArrayTotalElementsCount(BinaryArrayRecord jaggedArr } else { - result += nestedArrayRecord.TotalElementsCount; + Debug.Assert(nestedArrayRecord is not BinaryArrayRecord, "Ensure lack of recursive call"); + result += nestedArrayRecord.FlattenedLength; } break; case SerializationRecordType.ObjectNull: case SerializationRecordType.ObjectNullMultiple256: case SerializationRecordType.ObjectNullMultiple: - // Null Records nested inside jagged array do not increase total elements count. - // Example: "int[][] input = [[1, 2, 3], null]" is just 3 elements in total. + // Null Records that represent null arrays (not null elements in the most inner array) + // nested inside jagged array do not increase flat length count. + // Example: "string[][] input = [["1", "2", "3"], null]" is 3 elements in total. + // Example: "string[][] input = [["1", "2", "3"], ["4", "5", null]]" is 6 elements in total. break; default: result++; diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/RectangularArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/RectangularArrayRecord.cs index de3c6d671850a..da5318115157a 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/RectangularArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/RectangularArrayRecord.cs @@ -31,7 +31,7 @@ private RectangularArrayRecord(Type elementType, ArrayInfo arrayInfo, #if DEBUG _values = new LinkedList(); #else - _values = arrayInfo.TotalElementsCount <= ArrayInfo.MaxArrayLength + _values = arrayInfo.FlattenedLength <= ArrayInfo.MaxArrayLength ? new List(canPreAllocate ? arrayInfo.GetSZArrayLength() : Math.Min(4, arrayInfo.GetSZArrayLength())) : new LinkedList(); #endif @@ -181,7 +181,7 @@ internal static RectangularArrayRecord Create(BinaryReader reader, ArrayInfo arr if (sizeOfSingleValue > 0) { - long size = arrayInfo.TotalElementsCount * sizeOfSingleValue; + long size = arrayInfo.FlattenedLength * sizeOfSingleValue; bool? isDataAvailable = reader.IsDataAvailable(size); if (isDataAvailable.HasValue) { diff --git a/src/libraries/System.Formats.Nrbf/tests/ArraySinglePrimitiveRecordTests.cs b/src/libraries/System.Formats.Nrbf/tests/ArraySinglePrimitiveRecordTests.cs index 56091bf0182d2..4d923695f9f7f 100644 --- a/src/libraries/System.Formats.Nrbf/tests/ArraySinglePrimitiveRecordTests.cs +++ b/src/libraries/System.Formats.Nrbf/tests/ArraySinglePrimitiveRecordTests.cs @@ -94,7 +94,7 @@ private void Test(int size, bool canSeek) SZArrayRecord arrayRecord = (SZArrayRecord)NrbfDecoder.Decode(stream); Assert.Equal(size, arrayRecord.Length); - Assert.Equal(size, arrayRecord.TotalElementsCount); + Assert.Equal(size, arrayRecord.FlattenedLength); T?[] output = arrayRecord.GetArray(); Assert.Equal(input, output); Assert.Same(output, arrayRecord.GetArray()); diff --git a/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs b/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs index e4567c87cd781..4c2be8e9b52f8 100644 --- a/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs +++ b/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs @@ -20,11 +20,11 @@ public void CanReadJaggedArraysOfPrimitiveTypes_2D() Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); - Assert.Equal(input.Length * 3, arrayRecord.TotalElementsCount); + Assert.Equal(input.Length * 3, arrayRecord.FlattenedLength); } [Fact] - public void TotalElementsCountDoesNotIncludeNullArrays() + public void FlattenedLengthDoesNotIncludeNullArrays() { int[][] input = [[1, 2, 3], null]; @@ -32,7 +32,7 @@ public void TotalElementsCountDoesNotIncludeNullArrays() Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); - Assert.Equal(3, arrayRecord.TotalElementsCount); + Assert.Equal(3, arrayRecord.FlattenedLength); } [Fact] @@ -64,7 +64,7 @@ public void ItIsPossibleToHaveBinaryArrayRecordsHaveAnElementTypeOfArrayWithoutB Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); - Assert.Equal(3 * 4 * 5, arrayRecord.TotalElementsCount); + Assert.Equal(3 * 4 * 5, arrayRecord.FlattenedLength); } [Fact] @@ -82,7 +82,7 @@ public void CanReadJaggedArraysOfPrimitiveTypes_3D() Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); Assert.Equal(1, arrayRecord.Rank); - Assert.Equal(input.Length * 1 * 3, arrayRecord.TotalElementsCount); + Assert.Equal(input.Length * 1 * 3, arrayRecord.FlattenedLength); } [Fact] @@ -107,7 +107,7 @@ public void CanReadJaggedArrayOfRectangularArrays() Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); Assert.Equal(1, arrayRecord.Rank); - Assert.Equal(input.Length * 3 * 3, arrayRecord.TotalElementsCount); + Assert.Equal(input.Length * 3 * 3, arrayRecord.FlattenedLength); } [Fact] @@ -123,7 +123,7 @@ public void CanReadJaggedArraysOfStrings() Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); - Assert.Equal(input.Length * 3, arrayRecord.TotalElementsCount); + Assert.Equal(input.Length * 3, arrayRecord.FlattenedLength); } [Fact] @@ -139,7 +139,7 @@ public void CanReadJaggedArraysOfObjects() Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); - Assert.Equal(input.Length * 3, arrayRecord.TotalElementsCount); + Assert.Equal(input.Length * 3, arrayRecord.FlattenedLength); } [Serializable] @@ -162,7 +162,7 @@ public void CanReadJaggedArraysOfComplexTypes() var arrayRecord = (ArrayRecord)NrbfDecoder.Decode(Serialize(input)); Verify(input, arrayRecord); - Assert.Equal(totalElementsCount, arrayRecord.TotalElementsCount); + Assert.Equal(totalElementsCount, arrayRecord.FlattenedLength); var output = (ClassRecord?[][])arrayRecord.GetArray(input.GetType()); for (int i = 0; i < input.Length; i++) { diff --git a/src/libraries/System.Formats.Nrbf/tests/RectangularArraysTests.cs b/src/libraries/System.Formats.Nrbf/tests/RectangularArraysTests.cs index f3e7671c6b20c..3191d57ba807c 100644 --- a/src/libraries/System.Formats.Nrbf/tests/RectangularArraysTests.cs +++ b/src/libraries/System.Formats.Nrbf/tests/RectangularArraysTests.cs @@ -229,7 +229,7 @@ internal static void Verify(Array input, ArrayRecord arrayRecord) Assert.Equal(input.GetLength(i), arrayRecord.Lengths[i]); totalElementsCount *= input.GetLength(i); } - Assert.Equal(totalElementsCount, arrayRecord.TotalElementsCount); + Assert.Equal(totalElementsCount, arrayRecord.FlattenedLength); Assert.Equal(input.GetType().FullName, arrayRecord.TypeName.FullName); Assert.Equal(input.GetType().GetAssemblyNameIncludingTypeForwards(), arrayRecord.TypeName.AssemblyName!.FullName); } From e740b0a5d08ec55272406a7617abc2afe80f0f45 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 20 Aug 2024 19:56:14 +0200 Subject: [PATCH 08/11] improve exception messages: - don't include member name in the exception message - remove unused resource - make the argument int rather than an object to make it clear it's never a string --- .../System.Formats.Nrbf/src/Resources/Strings.resx | 5 +---- .../src/System/Formats/Nrbf/ClassInfo.cs | 2 +- .../src/System/Formats/Nrbf/Utils/ThrowHelper.cs | 10 +++++----- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/libraries/System.Formats.Nrbf/src/Resources/Strings.resx b/src/libraries/System.Formats.Nrbf/src/Resources/Strings.resx index 14eb97bd1db12..f3fcec88dff33 100644 --- a/src/libraries/System.Formats.Nrbf/src/Resources/Strings.resx +++ b/src/libraries/System.Formats.Nrbf/src/Resources/Strings.resx @@ -126,9 +126,6 @@ Unexpected Null Record count. - - The serialized array length ({0}) was larger than the configured limit {1}. - {0} Record Type is not supported by design. @@ -145,7 +142,7 @@ Invalid type or assembly name. - Duplicate member name: `{0}`. + Duplicate member name. Stream does not support seeking. diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ClassInfo.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ClassInfo.cs index 75340b72a4f0d..069f85f6758ac 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ClassInfo.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ClassInfo.cs @@ -70,7 +70,7 @@ internal static ClassInfo Decode(BinaryReader reader) continue; } #endif - throw new SerializationException(SR.Format(SR.Serialization_DuplicateMemberName, memberName)); + ThrowHelper.ThrowDuplicateMemberName(); } return new ClassInfo(id, typeName, memberNames); diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/ThrowHelper.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/ThrowHelper.cs index 275b70aca5742..9b71dbe161c23 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/ThrowHelper.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/ThrowHelper.cs @@ -6,11 +6,14 @@ namespace System.Formats.Nrbf.Utils; -// The exception messages do not contain type or assembly names on purpose, +// The exception messages do not contain member/type/assembly names on purpose, // as it's most likely corrupted/tampered/malicious data. internal static class ThrowHelper { - internal static void ThrowInvalidValue(object value) + internal static void ThrowDuplicateMemberName() + => throw new SerializationException(SR.Serialization_DuplicateMemberName); + + internal static void ThrowInvalidValue(int value) => throw new SerializationException(SR.Format(SR.Serialization_InvalidValue, value)); internal static void ThrowInvalidReference() @@ -22,9 +25,6 @@ internal static void ThrowInvalidTypeName() internal static void ThrowUnexpectedNullRecordCount() => throw new SerializationException(SR.Serialization_UnexpectedNullRecordCount); - internal static void ThrowMaxArrayLength(long limit, long actual) - => throw new SerializationException(SR.Format(SR.Serialization_MaxArrayLength, actual, limit)); - internal static void ThrowArrayContainedNulls() => throw new SerializationException(SR.Serialization_ArrayContainedNulls); From 140bf927ae16bd5059d45c792ddfad752a1c11fa Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 21 Aug 2024 10:19:33 +0200 Subject: [PATCH 09/11] add checked to ensure no long overflow for very unusual payloads --- .../src/System/Formats/Nrbf/BinaryArrayRecord.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs index ed27b51e3bccb..db9a7a304f153 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs @@ -217,7 +217,12 @@ private static long GetJaggedArrayFlattenedLength(BinaryArrayRecord jaggedArrayR else { Debug.Assert(nestedArrayRecord is not BinaryArrayRecord, "Ensure lack of recursive call"); - result += nestedArrayRecord.FlattenedLength; + // In theory somebody could create a payload that would represent + // a very nested array with total elements count > long.MaxValue. + checked + { + result += nestedArrayRecord.FlattenedLength; + } } break; case SerializationRecordType.ObjectNull: From 16394e228c77987ab5fde1fff6c6836fe7909c91 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 28 Aug 2024 19:41:23 +0200 Subject: [PATCH 10/11] FlattenedLength must include nulls --- .../src/System/Formats/Nrbf/BinaryArrayRecord.cs | 13 +++++++------ .../System.Formats.Nrbf/tests/JaggedArraysTests.cs | 11 +++++++---- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs index db9a7a304f153..1703763fbb154 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs @@ -217,10 +217,10 @@ private static long GetJaggedArrayFlattenedLength(BinaryArrayRecord jaggedArrayR else { Debug.Assert(nestedArrayRecord is not BinaryArrayRecord, "Ensure lack of recursive call"); - // In theory somebody could create a payload that would represent - // a very nested array with total elements count > long.MaxValue. checked { + // In theory somebody could create a payload that would represent + // a very nested array with total elements count > long.MaxValue. result += nestedArrayRecord.FlattenedLength; } } @@ -228,10 +228,11 @@ private static long GetJaggedArrayFlattenedLength(BinaryArrayRecord jaggedArrayR case SerializationRecordType.ObjectNull: case SerializationRecordType.ObjectNullMultiple256: case SerializationRecordType.ObjectNullMultiple: - // Null Records that represent null arrays (not null elements in the most inner array) - // nested inside jagged array do not increase flat length count. - // Example: "string[][] input = [["1", "2", "3"], null]" is 3 elements in total. - // Example: "string[][] input = [["1", "2", "3"], ["4", "5", null]]" is 6 elements in total. + // All nulls need to be included, as it's another form of possible attack. + checked + { + result += ((NullsRecord)item).NullCount; + } break; default: result++; diff --git a/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs b/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs index 4c2be8e9b52f8..488b7992d29e7 100644 --- a/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs +++ b/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs @@ -23,16 +23,19 @@ public void CanReadJaggedArraysOfPrimitiveTypes_2D() Assert.Equal(input.Length * 3, arrayRecord.FlattenedLength); } - [Fact] - public void FlattenedLengthDoesNotIncludeNullArrays() + [Theory] + [InlineData(1)] // SerializationRecordType.ObjectNull + [InlineData(200)] // SerializationRecordType.ObjectNullMultiple256 + [InlineData(10_000)] // SerializationRecordType.ObjectNullMultiple + public void FlattenedLengthIncludesNullArrays(int nullCount) { - int[][] input = [[1, 2, 3], null]; + int[][] input = new int[nullCount][]; var arrayRecord = (ArrayRecord)NrbfDecoder.Decode(Serialize(input)); Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); - Assert.Equal(3, arrayRecord.FlattenedLength); + Assert.Equal(nullCount, arrayRecord.FlattenedLength); } [Fact] From 8a61178089d9625e12351adb9eaebca49a02e611 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 13 Sep 2024 12:20:59 +0200 Subject: [PATCH 11/11] address code review feedback: count the arrays themselves --- .../src/System/Formats/Nrbf/ArrayRecord.cs | 2 +- .../System/Formats/Nrbf/BinaryArrayRecord.cs | 39 +++++++------------ .../tests/JaggedArraysTests.cs | 34 +++++++++++----- 3 files changed, 41 insertions(+), 34 deletions(-) diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs index aa2e168c2f0a7..237b7b72a2719 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs @@ -50,7 +50,7 @@ private protected ArrayRecord(ArrayInfo arrayInfo) internal long ValuesToRead { get; private protected set; } - private protected ArrayInfo ArrayInfo { get; } + internal ArrayInfo ArrayInfo { get; } internal bool IsJagged => ArrayInfo.ArrayType == BinaryArrayType.Jagged diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs index be8fb01d9ea3b..5e55f1cfbf8d1 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs @@ -191,24 +191,29 @@ private static long GetJaggedArrayFlattenedLength(BinaryArrayRecord jaggedArrayR Debug.Assert(jaggedArrayRecord.IsJagged); + // In theory somebody could create a payload that would represent + // a very nested array with total elements count > long.MaxValue. + // That is why this method is using checked arithmetic. + result = checked(result + jaggedArrayRecord.Length); // count the arrays themselves + foreach (object value in jaggedArrayRecord.Values) { - object item = value is MemberReferenceRecord referenceRecord - ? referenceRecord.GetReferencedRecord() - : value; - - if (item is not SerializationRecord record) + if (value is not SerializationRecord record) { - result++; continue; } + if (record.RecordType == SerializationRecordType.MemberReference) + { + record = ((MemberReferenceRecord)record).GetReferencedRecord(); + } + switch (record.RecordType) { - case SerializationRecordType.BinaryArray: case SerializationRecordType.ArraySinglePrimitive: case SerializationRecordType.ArraySingleObject: case SerializationRecordType.ArraySingleString: + case SerializationRecordType.BinaryArray: ArrayRecord nestedArrayRecord = (ArrayRecord)record; if (nestedArrayRecord.IsJagged) { @@ -216,26 +221,12 @@ private static long GetJaggedArrayFlattenedLength(BinaryArrayRecord jaggedArrayR } else { - Debug.Assert(nestedArrayRecord is not BinaryArrayRecord, "Ensure lack of recursive call"); - checked - { - // In theory somebody could create a payload that would represent - // a very nested array with total elements count > long.MaxValue. - result += nestedArrayRecord.FlattenedLength; - } - } - break; - case SerializationRecordType.ObjectNull: - case SerializationRecordType.ObjectNullMultiple256: - case SerializationRecordType.ObjectNullMultiple: - // All nulls need to be included, as it's another form of possible attack. - checked - { - result += ((NullsRecord)item).NullCount; + // Don't call nestedArrayRecord.FlattenedLength to avoid any potential recursion, + // just call nestedArrayRecord.ArrayInfo.FlattenedLength that returns pre-computed value. + result = checked(result + nestedArrayRecord.ArrayInfo.FlattenedLength); } break; default: - result++; break; } } diff --git a/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs b/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs index 488b7992d29e7..8bb844ff76a58 100644 --- a/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs +++ b/src/libraries/System.Formats.Nrbf/tests/JaggedArraysTests.cs @@ -7,20 +7,25 @@ namespace System.Formats.Nrbf.Tests; public class JaggedArraysTests : ReadTests { - [Fact] - public void CanReadJaggedArraysOfPrimitiveTypes_2D() + [Theory] + [InlineData(true)] + [InlineData(false)] + public void CanReadJaggedArraysOfPrimitiveTypes_2D(bool useReferences) { int[][] input = new int[7][]; + int[] same = [1, 2, 3]; for (int i = 0; i < input.Length; i++) { - input[i] = [i, i, i]; + input[i] = useReferences + ? same // reuse the same object (represented as a single record that is referenced multiple times) + : [i, i, i]; // create new array } var arrayRecord = (ArrayRecord)NrbfDecoder.Decode(Serialize(input)); Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); - Assert.Equal(input.Length * 3, arrayRecord.FlattenedLength); + Assert.Equal(input.Length + input.Length * 3, arrayRecord.FlattenedLength); } [Theory] @@ -42,13 +47,17 @@ public void FlattenedLengthIncludesNullArrays(int nullCount) public void ItIsPossibleToHaveBinaryArrayRecordsHaveAnElementTypeOfArrayWithoutBeingMarkedAsJagged() { int[][][] input = new int[3][][]; + long totalElementsCount = 0; for (int i = 0; i < input.Length; i++) { input[i] = new int[4][]; + totalElementsCount++; // count the arrays themselves for (int j = 0; j < input[i].Length; j++) { input[i][j] = [i, j, 0, 1, 2]; + totalElementsCount += input[i][j].Length; + totalElementsCount++; // count the arrays themselves } } @@ -67,17 +76,22 @@ public void ItIsPossibleToHaveBinaryArrayRecordsHaveAnElementTypeOfArrayWithoutB Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); - Assert.Equal(3 * 4 * 5, arrayRecord.FlattenedLength); + Assert.Equal(3 + 3 * 4 + 3 * 4 * 5, totalElementsCount); + Assert.Equal(totalElementsCount, arrayRecord.FlattenedLength); } [Fact] public void CanReadJaggedArraysOfPrimitiveTypes_3D() { int[][][] input = new int[7][][]; + long totalElementsCount = 0; for (int i = 0; i < input.Length; i++) { + totalElementsCount++; // count the arrays themselves input[i] = new int[1][]; + totalElementsCount++; // count the arrays themselves input[i][0] = [i, i, i]; + totalElementsCount += input[i][0].Length; } var arrayRecord = (ArrayRecord)NrbfDecoder.Decode(Serialize(input)); @@ -85,7 +99,8 @@ public void CanReadJaggedArraysOfPrimitiveTypes_3D() Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); Assert.Equal(1, arrayRecord.Rank); - Assert.Equal(input.Length * 1 * 3, arrayRecord.FlattenedLength); + Assert.Equal(7 + 7 * 1 + 7 * 1 * 3, totalElementsCount); + Assert.Equal(totalElementsCount, arrayRecord.FlattenedLength); } [Fact] @@ -110,7 +125,7 @@ public void CanReadJaggedArrayOfRectangularArrays() Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); Assert.Equal(1, arrayRecord.Rank); - Assert.Equal(input.Length * 3 * 3, arrayRecord.FlattenedLength); + Assert.Equal(input.Length + input.Length * 3 * 3, arrayRecord.FlattenedLength); } [Fact] @@ -126,7 +141,7 @@ public void CanReadJaggedArraysOfStrings() Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); - Assert.Equal(input.Length * 3, arrayRecord.FlattenedLength); + Assert.Equal(input.Length + input.Length * 3, arrayRecord.FlattenedLength); } [Fact] @@ -142,7 +157,7 @@ public void CanReadJaggedArraysOfObjects() Verify(input, arrayRecord); Assert.Equal(input, arrayRecord.GetArray(input.GetType())); - Assert.Equal(input.Length * 3, arrayRecord.FlattenedLength); + Assert.Equal(input.Length + input.Length * 3, arrayRecord.FlattenedLength); } [Serializable] @@ -160,6 +175,7 @@ public void CanReadJaggedArraysOfComplexTypes() { input[i] = Enumerable.Range(0, i + 1).Select(j => new ComplexType { SomeField = j }).ToArray(); totalElementsCount += input[i].Length; + totalElementsCount++; // count the arrays themselves } var arrayRecord = (ArrayRecord)NrbfDecoder.Decode(Serialize(input));