Skip to content

Commit 0633a2d

Browse files
adamsitniksirntar
adamsitnik
authored and
sirntar
committed
[NRBF] More bug fixes (dotnet#107682)
- Don't use `Debug.Fail` not followed by an exception (it may cause problems for apps deployed in Debug) - avoid Int32 overflow - throw for unexpected enum values just in case parsing has not rejected them - validate the number of chars read by BinaryReader.ReadChars - pass serialization record id to ex message - return false rather than throw EndOfStreamException when provided Stream has not enough data - don't restore the position in finally - limit max SZ and MD array length to Array.MaxLength, stop using LinkedList<T> as List<T> will be able to hold all elements now - remove internal enum values that were always illegal, but needed to be handled everywhere - Fix DebuggerDisplay
1 parent ac7ac2f commit 0633a2d

17 files changed

+97
-136
lines changed

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ namespace System.Formats.Nrbf;
1313
/// <remarks>
1414
/// ArrayInfo structures are described in <see href="https://learn.microsoft.com/openspecs/windows_protocols/ms-nrbf/8fac763f-e46d-43a1-b360-80eb83d2c5fb">[MS-NRBF] 2.4.2.1</see>.
1515
/// </remarks>
16-
[DebuggerDisplay("Length={Length}, {ArrayType}, rank={Rank}")]
16+
[DebuggerDisplay("{ArrayType}, rank={Rank}")]
1717
internal readonly struct ArrayInfo
1818
{
1919
internal const int MaxArrayLength = 2147483591; // Array.MaxLength

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

+8-35
Original file line numberDiff line numberDiff line change
@@ -41,17 +41,9 @@ internal ArraySinglePrimitiveRecord(ArrayInfo arrayInfo, IReadOnlyList<T> values
4141
public override T[] GetArray(bool allowNulls = true)
4242
=> (T[])(_arrayNullsNotAllowed ??= (Values is T[] array ? array : Values.ToArray()));
4343

44-
internal override (AllowedRecordTypes allowed, PrimitiveType primitiveType) GetAllowedRecordType()
45-
{
46-
Debug.Fail("GetAllowedRecordType should never be called on ArraySinglePrimitiveRecord");
47-
throw new InvalidOperationException();
48-
}
44+
internal override (AllowedRecordTypes allowed, PrimitiveType primitiveType) GetAllowedRecordType() => throw new InvalidOperationException();
4945

50-
private protected override void AddValue(object value)
51-
{
52-
Debug.Fail("AddValue should never be called on ArraySinglePrimitiveRecord");
53-
throw new InvalidOperationException();
54-
}
46+
private protected override void AddValue(object value) => throw new InvalidOperationException();
5547

5648
internal static IReadOnlyList<T> DecodePrimitiveTypes(BinaryReader reader, int count)
5749
{
@@ -94,7 +86,7 @@ internal static IReadOnlyList<T> DecodePrimitiveTypes(BinaryReader reader, int c
9486
#if NET
9587
reader.BaseStream.ReadExactly(resultAsBytes);
9688
#else
97-
byte[] bytes = ArrayPool<byte>.Shared.Rent(Math.Min(count * Unsafe.SizeOf<T>(), 256_000));
89+
byte[] bytes = ArrayPool<byte>.Shared.Rent((int)Math.Min(requiredBytes, 256_000));
9890

9991
while (!resultAsBytes.IsEmpty)
10092
{
@@ -159,31 +151,10 @@ internal static IReadOnlyList<T> DecodePrimitiveTypes(BinaryReader reader, int c
159151
private static List<decimal> DecodeDecimals(BinaryReader reader, int count)
160152
{
161153
List<decimal> values = new();
162-
#if NET
163-
Span<byte> buffer = stackalloc byte[256];
164-
for (int i = 0; i < count; i++)
165-
{
166-
int stringLength = reader.Read7BitEncodedInt();
167-
if (!(stringLength > 0 && stringLength <= buffer.Length))
168-
{
169-
ThrowHelper.ThrowInvalidValue(stringLength);
170-
}
171-
172-
reader.BaseStream.ReadExactly(buffer.Slice(0, stringLength));
173-
174-
if (!decimal.TryParse(buffer.Slice(0, stringLength), NumberStyles.Number, CultureInfo.InvariantCulture, out decimal value))
175-
{
176-
ThrowHelper.ThrowInvalidFormat();
177-
}
178-
179-
values.Add(value);
180-
}
181-
#else
182154
for (int i = 0; i < count; i++)
183155
{
184156
values.Add(reader.ParseDecimal());
185157
}
186-
#endif
187158
return values;
188159
}
189160

@@ -244,12 +215,14 @@ private static List<T> DecodeFromNonSeekableStream(BinaryReader reader, int coun
244215
{
245216
values.Add((T)(object)Utils.BinaryReaderExtensions.CreateDateTimeFromData(reader.ReadUInt64()));
246217
}
247-
else
218+
else if (typeof(T) == typeof(TimeSpan))
248219
{
249-
Debug.Assert(typeof(T) == typeof(TimeSpan));
250-
251220
values.Add((T)(object)new TimeSpan(reader.ReadInt64()));
252221
}
222+
else
223+
{
224+
throw new InvalidOperationException();
225+
}
253226
}
254227

255228
return values;

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ internal sealed class ArraySingleStringRecord : SZArrayRecord<string?>
2121
public override SerializationRecordType RecordType => SerializationRecordType.ArraySingleString;
2222

2323
/// <inheritdoc />
24-
public override TypeName TypeName => TypeNameHelpers.GetPrimitiveSZArrayTypeName(PrimitiveType.String);
24+
public override TypeName TypeName => TypeNameHelpers.GetPrimitiveSZArrayTypeName(TypeNameHelpers.StringPrimitiveType);
2525

2626
private List<SerializationRecord> Records { get; }
2727

‎src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs

+1-1
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ internal static ArrayRecord Decode(BinaryReader reader, RecordMap recordMap, Pay
138138
lengths[i] = ArrayInfo.ParseValidArrayLength(reader);
139139
totalElementCount *= lengths[i];
140140

141-
if (totalElementCount > uint.MaxValue)
141+
if (totalElementCount > ArrayInfo.MaxArrayLength)
142142
{
143143
ThrowHelper.ThrowInvalidValue(lengths[i]); // max array size exceeded
144144
}

‎src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryLibraryRecord.cs

+1-8
Original file line numberDiff line numberDiff line change
@@ -30,14 +30,7 @@ private BinaryLibraryRecord(SerializationRecordId libraryId, AssemblyNameInfo li
3030

3131
public override SerializationRecordType RecordType => SerializationRecordType.BinaryLibrary;
3232

33-
public override TypeName TypeName
34-
{
35-
get
36-
{
37-
Debug.Fail("TypeName should never be called on BinaryLibraryRecord");
38-
return TypeName.Parse(nameof(BinaryLibraryRecord).AsSpan());
39-
}
40-
}
33+
public override TypeName TypeName => TypeName.Parse(nameof(BinaryLibraryRecord).AsSpan());
4134

4235
internal string? RawLibraryName { get; }
4336

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

+12-7
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,14 @@ internal static MemberTypeInfo Decode(BinaryReader reader, int count, PayloadOpt
5353
case BinaryType.Class:
5454
info[i] = (type, ClassTypeInfo.Decode(reader, options, recordMap));
5555
break;
56-
default:
57-
// Other types have no additional data.
58-
Debug.Assert(type is BinaryType.String or BinaryType.ObjectArray or BinaryType.StringArray or BinaryType.Object);
56+
case BinaryType.String:
57+
case BinaryType.StringArray:
58+
case BinaryType.Object:
59+
case BinaryType.ObjectArray:
60+
// These types have no additional data.
5961
break;
62+
default:
63+
throw new InvalidOperationException();
6064
}
6165
}
6266

@@ -97,7 +101,8 @@ internal static MemberTypeInfo Decode(BinaryReader reader, int count, PayloadOpt
97101
BinaryType.PrimitiveArray => (PrimitiveArray, default),
98102
BinaryType.Class => (NonSystemClass, default),
99103
BinaryType.SystemClass => (SystemClass, default),
100-
_ => (ObjectArray, default)
104+
BinaryType.ObjectArray => (ObjectArray, default),
105+
_ => throw new InvalidOperationException()
101106
};
102107
}
103108

@@ -144,15 +149,15 @@ internal TypeName GetArrayTypeName(ArrayInfo arrayInfo)
144149

145150
TypeName elementTypeName = binaryType switch
146151
{
147-
BinaryType.String => TypeNameHelpers.GetPrimitiveTypeName(PrimitiveType.String),
148-
BinaryType.StringArray => TypeNameHelpers.GetPrimitiveSZArrayTypeName(PrimitiveType.String),
152+
BinaryType.String => TypeNameHelpers.GetPrimitiveTypeName(TypeNameHelpers.StringPrimitiveType),
153+
BinaryType.StringArray => TypeNameHelpers.GetPrimitiveSZArrayTypeName(TypeNameHelpers.StringPrimitiveType),
149154
BinaryType.Primitive => TypeNameHelpers.GetPrimitiveTypeName((PrimitiveType)additionalInfo!),
150155
BinaryType.PrimitiveArray => TypeNameHelpers.GetPrimitiveSZArrayTypeName((PrimitiveType)additionalInfo!),
151156
BinaryType.Object => TypeNameHelpers.GetPrimitiveTypeName(TypeNameHelpers.ObjectPrimitiveType),
152157
BinaryType.ObjectArray => TypeNameHelpers.GetPrimitiveSZArrayTypeName(TypeNameHelpers.ObjectPrimitiveType),
153158
BinaryType.SystemClass => (TypeName)additionalInfo!,
154159
BinaryType.Class => ((ClassTypeInfo)additionalInfo!).TypeName,
155-
_ => throw new ArgumentOutOfRangeException(paramName: nameof(binaryType), actualValue: binaryType, message: null)
160+
_ => throw new InvalidOperationException()
156161
};
157162

158163
// In general, arrayRank == 1 may have two different meanings:

‎src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MessageEndRecord.cs

+1-8
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,5 @@ private MessageEndRecord()
2424

2525
public override SerializationRecordId Id => SerializationRecordId.NoId;
2626

27-
public override TypeName TypeName
28-
{
29-
get
30-
{
31-
Debug.Fail("TypeName should never be called on MessageEndRecord");
32-
return TypeName.Parse(nameof(MessageEndRecord).AsSpan());
33-
}
34-
}
27+
public override TypeName TypeName => TypeName.Parse(nameof(MessageEndRecord).AsSpan());
3528
}

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

+18-22
Original file line numberDiff line numberDiff line change
@@ -69,28 +69,22 @@ public static bool StartsWithPayloadHeader(Stream stream)
6969
return false;
7070
}
7171

72-
try
72+
byte[] buffer = new byte[SerializedStreamHeaderRecord.Size];
73+
int offset = 0;
74+
while (offset < buffer.Length)
7375
{
74-
#if NET
75-
Span<byte> buffer = stackalloc byte[SerializedStreamHeaderRecord.Size];
76-
stream.ReadExactly(buffer);
77-
#else
78-
byte[] buffer = new byte[SerializedStreamHeaderRecord.Size];
79-
int offset = 0;
80-
while (offset < buffer.Length)
76+
int read = stream.Read(buffer, offset, buffer.Length - offset);
77+
if (read == 0)
8178
{
82-
int read = stream.Read(buffer, offset, buffer.Length - offset);
83-
if (read == 0)
84-
throw new EndOfStreamException();
85-
offset += read;
79+
stream.Position = beginning;
80+
return false;
8681
}
87-
#endif
88-
return StartsWithPayloadHeader(buffer);
89-
}
90-
finally
91-
{
92-
stream.Position = beginning;
82+
offset += read;
9383
}
84+
85+
bool result = StartsWithPayloadHeader(buffer);
86+
stream.Position = beginning;
87+
return result;
9488
}
9589

9690
/// <summary>
@@ -241,7 +235,8 @@ private static SerializationRecord DecodeNext(BinaryReader reader, RecordMap rec
241235
SerializationRecordType.ObjectNullMultiple => ObjectNullMultipleRecord.Decode(reader),
242236
SerializationRecordType.ObjectNullMultiple256 => ObjectNullMultiple256Record.Decode(reader),
243237
SerializationRecordType.SerializedStreamHeader => SerializedStreamHeaderRecord.Decode(reader),
244-
_ => SystemClassWithMembersAndTypesRecord.Decode(reader, recordMap, options),
238+
SerializationRecordType.SystemClassWithMembersAndTypes => SystemClassWithMembersAndTypesRecord.Decode(reader, recordMap, options),
239+
_ => throw new InvalidOperationException()
245240
};
246241

247242
recordMap.Add(record);
@@ -269,8 +264,8 @@ private static SerializationRecord DecodeMemberPrimitiveTypedRecord(BinaryReader
269264
PrimitiveType.Double => new MemberPrimitiveTypedRecord<double>(reader.ReadDouble()),
270265
PrimitiveType.Decimal => new MemberPrimitiveTypedRecord<decimal>(reader.ParseDecimal()),
271266
PrimitiveType.DateTime => new MemberPrimitiveTypedRecord<DateTime>(Utils.BinaryReaderExtensions.CreateDateTimeFromData(reader.ReadUInt64())),
272-
// String is handled with a record, never on it's own
273-
_ => new MemberPrimitiveTypedRecord<TimeSpan>(new TimeSpan(reader.ReadInt64())),
267+
PrimitiveType.TimeSpan => new MemberPrimitiveTypedRecord<TimeSpan>(new TimeSpan(reader.ReadInt64())),
268+
_ => throw new InvalidOperationException()
274269
};
275270
}
276271

@@ -295,7 +290,8 @@ private static SerializationRecord DecodeArraySinglePrimitiveRecord(BinaryReader
295290
PrimitiveType.Double => Decode<double>(info, reader),
296291
PrimitiveType.Decimal => Decode<decimal>(info, reader),
297292
PrimitiveType.DateTime => Decode<DateTime>(info, reader),
298-
_ => Decode<TimeSpan>(info, reader),
293+
PrimitiveType.TimeSpan => Decode<TimeSpan>(info, reader),
294+
_ => throw new InvalidOperationException()
299295
};
300296

301297
static SerializationRecord Decode<T>(ArrayInfo info, BinaryReader reader) where T : unmanaged

‎src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/NullsRecord.cs

+1-8
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,5 @@ internal abstract class NullsRecord : SerializationRecord
1212

1313
public override SerializationRecordId Id => SerializationRecordId.NoId;
1414

15-
public override TypeName TypeName
16-
{
17-
get
18-
{
19-
Debug.Fail($"TypeName should never be called on {GetType().Name}");
20-
return TypeName.Parse(GetType().Name.AsSpan());
21-
}
22-
}
15+
public override TypeName TypeName => TypeName.Parse(GetType().Name.AsSpan());
2316
}

‎src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/PrimitiveType.cs

+15-7
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,6 @@ namespace System.Formats.Nrbf;
1111
/// </remarks>
1212
internal enum PrimitiveType : byte
1313
{
14-
/// <summary>
15-
/// Used internally to express no value
16-
/// </summary>
17-
None = 0,
1814
Boolean = 1,
1915
Byte = 2,
2016
Char = 3,
@@ -30,7 +26,19 @@ internal enum PrimitiveType : byte
3026
DateTime = 13,
3127
UInt16 = 14,
3228
UInt32 = 15,
33-
UInt64 = 16,
34-
Null = 17,
35-
String = 18
29+
UInt64 = 16
30+
// This internal enum no longer contains Null and String as they were always illegal:
31+
// - In case of BinaryArray (NRBF 2.4.3.1):
32+
// "If the BinaryTypeEnum value is Primitive, the PrimitiveTypeEnumeration
33+
// value in AdditionalTypeInfo MUST NOT be Null (17) or String (18)."
34+
// - In case of MemberPrimitiveTyped (NRBF 2.5.1):
35+
// "PrimitiveTypeEnum (1 byte): A PrimitiveTypeEnumeration
36+
// value that specifies the Primitive Type of data that is being transmitted.
37+
// This field MUST NOT contain a value of 17 (Null) or 18 (String)."
38+
// - In case of ArraySinglePrimitive (NRBF 2.4.3.3):
39+
// "A PrimitiveTypeEnumeration value that identifies the Primitive Type
40+
// of the items of the Array. The value MUST NOT be 17 (Null) or 18 (String)."
41+
// - In case of MemberTypeInfo (NRBF 2.3.1.2):
42+
// "When the BinaryTypeEnum value is Primitive, the PrimitiveTypeEnumeration
43+
// value in AdditionalInfo MUST NOT be Null (17) or String (18)."
3644
}

‎src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/RecordMap.cs

+1-1
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ internal void Add(SerializationRecord record)
5656
return;
5757
}
5858
#endif
59-
throw new SerializationException(SR.Format(SR.Serialization_DuplicateSerializationRecordId, record.Id));
59+
throw new SerializationException(SR.Format(SR.Serialization_DuplicateSerializationRecordId, record.Id._id));
6060
}
6161
}
6262
}

