Skip to content

Commit 62526e0

Browse files
adamsitniksirntar
authored andcommitted
[NRBF] Fix bugs discovered by the fuzzer (dotnet#107368)
* bug #1: don't allow for values out of the SerializationRecordType enum range * bug #2: throw SerializationException rather than KeyNotFoundException when the referenced record is missing or it points to a record of different type * bug #3: throw SerializationException rather than FormatException when it's being thrown by BinaryReader (or sth else that we use) * bug dotnet#4: document the fact that IOException can be thrown * bug dotnet#5: throw SerializationException rather than OverflowException when parsing the decimal fails * bug dotnet#6: 0 and 17 are illegal values for PrimitiveType enum * bug dotnet#7: throw SerializationException when a surrogate character is read (so far an ArgumentException was thrown)
1 parent 31cd4e9 commit 62526e0

13 files changed

+197
-25
lines changed

src/libraries/System.Formats.Nrbf/src/Resources/Strings.resx

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@
133133
<value>{0} Record Type is not supported by design.</value>
134134
</data>
135135
<data name="Serialization_InvalidReference" xml:space="preserve">
136-
<value>Member reference was pointing to a record of unexpected type.</value>
136+
<value>Invalid member reference.</value>
137137
</data>
138138
<data name="Serialization_InvalidTypeName" xml:space="preserve">
139139
<value>Invalid type name: `{0}`.</value>
@@ -162,4 +162,10 @@
162162
<data name="Serialization_InvalidAssemblyName" xml:space="preserve">
163163
<value>Invalid assembly name: `{0}`.</value>
164164
</data>
165+
<data name="Serialization_InvalidFormat" xml:space="preserve">
166+
<value>Invalid format.</value>
167+
</data>
168+
<data name="Serialization_SurrogateCharacter" xml:space="preserve">
169+
<value>A surrogate character was read.</value>
170+
</data>
165171
</root>

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -171,12 +171,17 @@ private static List<decimal> DecodeDecimals(BinaryReader reader, int count)
171171

172172
reader.BaseStream.ReadExactly(buffer.Slice(0, stringLength));
173173

174-
values.Add(decimal.Parse(buffer.Slice(0, stringLength), CultureInfo.InvariantCulture));
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);
175180
}
176181
#else
177182
for (int i = 0; i < count; i++)
178183
{
179-
values.Add(decimal.Parse(reader.ReadString(), CultureInfo.InvariantCulture));
184+
values.Add(reader.ParseDecimal());
180185
}
181186
#endif
182187
return values;

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ internal static ClassTypeInfo Decode(BinaryReader reader, PayloadOptions options
2626
string rawName = reader.ReadString();
2727
SerializationRecordId libraryId = SerializationRecordId.Decode(reader);
2828

29-
BinaryLibraryRecord library = (BinaryLibraryRecord)recordMap[libraryId];
29+
BinaryLibraryRecord library = recordMap.GetRecord<BinaryLibraryRecord>(libraryId);
3030

3131
return new ClassTypeInfo(rawName.ParseNonSystemClassRecordTypeName(library, options));
3232
}

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

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,7 @@ internal static ClassWithIdRecord Decode(
3434
SerializationRecordId id = SerializationRecordId.Decode(reader);
3535
SerializationRecordId metadataId = SerializationRecordId.Decode(reader);
3636

37-
if (recordMap[metadataId] is not ClassRecord referencedRecord)
38-
{
39-
throw new SerializationException(SR.Serialization_InvalidReference);
40-
}
37+
ClassRecord referencedRecord = recordMap.GetRecord<ClassRecord>(metadataId);
4138

4239
return new ClassWithIdRecord(id, referencedRecord);
4340
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ internal static ClassWithMembersAndTypesRecord Decode(BinaryReader reader, Recor
2727
MemberTypeInfo memberTypeInfo = MemberTypeInfo.Decode(reader, classInfo.MemberNames.Count, options, recordMap);
2828
SerializationRecordId libraryId = SerializationRecordId.Decode(reader);
2929

30-
BinaryLibraryRecord library = (BinaryLibraryRecord)recordMap[libraryId];
30+
BinaryLibraryRecord library = recordMap.GetRecord<BinaryLibraryRecord>(libraryId);
3131
classInfo.LoadTypeName(library, options);
3232

3333
return new ClassWithMembersAndTypesRecord(classInfo, memberTypeInfo);

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,5 +38,5 @@ private MemberReferenceRecord(SerializationRecordId reference, RecordMap recordM
3838
internal static MemberReferenceRecord Decode(BinaryReader reader, RecordMap recordMap)
3939
=> new(SerializationRecordId.Decode(reader), recordMap);
4040

41-
internal SerializationRecord GetReferencedRecord() => RecordMap[Reference];
41+
internal SerializationRecord GetReferencedRecord() => RecordMap.GetRecord(Reference);
4242
}

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

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ public static bool StartsWithPayloadHeader(ReadOnlySpan<byte> bytes)
4646
/// <exception cref="ArgumentNullException"><paramref name="stream" /> is <see langword="null" />.</exception>
4747
/// <exception cref="NotSupportedException">The stream does not support reading or seeking.</exception>
4848
/// <exception cref="ObjectDisposedException">The stream was closed.</exception>
49+
/// <exception cref="IOException">An I/O error occurred.</exception>
4950
/// <remarks>When this method returns, <paramref name="stream" /> is restored to its original position.</remarks>
5051
public static bool StartsWithPayloadHeader(Stream stream)
5152
{
@@ -107,6 +108,7 @@ public static bool StartsWithPayloadHeader(Stream stream)
107108
/// <exception cref="ArgumentNullException"><paramref name="payload"/> is <see langword="null" />.</exception>
108109
/// <exception cref="ArgumentException"><paramref name="payload"/> does not support reading or is already closed.</exception>
109110
/// <exception cref="SerializationException">Reading from <paramref name="payload"/> encountered invalid NRBF data.</exception>
111+
/// <exception cref="IOException">An I/O error occurred.</exception>
110112
/// <exception cref="NotSupportedException">
111113
/// Reading from <paramref name="payload"/> encountered unsupported records,
112114
/// for example, arrays with non-zero offset or unsupported record types
@@ -142,7 +144,14 @@ public static SerializationRecord Decode(Stream payload, out IReadOnlyDictionary
142144
#endif
143145

144146
using BinaryReader reader = new(payload, ThrowOnInvalidUtf8Encoding, leaveOpen: leaveOpen);
145-
return Decode(reader, options ?? new(), out recordMap);
147+
try
148+
{
149+
return Decode(reader, options ?? new(), out recordMap);
150+
}
151+
catch (FormatException) // can be thrown by various BinaryReader methods
152+
{
153+
throw new SerializationException(SR.Serialization_InvalidFormat);
154+
}
146155
}
147156

148157
/// <summary>
@@ -213,12 +222,7 @@ private static SerializationRecord Decode(BinaryReader reader, PayloadOptions op
213222
private static SerializationRecord DecodeNext(BinaryReader reader, RecordMap recordMap,
214223
AllowedRecordTypes allowed, PayloadOptions options, out SerializationRecordType recordType)
215224
{
216-
byte nextByte = reader.ReadByte();
217-
if (((uint)allowed & (1u << nextByte)) == 0)
218-
{
219-
ThrowHelper.ThrowForUnexpectedRecordType(nextByte);
220-
}
221-
recordType = (SerializationRecordType)nextByte;
225+
recordType = reader.ReadSerializationRecordType(allowed);
222226

223227
SerializationRecord record = recordType switch
224228
{
@@ -254,7 +258,7 @@ private static SerializationRecord DecodeMemberPrimitiveTypedRecord(BinaryReader
254258
PrimitiveType.Boolean => new MemberPrimitiveTypedRecord<bool>(reader.ReadBoolean()),
255259
PrimitiveType.Byte => new MemberPrimitiveTypedRecord<byte>(reader.ReadByte()),
256260
PrimitiveType.SByte => new MemberPrimitiveTypedRecord<sbyte>(reader.ReadSByte()),
257-
PrimitiveType.Char => new MemberPrimitiveTypedRecord<char>(reader.ReadChar()),
261+
PrimitiveType.Char => new MemberPrimitiveTypedRecord<char>(reader.ParseChar()),
258262
PrimitiveType.Int16 => new MemberPrimitiveTypedRecord<short>(reader.ReadInt16()),
259263
PrimitiveType.UInt16 => new MemberPrimitiveTypedRecord<ushort>(reader.ReadUInt16()),
260264
PrimitiveType.Int32 => new MemberPrimitiveTypedRecord<int>(reader.ReadInt32()),
@@ -263,7 +267,7 @@ private static SerializationRecord DecodeMemberPrimitiveTypedRecord(BinaryReader
263267
PrimitiveType.UInt64 => new MemberPrimitiveTypedRecord<ulong>(reader.ReadUInt64()),
264268
PrimitiveType.Single => new MemberPrimitiveTypedRecord<float>(reader.ReadSingle()),
265269
PrimitiveType.Double => new MemberPrimitiveTypedRecord<double>(reader.ReadDouble()),
266-
PrimitiveType.Decimal => new MemberPrimitiveTypedRecord<decimal>(decimal.Parse(reader.ReadString(), CultureInfo.InvariantCulture)),
270+
PrimitiveType.Decimal => new MemberPrimitiveTypedRecord<decimal>(reader.ParseDecimal()),
267271
PrimitiveType.DateTime => new MemberPrimitiveTypedRecord<DateTime>(Utils.BinaryReaderExtensions.CreateDateTimeFromData(reader.ReadUInt64())),
268272
// String is handled with a record, never on it's own
269273
_ => new MemberPrimitiveTypedRecord<TimeSpan>(new TimeSpan(reader.ReadInt64())),

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,8 @@ internal void Add(SerializationRecord record)
6363

6464
internal SerializationRecord GetRootRecord(SerializedStreamHeaderRecord header)
6565
{
66-
SerializationRecord rootRecord = _map[header.RootId];
66+
SerializationRecord rootRecord = GetRecord(header.RootId);
67+
6768
if (rootRecord is SystemClassWithMembersAndTypesRecord systemClass)
6869
{
6970
// update the record map, so it's visible also to those who access it via Id
@@ -72,4 +73,14 @@ internal SerializationRecord GetRootRecord(SerializedStreamHeaderRecord header)
7273

7374
return rootRecord;
7475
}
76+
77+
internal SerializationRecord GetRecord(SerializationRecordId recordId)
78+
=> _map.TryGetValue(recordId, out SerializationRecord? record)
79+
? record
80+
: throw new SerializationException(SR.Serialization_InvalidReference);
81+
82+
internal T GetRecord<T>(SerializationRecordId recordId) where T : SerializationRecord
83+
=> _map.TryGetValue(recordId, out SerializationRecord? record) && record is T casted
84+
? casted
85+
: throw new SerializationException(SR.Serialization_InvalidReference);
7586
}

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/BinaryReaderExtensions.cs

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,19 @@ internal static class BinaryReaderExtensions
1515
{
1616
private static object? s_baseAmbiguousDstDateTime;
1717

18+
internal static SerializationRecordType ReadSerializationRecordType(this BinaryReader reader, AllowedRecordTypes allowed)
19+
{
20+
byte nextByte = reader.ReadByte();
21+
if (nextByte > (byte)SerializationRecordType.MethodReturn // MethodReturn is the last defined value.
22+
|| (nextByte > (byte)SerializationRecordType.ArraySingleString && nextByte < (byte)SerializationRecordType.MethodCall) // not part of the spec
23+
|| ((uint)allowed & (1u << nextByte)) == 0) // valid, but not allowed
24+
{
25+
ThrowHelper.ThrowForUnexpectedRecordType(nextByte);
26+
}
27+
28+
return (SerializationRecordType)nextByte;
29+
}
30+
1831
internal static BinaryArrayType ReadArrayType(this BinaryReader reader)
1932
{
2033
byte arrayType = reader.ReadByte();
@@ -48,7 +61,7 @@ internal static PrimitiveType ReadPrimitiveType(this BinaryReader reader)
4861
{
4962
byte primitiveType = reader.ReadByte();
5063
// String is the last defined value, 4 is not used at all.
51-
if (primitiveType is 4 or > (byte)PrimitiveType.String)
64+
if (primitiveType is 0 or 4 or (byte)PrimitiveType.Null or > (byte)PrimitiveType.String)
5265
{
5366
ThrowHelper.ThrowInvalidValue(primitiveType);
5467
}
@@ -64,7 +77,7 @@ internal static object ReadPrimitiveValue(this BinaryReader reader, PrimitiveTyp
6477
PrimitiveType.Boolean => reader.ReadBoolean(),
6578
PrimitiveType.Byte => reader.ReadByte(),
6679
PrimitiveType.SByte => reader.ReadSByte(),
67-
PrimitiveType.Char => reader.ReadChar(),
80+
PrimitiveType.Char => reader.ParseChar(),
6881
PrimitiveType.Int16 => reader.ReadInt16(),
6982
PrimitiveType.UInt16 => reader.ReadUInt16(),
7083
PrimitiveType.Int32 => reader.ReadInt32(),
@@ -73,11 +86,35 @@ internal static object ReadPrimitiveValue(this BinaryReader reader, PrimitiveTyp
7386
PrimitiveType.UInt64 => reader.ReadUInt64(),
7487
PrimitiveType.Single => reader.ReadSingle(),
7588
PrimitiveType.Double => reader.ReadDouble(),
76-
PrimitiveType.Decimal => decimal.Parse(reader.ReadString(), CultureInfo.InvariantCulture),
89+
PrimitiveType.Decimal => reader.ParseDecimal(),
7790
PrimitiveType.DateTime => CreateDateTimeFromData(reader.ReadUInt64()),
7891
_ => new TimeSpan(reader.ReadInt64()),
7992
};
8093

94+
// BinaryFormatter serializes decimals as strings and we can't BinaryReader.ReadDecimal.
95+
internal static decimal ParseDecimal(this BinaryReader reader)
96+
{
97+
string text = reader.ReadString();
98+
if (!decimal.TryParse(text, NumberStyles.Number, CultureInfo.InvariantCulture, out decimal result))
99+
{
100+
ThrowHelper.ThrowInvalidFormat();
101+
}
102+
103+
return result;
104+
}
105+
106+
internal static char ParseChar(this BinaryReader reader)
107+
{
108+
try
109+
{
110+
return reader.ReadChar();
111+
}
112+
catch (ArgumentException) // A surrogate character was read.
113+
{
114+
throw new SerializationException(SR.Serialization_SurrogateCharacter);
115+
}
116+
}
117+
81118
/// <summary>
82119
/// Creates a <see cref="DateTime"/> object from raw data with validation.
83120
/// </summary>

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/ThrowHelper.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,9 @@ internal static void ThrowArrayContainedNulls()
2929
internal static void ThrowInvalidAssemblyName(string rawName)
3030
=> throw new SerializationException(SR.Format(SR.Serialization_InvalidAssemblyName, rawName));
3131

32+
internal static void ThrowInvalidFormat()
33+
=> throw new SerializationException(SR.Serialization_InvalidFormat);
34+
3235
internal static void ThrowEndOfStreamException()
3336
=> throw new EndOfStreamException();
3437

0 commit comments

Comments
 (0)