-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Prohibit BF unless the app opts in #48527
Conversation
Tagging subscribers to this area: @safern Issue DetailsSwitch to Binary Reader/Writer instead and allow for the
|
...ntModel.TypeConverter/src/System/ComponentModel/Design/DesigntimeLicenseContextSerializer.cs
Outdated
Show resolved
Hide resolved
...ntModel.TypeConverter/src/System/ComponentModel/Design/DesigntimeLicenseContextSerializer.cs
Outdated
Show resolved
Hide resolved
...ntModel.TypeConverter/src/System/ComponentModel/Design/DesigntimeLicenseContextSerializer.cs
Outdated
Show resolved
Hide resolved
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 |
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.
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.
...ntModel.TypeConverter/src/System/ComponentModel/Design/DesigntimeLicenseContextSerializer.cs
Outdated
Show resolved
Hide resolved
...ntModel.TypeConverter/src/System/ComponentModel/Design/DesigntimeLicenseContextSerializer.cs
Outdated
Show resolved
Hide resolved
...ntModel.TypeConverter/src/System/ComponentModel/Design/DesigntimeLicenseContextSerializer.cs
Outdated
Show resolved
Hide resolved
...ntModel.TypeConverter/src/System/ComponentModel/Design/DesigntimeLicenseContextSerializer.cs
Outdated
Show resolved
Hide resolved
...ntModel.TypeConverter/src/System/ComponentModel/Design/DesigntimeLicenseContextSerializer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.ComponentModel.TypeConverter/src/ILLink/ILLink.Suppressions.xml
Outdated
Show resolved
Hide resolved
.../System.ComponentModel.TypeConverter/tests/Design/DesignTimeLicenseContextSerializerTests.cs
Outdated
Show resolved
Hide resolved
...ntModel.TypeConverter/src/System/ComponentModel/Design/DesigntimeLicenseContextSerializer.cs
Outdated
Show resolved
Hide resolved
...tem.ComponentModel.TypeConverter/src/System/ComponentModel/Design/LocalAppContextSwitches.cs
Outdated
Show resolved
Hide resolved
...ntModel.TypeConverter/src/System/ComponentModel/Design/DesigntimeLicenseContextSerializer.cs
Show resolved
Hide resolved
...tem.ComponentModel.TypeConverter/src/System/ComponentModel/Design/LocalAppContextSwitches.cs
Outdated
Show resolved
Hide resolved
...tem.ComponentModel.TypeConverter/src/System/ComponentModel/Design/LocalAppContextSwitches.cs
Outdated
Show resolved
Hide resolved
...del.TypeConverter/tests/TrimmingTests/System.ComponentModel.TypeConverter.TrimmingTests.proj
Outdated
Show resolved
Hide resolved
.../System.ComponentModel.TypeConverter/tests/Design/DesigntimeLicenseContextSerializerTests.cs
Outdated
Show resolved
Hide resolved
.../System.ComponentModel.TypeConverter/tests/Design/DesigntimeLicenseContextSerializerTests.cs
Outdated
Show resolved
Hide resolved
.../System.ComponentModel.TypeConverter/tests/Design/DesigntimeLicenseContextSerializerTests.cs
Outdated
Show resolved
Hide resolved
...ntModel.TypeConverter/src/System/ComponentModel/Design/DesigntimeLicenseContextSerializer.cs
Outdated
Show resolved
Hide resolved
...ntModel.TypeConverter/src/System/ComponentModel/Design/DesigntimeLicenseContextSerializer.cs
Outdated
Show resolved
Hide resolved
...ntModel.TypeConverter/src/System/ComponentModel/Design/DesigntimeLicenseContextSerializer.cs
Outdated
Show resolved
Hide resolved
...ntModel.TypeConverter/src/System/ComponentModel/Design/DesigntimeLicenseContextSerializer.cs
Outdated
Show resolved
Hide resolved
...ntModel.TypeConverter/src/System/ComponentModel/Design/DesigntimeLicenseContextSerializer.cs
Show resolved
Hide resolved
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 looks good to me, assuming CI passes.
@@ -19,6 +19,7 @@ configurations but their defaults might vary as any SDK can set the defaults dif | |||
| StartupHookSupport | System.StartupHookProvider.IsSupported | Startup hooks are disabled when set to false. Startup hook related functionality can be trimmed. | | |||
| TBD | System.Threading.ThreadPool.EnableDispatchAutoreleasePool | When set to true, creates an NSAutoreleasePool around each thread pool work item on applicable platforms. | | |||
| CustomResourceTypesSupport | System.Resources.ResourceManager.AllowCustomResourceTypes | Use of custom resource types is disabled when set to false. ResourceManager code paths that use reflection for custom types can be trimmed. | | |||
| EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization | System.ComponentModel.TypeConverter.EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization | BinaryFormatter serialization support is trimmed when set to false. | |
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.
Don't forget to open a PR in dotnet/sdk to add this MSBuild property.
.../System.ComponentModel.TypeConverter/tests/Design/DesigntimeLicenseContextSerializerTests.cs
Show resolved
Hide resolved
.../System.ComponentModel.TypeConverter/tests/Design/DesigntimeLicenseContextSerializerTests.cs
Show resolved
Hide resolved
.../System.ComponentModel.TypeConverter/tests/Design/DesigntimeLicenseContextSerializerTests.cs
Outdated
Show resolved
Hide resolved
.../System.ComponentModel.TypeConverter/tests/Design/DesigntimeLicenseContextSerializerTests.cs
Outdated
Show resolved
Hide resolved
...del.TypeConverter/tests/TrimmingTests/System.ComponentModel.TypeConverter.TrimmingTests.proj
Show resolved
Hide resolved
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.
Thanks for the good work here @pgovind.
The Android builds stalling is a known issue. Merging this PR. Thanks for reviews everyone! |
return true; | ||
} | ||
|
||
[DynamicDependency(DynamicallyAccessedMemberTypes.NonPublicConstructors, typeof(Hashtable))] |
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 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?
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.
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?
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 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
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.
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!
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
* Delete infra changes from #48527 * sq
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