[NRBF] Comments and bug fixes from internal code review (dotnet#107735)

* copy comments and asserts from Levis internal code review

* apply Levis suggestion: don't store Array.MaxLength as a const, as it may change in the future

* add missing and fix some of the existing comments

* first bug fix: SerializationRecord.TypeNameMatches should throw ArgumentNullException for null Type argument

* second bug fix: SerializationRecord.TypeNameMatches should know the difference between SZArray and single-dimension, non-zero offset arrays (example: int[] and int[*])

* third bug fix: don't cast bytes to booleans

* fourth bug fix: don't cast bytes to DateTimes

* add one test case that I've forgot in previous PR

Author: adamsitnik

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/AllowedRecordType.cs

@@ -3,6 +3,9 @@
 
 namespace System.Formats.Nrbf;
 
+// See [MS-NRBF] Sec. 2.7 for more information.
+// https://learn.microsoft.com/openspecs/windows_protocols/ms-nrbf/ca3ad2bc-777b-413a-a72a-9ba6ced76bc3
+
 [Flags]
 internal enum AllowedRecordTypes : uint
 {

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayInfo.cs

@@ -16,7 +16,11 @@ namespace System.Formats.Nrbf;
 [DebuggerDisplay("{ArrayType}, rank={Rank}")]
 internal readonly struct ArrayInfo
 {
-    internal const int MaxArrayLength = 2147483591; // Array.MaxLength
+#if NET8_0_OR_GREATER
+    internal static int MaxArrayLength => Array.MaxLength; // dynamic lookup in case the value changes in a future runtime
+#else
+    internal const int MaxArrayLength = 2147483591; // hardcode legacy Array.MaxLength for downlevel runtimes
+#endif
 
     internal ArrayInfo(SerializationRecordId id, long totalElementsCount, BinaryArrayType arrayType = BinaryArrayType.Single, int rank = 1)
     {
@@ -47,7 +51,7 @@ internal static int ParseValidArrayLength(BinaryReader reader)
     {
         int length = reader.ReadInt32();
 
-        if (length is < 0 or > MaxArrayLength)
+        if (length < 0 || length > MaxArrayLength)
         {
             ThrowHelper.ThrowInvalidValue(length);
         }

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayOfClassesRecord.cs

@@ -4,6 +4,7 @@
 using System.Collections.Generic;
 using System.Reflection.Metadata;
 using System.Formats.Nrbf.Utils;
+using System.Diagnostics;
 
 namespace System.Formats.Nrbf;
 
@@ -54,6 +55,7 @@ public override TypeName TypeName
                 }
 
                 int nullCount = ((NullsRecord)actual).NullCount;
+                Debug.Assert(nullCount > 0, "All implementations of NullsRecord are expected to return a positive value for NullCount.");
                 do
                 {
                     result[resultIndex++] = null;
@@ -63,6 +65,8 @@ public override TypeName TypeName
             }
         }
 
+        Debug.Assert(resultIndex == result.Length, "We should have traversed the entirety of the newly created array.");
+
         return result;
     }
 

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleObjectRecord.cs

@@ -5,6 +5,7 @@
 using System.IO;
 using System.Reflection.Metadata;
 using System.Formats.Nrbf.Utils;
+using System.Diagnostics;
 
 namespace System.Formats.Nrbf;
 
@@ -33,13 +34,15 @@ public override TypeName TypeName
     {
         object?[] values = new object?[Length];
 
-        for (int recordIndex = 0, valueIndex = 0; recordIndex < Records.Count; recordIndex++)
+        int valueIndex = 0;
+        for (int recordIndex = 0; recordIndex < Records.Count; recordIndex++)
         {
             SerializationRecord record = Records[recordIndex];
 
             int nullCount = record is NullsRecord nullsRecord ? nullsRecord.NullCount : 0;
             if (nullCount == 0)
             {
+                // "new object[] { <SELF> }" is special cased because it allows for storing reference to itself.
                 values[valueIndex++] = record is MemberReferenceRecord referenceRecord && referenceRecord.Reference.Equals(Id)
                     ? values // a reference to self, and a way to get StackOverflow exception ;)
                     : record.GetValue();
@@ -59,6 +62,8 @@ public override TypeName TypeName
             while (nullCount > 0);
         }
 
+        Debug.Assert(valueIndex == values.Length, "We should have traversed the entirety of the newly created array.");
+
         return values;
     }
 

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySinglePrimitiveRecord.cs

@@ -53,8 +53,32 @@ internal static IReadOnlyList<T> DecodePrimitiveTypes(BinaryReader reader, int c
             return (List<T>)(object)DecodeDecimals(reader, count);
         }
 
+        // char[] has a unique representation in NRBF streams. Typical strings are transcoded
+        // to UTF-8 and prefixed with the number of bytes in the UTF-8 representation. char[]
+        // is also serialized as UTF-8, but it is instead prefixed with the number of chars
+        // in the UTF-16 representation, not the number of bytes in the UTF-8 representation.
+        // This number doesn't directly precede the UTF-8 contents in the NRBF stream; it's
+        // instead contained within the ArrayInfo structure (passed to this method as the
+        // 'count' argument).
+        //
+        // The practical consequence of this is that we don't actually know how many UTF-8
+        // bytes we need to consume in order to ensure we've read 'count' chars. We know that
+        // an n-length UTF-16 string turns into somewhere between [n .. 3n] UTF-8 bytes.
+        // The best we can do is that when reading an n-element char[], we'll ensure that
+        // there are at least n bytes remaining in the input stream. We'll still need to
+        // account for that even with this check, we might hit EOF before fully populating
+        // the char[]. But from a safety perspective, it does appropriately limit our
+        // allocations to be proportional to the amount of data present in the input stream,
+        // which is a sufficient defense against DoS.
+
         long requiredBytes = count;
-        if (typeof(T) != typeof(char)) // the input is UTF8
+        if (typeof(T) == typeof(DateTime) || typeof(T) == typeof(TimeSpan))
+        {
+            // We can't assume DateTime as represented by the runtime is 8 bytes.
+            // The only assumption we can make is that it's 8 bytes on the wire.
+            requiredBytes *= 8;
+        }
+        else if (typeof(T) != typeof(char))
         {
             requiredBytes *= Unsafe.SizeOf<T>();
         }
@@ -79,6 +103,10 @@ internal static IReadOnlyList<T> DecodePrimitiveTypes(BinaryReader reader, int c
         {
             return (T[])(object)reader.ParseChars(count);
         }
+        else if (typeof(T) == typeof(TimeSpan) || typeof(T) == typeof(DateTime))
+        {
+            return DecodeTime(reader, count);
+        }
 
         // It's safe to pre-allocate, as we have ensured there is enough bytes in the stream.
         T[] result = new T[count];
@@ -130,8 +158,7 @@ internal static IReadOnlyList<T> DecodePrimitiveTypes(BinaryReader reader, int c
                 }
 #endif
             }
-            else if (typeof(T) == typeof(long) || typeof(T) == typeof(ulong) || typeof(T) == typeof(double)
-                  || typeof(T) == typeof(DateTime) || typeof(T) == typeof(TimeSpan))
+            else if (typeof(T) == typeof(long) || typeof(T) == typeof(ulong) || typeof(T) == typeof(double))
             {
                 Span<long> span = MemoryMarshal.Cast<T, long>(result);
 #if NET
@@ -145,6 +172,21 @@ internal static IReadOnlyList<T> DecodePrimitiveTypes(BinaryReader reader, int c
             }
         }
 
+        if (typeof(T) == typeof(bool))
+        {
+            // See DontCastBytesToBooleans test to see what could go wrong.
+            bool[] booleans = (bool[])(object)result;
+            resultAsBytes = MemoryMarshal.AsBytes<T>(result);
+            for (int i = 0; i < booleans.Length; i++)
+            {
+                // We don't use the bool array to get the value, as an optimizing compiler or JIT could elide this.
+                if (resultAsBytes[i] != 0) // it can be any byte different than 0
+                {
+                    booleans[i] = true; // set it to 1 in explicit way
+                }
+            }
+        }
+
         return result;
     }
 
@@ -158,8 +200,34 @@ private static List<decimal> DecodeDecimals(BinaryReader reader, int count)
         return values;
     }
 
+    private static T[] DecodeTime(BinaryReader reader, int count)
+    {
+        T[] values = new T[count];
+        for (int i = 0; i < values.Length; i++)
+        {
+            if (typeof(T) == typeof(DateTime))
+            {
+                values[i] = (T)(object)Utils.BinaryReaderExtensions.CreateDateTimeFromData(reader.ReadUInt64());
+            }
+            else if (typeof(T) == typeof(TimeSpan))
+            {
+                values[i] = (T)(object)new TimeSpan(reader.ReadInt64());
+            }
+            else
+            {
+                throw new InvalidOperationException();
+            }
+        }
+
+        return values;
+    }
+
     private static List<T> DecodeFromNonSeekableStream(BinaryReader reader, int count)
     {
+        // The count arg could originate from untrusted input, so we shouldn't
+        // pass it as-is to the ctor's capacity arg. We'll instead rely on
+        // List<T>.Add's O(1) amortization to keep the entire loop O(count).
+
         List<T> values = new List<T>(Math.Min(count, 4));
         for (int i = 0; i < count; i++)
         {

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleStringRecord.cs

@@ -5,6 +5,7 @@
 using System.IO;
 using System.Reflection.Metadata;
 using System.Formats.Nrbf.Utils;
+using System.Diagnostics;
 
 namespace System.Formats.Nrbf;
 
@@ -47,7 +48,8 @@ internal override (AllowedRecordTypes allowed, PrimitiveType primitiveType) GetA
     {
         string?[] values = new string?[Length];
 
-        for (int recordIndex = 0, valueIndex = 0; recordIndex < Records.Count; recordIndex++)
+        int valueIndex = 0;
+        for (int recordIndex = 0; recordIndex < Records.Count; recordIndex++)
         {
             SerializationRecord record = Records[recordIndex];
 
@@ -73,6 +75,7 @@ record = memberReference.GetReferencedRecord();
             }
 
             int nullCount = ((NullsRecord)record).NullCount;
+            Debug.Assert(nullCount > 0, "All implementations of NullsRecord are expected to return a positive value for NullCount.");
             do
             {
                 values[valueIndex++] = null;
@@ -81,6 +84,8 @@ record = memberReference.GetReferencedRecord();
             while (nullCount > 0);
         }
 
+        Debug.Assert(valueIndex == values.Length, "We should have traversed the entirety of the newly created array.");
+
         return values;
     }
 }

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;
 
@@ -84,6 +85,10 @@ private protected override Array Deserialize(Type arrayType, bool allowNulls)
                 case SerializationRecordType.ArraySinglePrimitive:
                 case SerializationRecordType.ArraySingleObject:
                 case SerializationRecordType.ArraySingleString:
+
+                    // Recursion depth is bounded by the depth of arrayType, which is
+                    // a trustworthy Type instance. Don't need to worry about stack overflow.
+
                     ArrayRecord nestedArrayRecord = (ArrayRecord)record;
                     Array nestedArray = nestedArrayRecord.GetArray(actualElementType, allowNulls);
                     array.SetValue(nestedArray, resultIndex++);
@@ -97,6 +102,7 @@ private protected override Array Deserialize(Type arrayType, bool allowNulls)
                     }
 
                     int nullCount = ((NullsRecord)item).NullCount;
+                    Debug.Assert(nullCount > 0, "All implementations of NullsRecord are expected to return a positive value for NullCount.");
                     do
                     {
                         array.SetValue(null, resultIndex++);
@@ -110,6 +116,8 @@ private protected override Array Deserialize(Type arrayType, bool allowNulls)
             }
         }
 
