-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NRBF] Reject orphaned records and invalid references #103632
Conversation
@@ -57,11 +57,6 @@ internal override (AllowedRecordTypes allowed, PrimitiveType primitiveType) GetA | |||
if (record is MemberReferenceRecord memberReference) | |||
{ | |||
record = memberReference.GetReferencedRecord(); | |||
|
|||
if (record is not BinaryObjectStringRecord) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check is no longer needed here, as there is a universal check for all references in RecordMap.Add
. The benefit is that we throw as soon as we encounter an invalid reference.
# Conflicts: # src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecordId.cs
# Conflicts: # src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/NrbfDecoder.cs # src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/ThrowHelper.cs
We are going to discuss this further to decide if we want to change this logic in .NET 9 or document that the decoder does not reject these records. We could potentially add this logic in .NET 10 with the This is therefore not blocking the release. |
# Conflicts: # src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberReferenceRecord.cs # src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/NrbfDecoder.cs # src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/RecordMap.cs # src/libraries/System.Formats.Nrbf/tests/InvalidInputTests.cs # src/libraries/System.Formats.Nrbf/tests/System.Formats.Nrbf.Tests.csproj
A brief explanation of how I've implemented it:
RootId
, but nothing else (its type etc).BinaryLibraryRecord
at any point.ClassRecord
with any members orArrayRecord
with any items, a stack is being populated with information of expected record type. We read those records and if there is a type mismatch, we throw.@GrabYourPitchforks PTAL and let me know if you have any concerns. I would be happy to convert specific scenarios into test cases.