‎src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/RectangularArrayRecord.cs

+13-17
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ namespace System.Formats.Nrbf;
1414
internal sealed class RectangularArrayRecord : ArrayRecord
1515
{
1616
private readonly int[] _lengths;
17-
private readonly ICollection<object> _values;
17+
private readonly List<object> _values;
1818
private TypeName? _typeName;
1919

2020
private RectangularArrayRecord(Type elementType, ArrayInfo arrayInfo,
@@ -24,18 +24,8 @@ private RectangularArrayRecord(Type elementType, ArrayInfo arrayInfo,
2424
MemberTypeInfo = memberTypeInfo;
2525
_lengths = lengths;
2626

27-
// A List<T> can hold as many objects as an array, so for multi-dimensional arrays
28-
// with more elements than Array.MaxLength we use LinkedList.
29-
// Testing that many elements takes a LOT of time, so to ensure that both code paths are tested,
30-
// we always use LinkedList code path for Debug builds.
31-
#if DEBUG
32-
_values = new LinkedList<object>();
33-
#else
34-
_values = arrayInfo.TotalElementsCount <= ArrayInfo.MaxArrayLength
35-
? new List<object>(canPreAllocate ? arrayInfo.GetSZArrayLength() : Math.Min(4, arrayInfo.GetSZArrayLength()))
36-
: new LinkedList<object>();
37-
#endif
38-
27+
// ArrayInfo.GetSZArrayLength ensures to return a value <= Array.MaxLength
28+
_values = new List<object>(canPreAllocate ? arrayInfo.GetSZArrayLength() : Math.Min(4, arrayInfo.GetSZArrayLength()));
3929
}
4030

4131
public override SerializationRecordType RecordType => SerializationRecordType.BinaryArray;
@@ -108,6 +98,7 @@ private protected override Array Deserialize(Type arrayType, bool allowNulls)
10898
else if (ElementType == typeof(TimeSpan)) CopyTo<TimeSpan>(_values, result);
10999
else if (ElementType == typeof(DateTime)) CopyTo<DateTime>(_values, result);
110100
else if (ElementType == typeof(decimal)) CopyTo<decimal>(_values, result);
101+
else throw new InvalidOperationException();
111102
}
112103
else
113104
{
@@ -116,7 +107,7 @@ private protected override Array Deserialize(Type arrayType, bool allowNulls)
116107

117108
return result;
118109

119-
static void CopyTo<T>(ICollection<object> list, Array array)
110+
static void CopyTo<T>(List<object> list, Array array)
120111
{
121112
ref byte arrayDataRef = ref MemoryMarshal.GetArrayDataReference(array);
122113
ref T firstElementRef = ref Unsafe.As<byte, T>(ref arrayDataRef);
@@ -176,7 +167,10 @@ internal static RectangularArrayRecord Create(BinaryReader reader, ArrayInfo arr
176167
PrimitiveType.Int64 => sizeof(long),
177168
PrimitiveType.UInt64 => sizeof(ulong),
178169
PrimitiveType.Double => sizeof(double),
179-
_ => -1
170+
PrimitiveType.TimeSpan => sizeof(ulong),
171+
PrimitiveType.DateTime => sizeof(ulong),
172+
PrimitiveType.Decimal => -1, // represented as variable-length string
173+
_ => throw new InvalidOperationException()
180174
};
181175

182176
if (sizeOfSingleValue > 0)
@@ -215,7 +209,8 @@ private static Type MapPrimitive(PrimitiveType primitiveType)
215209
PrimitiveType.DateTime => typeof(DateTime),
216210
PrimitiveType.UInt16 => typeof(ushort),
217211
PrimitiveType.UInt32 => typeof(uint),
218-
_ => typeof(ulong)
212+
PrimitiveType.UInt64 => typeof(ulong),
213+
_ => throw new InvalidOperationException()
219214
};
220215

221216
private static Type MapPrimitiveArray(PrimitiveType primitiveType)
@@ -235,7 +230,8 @@ private static Type MapPrimitiveArray(PrimitiveType primitiveType)
235230
PrimitiveType.DateTime => typeof(DateTime[]),
236231
PrimitiveType.UInt16 => typeof(ushort[]),
237232
PrimitiveType.UInt32 => typeof(uint[]),
238-
_ => typeof(ulong[]),
233+
PrimitiveType.UInt64 => typeof(ulong[]),
234+
_ => throw new InvalidOperationException()
239235
};
240236

241237
private static object? GetActualValue(object value)

0 commit comments

Comments
 (0)