+        Debug.Assert(resultIndex == array.Length, "We should have traversed the entirety of the newly created array.");
+
         return array;
     }
 
@@ -122,6 +130,7 @@ internal static ArrayRecord Decode(BinaryReader reader, RecordMap recordMap, Pay
         bool isRectangular = arrayType is BinaryArrayType.Rectangular;
 
         // It is an arbitrary limit in the current CoreCLR type loader.
+        // Don't change this value without reviewing the loop a few lines below.
         const int MaxSupportedArrayRank = 32;
 
         if (rank < 1 || rank > MaxSupportedArrayRank
@@ -132,18 +141,26 @@ internal static ArrayRecord Decode(BinaryReader reader, RecordMap recordMap, Pay
         }
 
         int[] lengths = new int[rank]; // adversary-controlled, but acceptable since upper limit of 32
-        long totalElementCount = 1;
+        long totalElementCount = 1; // to avoid integer overflow during the multiplication below
         for (int i = 0; i < lengths.Length; i++)
         {
             lengths[i] = ArrayInfo.ParseValidArrayLength(reader);
             totalElementCount *= lengths[i];
 
+            // n.b. This forbids "new T[Array.MaxLength, Array.MaxLength, Array.MaxLength, ..., 0]"
+            // but allows "new T[0, Array.MaxLength, Array.MaxLength, Array.MaxLength, ...]". But
+            // that's the same behavior that newarr and Array.CreateInstance exhibit, so at least
+            // we're consistent.
+
             if (totalElementCount > ArrayInfo.MaxArrayLength)
             {
                 ThrowHelper.ThrowInvalidValue(lengths[i]); // max array size exceeded
             }
         }
 
+        // Per BinaryReaderExtensions.ReadArrayType, we do not support nonzero offsets, so
+        // we don't need to read the NRBF stream 'LowerBounds' field here.
+
         MemberTypeInfo memberTypeInfo = MemberTypeInfo.Decode(reader, 1, options, recordMap);
         ArrayInfo arrayInfo = new(objectId, totalElementCount, arrayType, rank);
 
@@ -186,6 +203,9 @@ private static Type MapElementType(Type arrayType, out bool isClassRecord)
         Type elementType = arrayType;
         int arrayNestingDepth = 0;
 
+        // Loop iteration counts are bound by the nesting depth of arrayType,
+        // which is a trustworthy input. No DoS concerns.
+
         while (elementType.IsArray)
         {
             elementType = elementType.GetElementType()!;

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ClassInfo.cs

@@ -50,7 +50,8 @@ internal static ClassInfo Decode(BinaryReader reader)
 
         // Use Dictionary instead of List so that searching for member IDs by name
         // is O(n) instead of O(m * n), where m = memberCount and n = memberNameLength,
-        // in degenerate cases.
+        // in degenerate cases. Since memberCount may be hostile, don't allow it to be
+        // used as the initial capacity in the collection instance.
         Dictionary<string, int> memberNames = new(StringComparer.Ordinal);
         for (int i = 0; i < memberCount; i++)
         {

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ClassTypeInfo.cs

@@ -9,7 +9,7 @@
 namespace System.Formats.Nrbf;
 
 /// <summary>
-/// Identifies a class by it's name and library id.
+/// Identifies a class by its name and library id.
 /// </summary>
 /// <remarks>
 /// ClassTypeInfo structures are described in <see href="https://learn.microsoft.com/openspecs/windows_protocols/ms-nrbf/844b24dd-9f82-426e-9b98-05334307a239">[MS-NRBF] 2.1.1.8</see>.

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs

@@ -110,7 +110,7 @@ internal bool ShouldBeRepresentedAsArrayOfClassRecords()
     {
         // This library tries to minimize the number of concepts the users need to learn to use it.
         // Since SZArrays are most common, it provides an SZArrayRecord<T> abstraction.
-        // Every other array (jagged, multi-dimensional etc) is represented using SZArrayRecord.
+        // Every other array (jagged, multi-dimensional etc) is represented using ArrayRecord.
         // The goal of this method is to determine whether given array can be represented as SZArrayRecord<ClassRecord>.
 
         (BinaryType binaryType, object? additionalInfo) = Infos[0];

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/NextInfo.cs

@@ -27,7 +27,5 @@ internal NextInfo(AllowedRecordTypes allowed, SerializationRecord parent,
     internal PrimitiveType PrimitiveType { get; }
 
     internal NextInfo With(AllowedRecordTypes allowed, PrimitiveType primitiveType)
-        => allowed == Allowed && primitiveType == PrimitiveType
-            ? this // previous record was of the same type
-            : new(allowed, Parent, Stack, primitiveType);
+        => new(allowed, Parent, Stack, primitiveType);
 }

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/NrbfDecoder.cs

@@ -22,7 +22,7 @@ public static class NrbfDecoder
     // The header consists of:
     // - a byte that describes the record type (SerializationRecordType.SerializedStreamHeader)
     // - four 32 bit integers:
-    //   - root Id (every value is valid)
+    //   - root Id (every value except of 0 is valid)
     //   - header Id (value is ignored)
     //   - major version, it has to be equal 1.
     //   - minor version, it has to be equal 0.

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/PayloadOptions.cs

@@ -25,10 +25,17 @@ public PayloadOptions() { }
     /// </summary>
     /// <value><see langword="true" /> if truncated type names should be reassembled; otherwise, <see langword="false" />.</value>
     /// <remarks>
+    /// <para>
     /// Example:
     /// TypeName: "Namespace.TypeName`1[[Namespace.GenericArgName"
     /// LibraryName: "AssemblyName]]"
     /// Is combined into "Namespace.TypeName`1[[Namespace.GenericArgName, AssemblyName]]"
+    /// </para>
+    /// <para>
+    /// Setting this to <see langword="true" /> can render <see cref="NrbfDecoder"/> susceptible to Denial of Service
+    /// attacks when parsing or handling malicious input.
+    /// </para>
+    /// <para>The default value is <see langword="false" />.</para>
     /// </remarks>
     public bool UndoTruncatedTypeNames { get; set; }
 }