Prohibit BF unless the app opts in

[pgovind]

Switch to Binary Reader/Writer instead and allow for a System.ComponentModel.TypeConverter.EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization switch to fall back to the current Binary Formatter based (de)serialization.

Fixes #39293

As a follow up, I need to do something similar to https://github.com/dotnet/sdk/pull/15702/files in the sdk once this PR goes in

[ghost]

Tagging subscribers to this area: @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Switch to Binary Reader/Writer instead and allow for the System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization switch to fall back to the current Binary Formatter based (de)serialization.

Author:	pgovind
Assignees:	-
Labels:	area-System.ComponentModel
Milestone:	-

[pgovind]

CI failure are because of this pre-existing unit test:

        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsBinaryFormatterSupported))]
        public void Ctor_SerializationInfo_StreamingContext()
        {
            using (var stream = new MemoryStream())
            {
                var binaryFormatter = new BinaryFormatter();
                binaryFormatter.Serialize(stream, new LicenseException(typeof(int)));

                stream.Seek(0, SeekOrigin.Begin);
                Assert.IsType<LicenseException>(binaryFormatter.Deserialize(stream));
            }
        }

 System.NotSupportedException : BinaryFormatter serialization and deserialization are disabled within this application. See https://aka.ms/binaryformatter for more information.

But I don't understand why this is failing suddenly? Have we disabled Binary Formatter usage in the libs already? If so, shouldn't more libraries fail? @GrabYourPitchforks

[ericstj]

But I don't understand why this is failing suddenly?

your added tests are leaking AppContext state. You should cleanup any static state or run in isolation (eg:remote executor). Check what other tests around the same switches are doing.

[eerhardt]

This looks good to me, assuming CI passes.

[eerhardt]

Don't forget to open a PR in dotnet/sdk to add this MSBuild property.

[eerhardt]

Thanks for the good work here @pgovind.

[pgovind]

The Android builds stalling is a known issue. Merging this PR. Thanks for reviews everyone!

[jkotas]

This means that the Hashtable constructors are going to be kept even when EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization is disabled. This is not linker friendly.

We generally do not care about linker compatibility with binary serialization enabled. Could you please delete this DynamicDependency if you agree?

[pgovind]

do not care about linker compatibility with binary serialization enabled

What do you mean by linker compatibility here?

If I remove the DynamicDependency here, an app with binary serialization enabled will fail at runtime because Reflection will try to construct a HashTable and fail to find the right constructor?

[eerhardt]

This means that the Hashtable constructors are going to be kept even when EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization is disabled.

The Hashtable constructors will only be preserved if EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization is explicitly enabled. @pgovind is going to add a follow up PR in dotnet/sdk which will set EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization=false by default when PublishTrimmed=true.

This follows the same pattern with other feature switches that are set by default for trimmed apps:

• StartupHookSupport
• CustomResourceTypesSupport

See https://github.com/dotnet/sdk/blob/f922d2ef62dcaf6b99e64ed2ff48f772ad081d86/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets#L23-L41

[eerhardt]

Oh, I take that back... the check for the feature switch is inside this method. This needs to be refactored so the method attributed with [DynamicDependency] is only called within the feature switch check.

Nice catch @jkotas!

[jkotas]

If I remove the DynamicDependency here, an app with binary serialization enabled will fail at runtime because Reflection will try to construct a HashTable and fail to find the right constructor?

It is fine for an trimmed app with binary serialization enabled to fail at the runtime. If you enable BinarySerialization you will get a lot linker warnings about non-analyzable reflection uses. We do not plan to fix those warnings. BinarySerialization is deprecated feature and it is fine that it does not work with trimming.
We have been deleting old code that was trying to make the binary serialization compatible with trimming, example dotnet/linker#1925.

It is why I am suggesting to just delete this [DynamicDependency]. Deleting it would be consistent with approach we use everywhere else wrt binary serialization.

[jkotas]

In other words, if we are not cleaning up all build-time linker warnings for given scenario, we should make no affort to make the scenario work.

[eerhardt]

I am saying that we should be doing same thing here as what we are doing for CustomResourceTypesSupport.

I agree. And for CustomResourceTypesSupport, we are going to need to make it work in the context of WinForms apps. Or else WinForms apps that use resources (pretty much any real app) plainly won't work once trimmed. And WinForms, the owners of the scenario, will have to figure out the right things to preserve to make it work.

Here - we are the owners of the scenario. The runtime is calling this code. I don't think it is unreasonable for us to figure out what are the right things to preserve here - especially since it is so simple.

[eerhardt]

if we are not cleaning up all build-time linker warnings for given scenario

We did clean up the linker warnings for this scenario, because this was so simple - it is just (de)serialization string and Hashtable. And we added a trimming test to ensure it works.

[jkotas]

And for CustomResourceTypesSupport, we are going to need to make it work in the context of WinForms apps.

And the scheme to make it work should not be based on binary serializer to deliver on https://github.com/dotnet/designs/blob/main/accepted/2020/better-obsoletion/binaryformatter-obsoletion.md#net-6-nov-2021

We did clean up the linker warnings for this scenario, because this was so simple

We should not be spending any time on improving binary serializer paths, even if it is simple. It is not a good use of time and it creates conflicting message about aggresively deprecating binary serializer.

[eerhardt]

OK. I just felt that if we save a customer a few hours of trying to figure out why their app doesn't work, it would be worth it.

We can delete this attribute, plus the tests. and add the ILLink warnings to an ILLink.Suppressions.LibraryBuild.xml file.