Make parts of ResourceManager trimming safe #38432
Conversation
We'll need a feature switch to turn off support for the parts that read text strings out of resources to make this fully safe. I got rid of the LazyInitializer because it's just extra overhead that is not buying anything, except making things impossible to statically analyze. There won't be races when it comes to reflection object...
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
| public ResourceManager(string baseName, Assembly assembly, Type? usingResourceSet) | ||
| public ResourceManager(string baseName, Assembly assembly, | ||
| [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)] | ||
| Type? usingResourceSet) |
| Type.GetType("System.Runtime.Serialization.Formatters.Binary.BinaryFormatter, System.Runtime.Serialization.Formatters, Version=0.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a", | ||
| throwOnError: true)!; | ||
|
|
||
| if (s_binaryFormatterType == null) |
There was a problem hiding this comment.
Either delete the null check; or change this to Interlocked.CompareExchange to make sure the first one wins in thread-safe way.
There was a problem hiding this comment.
Type.GetType with the same string always returns the same instance, right? So deleting the null check should be fine.
There was a problem hiding this comment.
What is the thread safety concern? I got rid of the LazyInitializer because it looked like unnecessary ceremony for something that is inherently thread safe, but I would like to understand what it was trying to solve.
There was a problem hiding this comment.
Here was past discussion around this: dotnet/coreclr#20907 (comment)
There was a problem hiding this comment.
Type.GetType with the same string always returns the same instance, right?
Unless you manage to load multiple copies of the framework. We generally do not handle such situation well, so ignoring its existence should be ok.
What is the thread safety concern?
My concern was that the null check as it is written right now provides false sense of thread-safety. It is either unnecessary or a thread safety bug.
I agree that LazyInitializer is way too expensive for that it does in 99% cases.
| @@ -65,24 +67,47 @@ private object DeserializeObject(int typeIndex) | |||
| return graph; | |||
| } | |||
|
|
|||
| // TODO: Remove this DynamicDependencyAttributes when https://github.com/mono/linker/issues/943 is fixed. | |||
| [DynamicDependency("Deserialize", "System.Runtime.Serialization.Formatters.Binary.BinaryFormatter", "System.Runtime.Serialization.Formatters")] | |||
There was a problem hiding this comment.
Note that today the BinaryFormatter type exists, and is empty in a default Blazor app. So this is going to make the app size larger.
We should implement a way to turn this behavior off. There are probably 2 levels that both could be done.
- Completely removing BinaryFormatter from an app, no matter where it is used. This was discussed in the feature-switch.md#security section of the design doc.
- Disallow deserialization in ResourceReader. The boolean is already there, we would just need to add a feature switch for it:
See the discussion here #32862 (comment)
cc @joperezr since he is already working in the Resources area for linking.
There was a problem hiding this comment.
On a second thought, I should probably just remove this part of the change - if the BinarySerializer indeed gets used for something, after trimming, chances are pretty low that we would end up in a happy place. I'll instead tag this and a couple other places as RequiresUnreferencedCode to get rid of some of the warning noise and leave this for the feature switches to take care of.
ghost
locked as resolved and limited conversation to collaborators
We'll need a feature switch to turn off support for the parts that read text strings out of resources to make this fully safe.
I got rid of the LazyInitializer because it's just extra overhead that is not buying anything, except making things impossible to statically analyze. There won't be races when it comes to reflection object...