From ab07f0d56f53c1980896c53fc2aa19afd4b4b747 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 13 Aug 2026 21:53:28 +0000 Subject: [PATCH 1/2] Improve NRBF ArrayRecord.GetArray(allowNulls: false) perf by throwing early when null records are detected Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com> --- .../src/System/Formats/Nrbf/ArrayRecord.cs | 6 ++++ .../Formats/Nrbf/ArraySingleObjectRecord.cs | 6 ++++ .../Formats/Nrbf/ArraySingleStringRecord.cs | 6 ++++ .../System/Formats/Nrbf/SZArrayOfRecords.cs | 6 ++++ .../tests/ArrayOfSerializationRecordsTests.cs | 35 +++++++++++++++++++ 5 files changed, 59 insertions(+) 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 4fe6fb1e0942b4..bfa399818de4d6 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 @@ -116,6 +116,12 @@ private void HandleNext(object value, NextInfo info, int size) internal static void Populate(List source, Array destination, int[] lengths, AllowedRecordTypes allowedRecordTypes, bool allowNulls) { + // When destination length is different than record count, we know the record list contains at least one Multiple Null Record. + if (!allowNulls && destination.LongLength != source.Count) + { + ThrowHelper.ThrowArrayContainedNulls(); + } + int[] indices = new int[lengths.Length]; nuint numElementsWritten = 0; // only for debugging; not used in release builds diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleObjectRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleObjectRecord.cs index 3010b2930494ae..58589c19e82290 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleObjectRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleObjectRecord.cs @@ -32,6 +32,12 @@ public override TypeName TypeName private SerializationRecord?[] ToArray(bool allowNulls) { + // When Length is different than record count, we know the record list contains at least one Multiple Null Record. + if (!allowNulls && Length != Records.Count) + { + ThrowHelper.ThrowArrayContainedNulls(); + } + SerializationRecord?[] values = new SerializationRecord?[Length]; int valueIndex = 0; diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleStringRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleStringRecord.cs index b276fc8955c5f7..72468c853b8616 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleStringRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleStringRecord.cs @@ -46,6 +46,12 @@ internal override (AllowedRecordTypes allowed, PrimitiveType primitiveType) GetA private string?[] ToArray(bool allowNulls) { + // When Length is different than record count, we know the record list contains at least one Multiple Null Record. + if (!allowNulls && Length != Records.Count) + { + ThrowHelper.ThrowArrayContainedNulls(); + } + string?[] values = new string?[Length]; int valueIndex = 0; diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SZArrayOfRecords.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SZArrayOfRecords.cs index b77a4a57a2a348..6cd707875e96bc 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SZArrayOfRecords.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SZArrayOfRecords.cs @@ -38,6 +38,12 @@ public override TypeName TypeName private SerializationRecord?[] ToArray(bool allowNulls) { + // When Length is different than record count, we know the record list contains at least one Multiple Null Record. + if (!allowNulls && Length != Records.Count) + { + ThrowHelper.ThrowArrayContainedNulls(); + } + SerializationRecord?[] result = new SerializationRecord?[Length]; int resultIndex = 0; diff --git a/src/libraries/System.Formats.Nrbf/tests/ArrayOfSerializationRecordsTests.cs b/src/libraries/System.Formats.Nrbf/tests/ArrayOfSerializationRecordsTests.cs index 18e39a5fd68e1f..3bb210402ddc9f 100644 --- a/src/libraries/System.Formats.Nrbf/tests/ArrayOfSerializationRecordsTests.cs +++ b/src/libraries/System.Formats.Nrbf/tests/ArrayOfSerializationRecordsTests.cs @@ -512,5 +512,40 @@ private static void Verify(Array input, ArrayRecord arrayRecord, Array output, } } } + +#if !NETFRAMEWORK + [Fact] + public void GetArray_AllowNullsFalse_DoesNotAllocateLargeArray_WhenMultipleNullRecordIsPresent() + { + // The tests need to ensure that 2GB+ array does not get allocated. + // 200k is enough to get the job done and avoid getting false positives. + const long AllocationThreshold = 200_000; + + using MemoryStream stream = new(); + BinaryWriter writer = new(stream, Text.Encoding.UTF8); + + WriteSerializedStreamHeader(writer); + + writer.Write((byte)SerializationRecordType.ArraySingleObject); + writer.Write(1); // object ID + writer.Write(Array.MaxLength); // length + writer.Write((byte)SerializationRecordType.ObjectNullMultiple); + writer.Write(Array.MaxLength); // null count + writer.Write((byte)SerializationRecordType.MessageEnd); + + stream.Position = 0; + + SerializationRecord serializationRecord = NrbfDecoder.Decode(stream); + + long before = GC.GetAllocatedBytesForCurrentThread(); + + SZArrayRecord arrayRecord = (SZArrayRecord)serializationRecord; + Assert.Throws(() => arrayRecord.GetArray(allowNulls: false)); + + long after = GC.GetAllocatedBytesForCurrentThread(); + + Assert.InRange(after, before, before + AllocationThreshold); + } +#endif } } From bbc5c737b123bc95104717f3b80bc087792025c6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 13 Aug 2026 22:47:58 +0000 Subject: [PATCH 2/2] Move null check before array allocation and cover all SZ array record types with a Theory Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com> --- .../src/System/Formats/Nrbf/ArrayRecord.cs | 16 ++++++-- .../Formats/Nrbf/ArraySingleObjectRecord.cs | 6 +-- .../Formats/Nrbf/ArraySingleStringRecord.cs | 6 +-- .../System/Formats/Nrbf/JaggedArrayRecord.cs | 2 + .../Formats/Nrbf/RectangularArrayRecord.cs | 2 + .../System/Formats/Nrbf/SZArrayOfRecords.cs | 6 +-- .../tests/ArrayOfSerializationRecordsTests.cs | 41 +++++++++++++++---- 7 files changed, 54 insertions(+), 25 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 bfa399818de4d6..cda3ca97dee6b2 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 @@ -114,14 +114,24 @@ private void HandleNext(object value, NextInfo info, int size) internal abstract (AllowedRecordTypes allowed, PrimitiveType primitiveType) GetAllowedRecordType(); - internal static void Populate(List source, Array destination, int[] lengths, AllowedRecordTypes allowedRecordTypes, bool allowNulls) + /// + /// Throws if nulls are not allowed and the records are known to contain at least one Multiple Null Record. + /// + /// + /// Every record represents a single value, except for the Multiple Null Records which represent more than one null. + /// Because of that, a record count different than the total element count means that nulls are present. + /// It's important to perform this check before allocating the array, as the array can be very large. + /// + private protected void ThrowIfNullsAreNotAllowedButPresent(List records, bool allowNulls) { - // When destination length is different than record count, we know the record list contains at least one Multiple Null Record. - if (!allowNulls && destination.LongLength != source.Count) + if (!allowNulls && ArrayInfo.FlattenedLength != records.Count) { ThrowHelper.ThrowArrayContainedNulls(); } + } + internal static void Populate(List source, Array destination, int[] lengths, AllowedRecordTypes allowedRecordTypes, bool allowNulls) + { int[] indices = new int[lengths.Length]; nuint numElementsWritten = 0; // only for debugging; not used in release builds diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleObjectRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleObjectRecord.cs index 58589c19e82290..564b2e7924a41c 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleObjectRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleObjectRecord.cs @@ -32,11 +32,7 @@ public override TypeName TypeName private SerializationRecord?[] ToArray(bool allowNulls) { - // When Length is different than record count, we know the record list contains at least one Multiple Null Record. - if (!allowNulls && Length != Records.Count) - { - ThrowHelper.ThrowArrayContainedNulls(); - } + ThrowIfNullsAreNotAllowedButPresent(Records, allowNulls); SerializationRecord?[] values = new SerializationRecord?[Length]; diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleStringRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleStringRecord.cs index 72468c853b8616..aa104ee0cad646 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleStringRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleStringRecord.cs @@ -46,11 +46,7 @@ internal override (AllowedRecordTypes allowed, PrimitiveType primitiveType) GetA private string?[] ToArray(bool allowNulls) { - // When Length is different than record count, we know the record list contains at least one Multiple Null Record. - if (!allowNulls && Length != Records.Count) - { - ThrowHelper.ThrowArrayContainedNulls(); - } + ThrowIfNullsAreNotAllowedButPresent(Records, allowNulls); string?[] values = new string?[Length]; diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/JaggedArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/JaggedArrayRecord.cs index 6ac97ef40675d6..9d596e673d2995 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/JaggedArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/JaggedArrayRecord.cs @@ -45,6 +45,8 @@ internal JaggedArrayRecord(ArrayInfo arrayInfo, MemberTypeInfo memberTypeInfo, i [RequiresDynamicCode("May call Array.CreateInstance().")] private protected override Array Deserialize(Type arrayType, bool allowNulls) { + ThrowIfNullsAreNotAllowedButPresent(_records, allowNulls); + // This method returns arrays of ArrayRecords. Array array = _lengths.Length switch { 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 64cd104eea44e1..ea3d8b10ad86e2 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 @@ -46,6 +46,8 @@ private protected override Array Deserialize(Type arrayType, bool allowNulls) { bool storeStrings = _elementType == typeof(string); + ThrowIfNullsAreNotAllowedButPresent(_records, allowNulls); + // We can not deserialize non-string types. // This method returns arrays of SerializationRecord for arrays of complex types. Array result = diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SZArrayOfRecords.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SZArrayOfRecords.cs index 6cd707875e96bc..c71f352af38a67 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SZArrayOfRecords.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SZArrayOfRecords.cs @@ -38,11 +38,7 @@ public override TypeName TypeName private SerializationRecord?[] ToArray(bool allowNulls) { - // When Length is different than record count, we know the record list contains at least one Multiple Null Record. - if (!allowNulls && Length != Records.Count) - { - ThrowHelper.ThrowArrayContainedNulls(); - } + ThrowIfNullsAreNotAllowedButPresent(Records, allowNulls); SerializationRecord?[] result = new SerializationRecord?[Length]; diff --git a/src/libraries/System.Formats.Nrbf/tests/ArrayOfSerializationRecordsTests.cs b/src/libraries/System.Formats.Nrbf/tests/ArrayOfSerializationRecordsTests.cs index 3bb210402ddc9f..4f60c30bac2f11 100644 --- a/src/libraries/System.Formats.Nrbf/tests/ArrayOfSerializationRecordsTests.cs +++ b/src/libraries/System.Formats.Nrbf/tests/ArrayOfSerializationRecordsTests.cs @@ -514,10 +514,14 @@ private static void Verify(Array input, ArrayRecord arrayRecord, Array output, } #if !NETFRAMEWORK - [Fact] - public void GetArray_AllowNullsFalse_DoesNotAllocateLargeArray_WhenMultipleNullRecordIsPresent() + // GC.GetAllocatedBytesForCurrentThread() is not available on Full Framework. + [Theory] + [InlineData(SerializationRecordType.ArraySingleObject)] + [InlineData(SerializationRecordType.ArraySingleString)] + [InlineData(SerializationRecordType.BinaryArray)] + public void GetArray_AllowNullsFalse_DoesNotAllocateLargeArray_WhenMultipleNullRecordIsPresent(SerializationRecordType recordType) { - // The tests need to ensure that 2GB+ array does not get allocated. + // The test needs to ensure that 2GB+ array does not get allocated. // 200k is enough to get the job done and avoid getting false positives. const long AllocationThreshold = 200_000; @@ -526,9 +530,22 @@ public void GetArray_AllowNullsFalse_DoesNotAllocateLargeArray_WhenMultipleNullR WriteSerializedStreamHeader(writer); - writer.Write((byte)SerializationRecordType.ArraySingleObject); + writer.Write((byte)recordType); writer.Write(1); // object ID - writer.Write(Array.MaxLength); // length + + if (recordType is SerializationRecordType.BinaryArray) + { + writer.Write((byte)BinaryArrayType.Single); + writer.Write(1); // rank + writer.Write(Array.MaxLength); // length + writer.Write((byte)3); // BinaryType.SystemClass + writer.Write("System.Exception"); // element type name + } + else + { + writer.Write(Array.MaxLength); // length + } + writer.Write((byte)SerializationRecordType.ObjectNullMultiple); writer.Write(Array.MaxLength); // null count writer.Write((byte)SerializationRecordType.MessageEnd); @@ -536,11 +553,21 @@ public void GetArray_AllowNullsFalse_DoesNotAllocateLargeArray_WhenMultipleNullR stream.Position = 0; SerializationRecord serializationRecord = NrbfDecoder.Decode(stream); + Assert.Equal(recordType, serializationRecord.RecordType); long before = GC.GetAllocatedBytesForCurrentThread(); - SZArrayRecord arrayRecord = (SZArrayRecord)serializationRecord; - Assert.Throws(() => arrayRecord.GetArray(allowNulls: false)); + Assert.Throws(() => + { + if (serializationRecord is SZArrayRecord arrayOfStrings) + { + arrayOfStrings.GetArray(allowNulls: false); + } + else + { + ((SZArrayRecord)serializationRecord).GetArray(allowNulls: false); + } + }); long after = GC.GetAllocatedBytesForCurrentThread();