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.

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

Use PayloadReader in System.Resources.Extensions #102379

Merged
merged 40 commits into from
Jun 10, 2024
Merged

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented May 17, 2024

High level overview.

  1. All PayloadReader related code was moved to System.Runtime.Serialization.BinaryFormat library, the library is marked with IsPackable set to false so we don't publish any NuGet package by accident. Other projects reference it's code via links (to ensure nothing relies on the project before the API gets approved).
  2. The deserializer authored by @JeremyKuhne for WinForms was modified only to allow support of older monikers (see the commits).
  3. System.Resources.Extensions is now using simplified deserializer by default.
  4. It's possible to request using BinaryFormatter by setting System.Resources.Extensions.UseBinaryFormatter app context switch.
  5. All tests have been migrated and are passing.
  6. I've not reduced the code duplication between System.Resources.Extensions.BinaryFormat.Tests and actual BinaryFormatter test project as I assumed the latter is soon going to be removed from this repo.

@adamsitnik adamsitnik added this to the 9.0.0 milestone May 17, 2024
@adamsitnik adamsitnik self-assigned this May 17, 2024
@dotnet dotnet locked and limited conversation to collaborators May 17, 2024
@adamsitnik adamsitnik changed the title [DRAFT] Remove dependency to BinaryFormatter from System.Resources.Extensions Use PayloadReader in System.Resources.Extensions May 20, 2024
@adamsitnik
Copy link
Member Author

@JeremyKuhne I am not sure if you want to review all the files (you already did that in dotnet/winforms#11341). In case you don't, you can simply review the following commits:

  • f0c20fc where I applied some non trivial changes to get the code compile for older monikers
  • 3a90452 where I've fixed bugs the commit above has introduced
  • 00608e4 where I've made the switch
  • 6a36687 where I added the compat switch

What is still on my TODO list is to move the tests from https://github.com/dotnet/winforms/tree/main/src/System.Private.Windows.Core/tests/BinaryFormatTests. It will be another commit with no product code changes (only copy paste of the tests and maybe some minor MSBuild changes)

@adamsitnik adamsitnik marked this pull request as ready for review May 20, 2024 17:12
@dotnet dotnet unlocked this conversation May 20, 2024
}

[RequiresUnreferencedCode("Calls System.Reflection.Assembly.GetType(String, Boolean, Boolean)")]
private static Type? GetSimplyNamedTypeFromAssembly(Assembly assembly, TypeName typeName)
Copy link
Member

Choose a reason for hiding this comment

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

Note that this is effectively a straight copy of the existing BinaryFormatter logic. All catch blocks in this class match the existing behavior.

{
PendingSerializationInfo? pending = _pendingSerializationInfo.Dequeue();

// Using pendingCount to only requeue on the first pass.
Copy link
Member

Choose a reason for hiding this comment

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

I haven't found a need to requeue multiple times. One could, however, make this a bit more complicated and keep requeuing as long as the count continues to go down.

// Everything has to start with a header
var header = (SerializedStreamHeaderRecord)ReadNext(reader, recordMap, AllowedRecordTypes.SerializedStreamHeader, options, out _);
// and can be followed by any Object, BinaryLibrary and a MessageEnd.
const AllowedRecordTypes allowed = AllowedRecordTypes.AnyObject
Copy link
Member

Choose a reason for hiding this comment

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

Do the record type checks prevent any observably bad behavior? I worry a little bit about things ending up in orders that don't match the constraints of the spec, but it otherwise being ok.


private static List<T> DecodePrimitiveTypesToList(BinaryReader reader, int count)
{
List<T> values = [];
Copy link
Member

Choose a reason for hiding this comment

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

This will grow from 0 to 4 to 8 to ...

The 99% case for large arrays will be bytes, and that is covered for most inputs by the "if I have that much data remaining, just read it" above. But large arrays of TimeSpan (or int when not on new.NET) are going to have to go through many reallocs.

The easiest, of course, is

Suggested change
List<T> values = [];
List<T> values = new List<T>(count);

If the threat model indicates that this puts too much power in the hands of the payload, then it needs to change to a page based allocation and a custom type.

For expediency, I recommend just pre-sizing the list and taking this as an issue to follow up on later (as part of, or prior to, the threat model). As current usage is limited to resources, it's a fine thing to do.

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 list is not pre-sized on purpose: adamsitnik/SafePayloadReader#4

Since the code for decoding is already optimized for .NET Core, I am going to add the optimization for other TFMs

@steveharter
Copy link
Member

FYI test failure:

Microsoft.DotNet.XUnitExtensions.SkipTestException : Not enough memory available to test max array size support
Stack Trace
   at System.Runtime.Serialization.BinaryFormat.Tests.EdgeCaseTests.CanReadArrayOfAnySize(Int32 length) in /_/src/libraries/System.Runtime.Serialization.BinaryFormat/tests/EdgeCaseTests.cs:line 85
   at InvokeStub_EdgeCaseTests.CanReadArrayOfAnySize(Object, Span`1)
   at System.Reflection.MethodBaseInvoker.InvokeWithOneArg(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

adamsitnik and others added 2 commits June 6, 2024 12:38
Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
- prefer NET over NETCOREAPP when writing #if defines
- add ConditionalTheory to make the SkipTestException actually work
- ObjectNullMultiple must not allow for 0 inputs
- preallocate the List<T> for TFMs other than .NET
- ensure that reading arrays of primitive types is as fast as possible when there is enough data in the stream, throw when there is not, fall back to slow path when we don't know
- make GetArray reuse the previously created instance
- and provide tests for all of that
…tes as what is stored in the test source code. The blobs may not be identical (the output is not deterministic), but still valid.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Resources binary-serialization breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants