Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Extend PayloadReader APIs to meet the WinForms requirements #11341

Closed
wants to merge 16 commits into from
Closed

Extend PayloadReader APIs to meet the WinForms requirements #11341

wants to merge 16 commits into from

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented May 9, 2024 •
edited by dotnet-policy-service bot
Loading

@JeremyKuhne I was working on the MSRCs today and managed to make only a small progress with transition to SafePayloadReader APIs. What I have here compiles, but you can revert the last revert to have a temporary state where some APIs have been transitioned and there are compiler errors.

Microsoft Reviewers: Open in CodeFlow

@adamsitnik
extend PayloadReader public API surface:
b59abfd 
- RecordMap needs to remain internal, but implement IReadOnlyDictionary<int, SerializationRecord> so it can be returned from a public method without exposing the type itself
- introduce new PayloadReader.Read overload that can return RecordMap via out parameter
- make SerializationRecord.ObjectId public
- make MemberReferenceRecord public, expose Reference property
@adamsitnik adamsitnik requested a review from a team as a code owner May 9, 2024 18:02
@adamsitnik
extend PayloadReader public API surface:
297381c 
- extend ArrayRecord with ElementTypeName and ElementTypeLibraryName public properties
@adamsitnik
switch to the new API (System.Private.Windows.Core compiles)
25e123e
@adamsitnik
remove code that won't be needed for reading, move code that will be …
c6f612d 
…needed for writing to System.Windows.Forms where it's used (System.Windows.Forms compiles)
@adamsitnik
- update test project so it compiles
ce67096 
- stop using internal APIs
- fix the bugs:
	- update the record map, so mapping system class of common primtive types to PrimitiveTypeRecord<T> is visible also to those who access it via Id
	- add missing support for mapping to PrimitiveTypeRecord for IntPtr and UIntPtr
	- use GetSerializationRecord when unwrapping
- remove FormatterTypeStyle.TypesWhenNeeded test (not supported)
- remove tests for internal APIs tested in PayloadReader project:
	- RecordMapTests
	- NullRecordTests
	- ClassInfoTests
	- MemberTypeInfo
@adamsitnik
fix the bugs:
045baed 
- check records by Ids to detect custom comparers for Hashtable
- ObjectRecordDeserializer needs to handle ArrayRecords and raw primitive values
- add support for jagged arrays with custom offsets (just to rejct them later)
- materialize arrays of primitive types using the provided API
- when mapping System Classes to PrimitiveTypeRecord<T> the Ids need to be preserved
- don't iterate over the same member twice
- support arrays of nullable primitives (don't represent them as ArrayRecord<ClassRecord> because they consists of MemberPrimitiveTypedRecord and NullsRecords)
@adamsitnik
disable some of the FormatterTypeStyle.TypesWhenNeeded tests (this ne…
7f409ae 
…eds a discussion whether we want to support that or not)
@adamsitnik
move the payload reader code to separate project to ensure we are not…
85c01ff 
… using any of the internal APIs
@adamsitnik
silence the compiler warnings
7b7d485
@adamsitnik
MemberReferenceRecord does not need to be public
c88443f
@adamsitnik
rename ClassRecord.GetObject to GetRawValue
c63a18c
adamsitnik
adamsitnik commented May 14, 2024
View reviewed changes
Winforms.sln Outdated Show resolved Hide resolved
adamsitnik
adamsitnik commented May 14, 2024
View reviewed changes
...c/System/Windows/Forms/BinaryFormat/Deserializer/ClassRecordSerializationInfoDeserializer.cs
}
while (_memberNamesIterator.MoveNext());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the order is no longer guaranteed to be preserved and I know it looks less clean than before, but I really hope that I don't need to expose an array (we are using dictionary to store the member names for security reasons:

https://github.com/adamsitnik/SafePayloadReader/blob/f9beee7176ce0bf29fdea794d4f79de5c024103c/System.Runtime.Serialization.BinaryFormat/Infos/ClassInfo.cs#L40-L45

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does have to be maintained for this to be compatible.

I'm not sure guarding against a lot of member names and member data is that useful. Are they really going to be able to DOS with this? Wouldn't an upper cap on number of members be more practical? I can't imagine any real-world class having more than 64 members that get serialized, picking some large number beyond that might be more useful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does have to be maintained for this to be compatible.

I tried to write a failing test for that and I've failed, it seems that the order is preserved as long as we don't mutate the dictionary afterwards: https://stackoverflow.com/a/64596145/5852046 But is it safe to rely on that assumption? I am not convinced.

We could use OrderedDictionary which offers O(1) lookups, but the inserts can be O(n) at worst, but the same is true for Dictionary. (source)

Wouldn't an upper cap on number of members be more practical?

I was thinking about introducing such limit: https://github.com/adamsitnik/SafePayloadReader/blob/f9beee7176ce0bf29fdea794d4f79de5c024103c/System.Runtime.Serialization.BinaryFormat/PayloadOptions.cs#L14

@GrabYourPitchforks Thoughts?

JeremyKuhne
JeremyKuhne reviewed May 14, 2024
View reviewed changes
...ate.Windows.Core/src/System/Windows/Forms/BinaryFormat/BinaryFormattedObject.TypeResolver.cs
return _lastType;
}

_lastLibraryId = libraryId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last retrieved is an important perf optimization, we should try and keep that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libraryId == _lastLibraryId was much faster when the API was accepting Id libraryId (integer). Now the public API provides AssemblyNameInfo and we need to compare strings to check for equality (in this particular case).
I doubt it would move the needle now, as what we have (a dictionary lookup) is very fast (O(1))

JeremyKuhne
JeremyKuhne reviewed May 14, 2024
View reviewed changes
src/System.Private.Windows.Core/tests/BinaryFormatTests/FormatTests/Common/NullRecordTests.cs

namespace FormatTests.Common;

public abstract class NullRecordTests<T> : SerializationTest<T> where T : ISerializer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These bad input tests need to be retained. As a quick hack you can make them just have the raw data streams.

@adamsitnik
address code review feedback:
f9e1aee 
- FormatterTypeStyle.TypeAlways is a must have, remove unsupported RecordTypes from the public enum and throw NotSupportedException when they are provided
- add support for Formatters.FormatterTypeStyle.XsdString
- enable tests for FormatterTypeStyle.TypesAlways | FormatterTypeStyle.XsdStringr
@adamsitnik adamsitnik mentioned this pull request May 15, 2024
Closed
JeremyKuhne
JeremyKuhne reviewed May 15, 2024
View reviewed changes
...e.Windows.Core/src/System/Windows/Forms/BinaryFormat/Deserializer/ArrayRecordDeserializer.cs
ArrayType.Rectangular => _elementType.MakeArrayType(arrayRecord.Rank),
_ => _elementType.MakeArrayType()
};
// Tricky part: for arrays of classes/structs the following record allocates and array of class records
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamsitnik Is this allocating an additional array or is it just returning the internal data structure?

@adamsitnik adamsitnik mentioned this pull request May 17, 2024
Merged
@JeremyKuhne JeremyKuhne added this to the .NET 10.0 milestone Jul 11, 2024
@adamsitnik
Merge remote-tracking branch 'upstream/feature/SafePayloadReader' int…
b752057 
…o switch

# Conflicts:
#	src/System.Private.Windows.Core/src/System/Windows/Forms/BinaryFormat/Deserializer/Deserializer.cs
@adamsitnik
switch to the new NuGet package with public API
acbb0bb
@adamsitnik
Copy link
Member Author

Closing in favor of #11745

@adamsitnik adamsitnik closed this Jul 25, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JeremyKuhne JeremyKuhne JeremyKuhne left review comments

Assignees

@adamsitnik adamsitnik

Labels
None yet
Projects
None yet
Milestone
.NET 10.0
Development

Successfully merging this pull request may close these issues.
2 participants
@adamsitnik @JeremyKuhne