Move System.Runtime.Serialization.Formatters.Binary.BinaryFormatter to CoreLib

[marek-safar]

System.Runtime.Serialization.Formatters.Binary.BinaryFormatter type is always required by System.Private.CoreLib because it's used for resources serialization/deserialization. The required code after trimming in size sensitive scenarios ends up quite small (about 6kB) which indicates there is not a large chunk of code to be moved.

Moving the implementation would help reduce the number of file transfers for browser and get rid of extra reflection call in

runtime/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.Core.cs

Lines 68 to 86 in 9907151

	private void InitializeBinaryFormatter()
	{
	LazyInitializer.EnsureInitialized(ref s_binaryFormatterType, () =>
	Type.GetType("System.Runtime.Serialization.Formatters.Binary.BinaryFormatter, System.Runtime.Serialization.Formatters, Version=0.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a",
	throwOnError: true)!);
	LazyInitializer.EnsureInitialized(ref s_deserializeMethod, () =>
	{
	MethodInfo binaryFormatterDeserialize = s_binaryFormatterType!.GetMethod("Deserialize", new Type[] { typeof(Stream) })!;
	// create an unbound delegate that can accept a BinaryFormatter instance as object
	return (Func<object?, Stream, object>)typeof(ResourceReader)
	.GetMethod(nameof(CreateUntypedDelegate), BindingFlags.NonPublic | BindingFlags.Static)!
	.MakeGenericMethod(s_binaryFormatterType)
	.Invoke(null, new object[] { binaryFormatterDeserialize })!;
	});
	_binaryFormatter = Activator.CreateInstance(s_binaryFormatterType!)!;
	}

@eerhardt

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[GrabYourPitchforks]

BinaryFormatter doesn't work at all in browser after the changes in #38963 come online. Why not ifdef this code away in wasm? Or condition this check on the internal API SerializationInfo.IsBinaryFormatterEnabled (also part of that PR), which will always evaluate to false on wasm and other scenarios where BF is intended to be trimmed away?

[jkotas]

I agree with @GrabYourPitchforks . System.Runtime.Serialization.Formatters.Binary.BinaryFormatter has problematic security characteristics. It is better for it to be in separate binary.

[eerhardt]

The required code after trimming in size sensitive scenarios ends up quite small (about 6kB) which indicates there is not a large chunk of code to be moved.

Note that it is that small because the linker is removing all the methods from the type. This is because it can't automatically detect the above reflection usage and produces a warning (which we are currently ignoring all warnings).

See the conversation here: #38432 (comment)

[GrabYourPitchforks]

If there aren't any objections, I'll experiment with sending a new iteration of #38963 that addresses this code path as well.

[GrabYourPitchforks]

@marek-safar These code paths should no longer be called in wasm applications, so there should be no remaining references to the Formatters package. Anything else we need to do here?

[eerhardt]

I don't believe there is anything else we need to do here. After linking a Blazor WASM app with the latest runtime, I am not seeing System.Runtime.Serialization.Formatters.dll in the output at all.
Closing.