Prohibit BF unless the app opts in #48527
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| <linker> | ||
| <assembly fullname="System.ComponentModel.TypeConverter"> | ||
| <type fullname="System.ComponentModel.Design.DesigntimeLicenseContextSerializer" feature="System.ComponentModel.TypeConverter.EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization" featurevalue="false"> | ||
| <method signature="System.Boolean get_EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization()" body="stub" value="false" /> | ||
| </type> | ||
| </assembly> | ||
| </linker> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| using System.Runtime.Serialization; | ||
| using System.Collections; | ||
| using System.IO; | ||
| using System.Diagnostics; | ||
| using System.Diagnostics.CodeAnalysis; | ||
|
|
||
| namespace System.ComponentModel.Design | ||
|
|
@@ -14,6 +15,10 @@ namespace System.ComponentModel.Design | |
| /// </summary> | ||
| public class DesigntimeLicenseContextSerializer | ||
| { | ||
| internal const byte BinaryWriterMagic = 255; | ||
|
|
||
| private static bool EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization { get; } = AppContext.TryGetSwitch("System.ComponentModel.TypeConverter.EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization", out bool isEnabled) ? isEnabled : false; | ||
|
|
||
| // Not creatable. | ||
| private DesigntimeLicenseContextSerializer() | ||
| { | ||
|
|
@@ -24,26 +29,156 @@ private DesigntimeLicenseContextSerializer() | |
| /// using the specified key and output stream. | ||
| /// </summary> | ||
| public static void Serialize(Stream o, string cryptoKey, DesigntimeLicenseContext context) | ||
| { | ||
| if (EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization) | ||
| { | ||
| SerializeWithBinaryFormatter(o, cryptoKey, context); | ||
| } | ||
| else | ||
| { | ||
| using (BinaryWriter writer = new BinaryWriter(o, encoding: Text.Encoding.UTF8, leaveOpen: true)) | ||
| { | ||
| writer.Write(BinaryWriterMagic); // flag to identify BinaryWriter | ||
| writer.Write(cryptoKey); | ||
| writer.Write(context._savedLicenseKeys.Count); | ||
| foreach (DictionaryEntry keyAndValue in context._savedLicenseKeys) | ||
| { | ||
| writer.Write(keyAndValue.Key.ToString()); | ||
| writer.Write(keyAndValue.Value.ToString()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", | ||
| Justification = "Only simple types (string and HashTable) are serialized with BinaryFormatter. These types can be serialized in a trimmed application.")] | ||
| private static void SerializeWithBinaryFormatter(Stream o, string cryptoKey, DesigntimeLicenseContext context) | ||
pgovind marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| IFormatter formatter = new BinaryFormatter(); | ||
| #pragma warning disable SYSLIB0011 | ||
| formatter.Serialize(o, new object[] { cryptoKey, context._savedLicenseKeys }); | ||
| #pragma warning restore SYSLIB0011 | ||
| } | ||
|
|
||
| private class StreamWrapper : Stream | ||
| { | ||
| private Stream _stream; | ||
| private bool _readFirstByte; | ||
| internal byte _firstByte; | ||
| public StreamWrapper(Stream stream) | ||
| { | ||
| _stream = stream; | ||
| _readFirstByte = false; | ||
| _firstByte = 0; | ||
| } | ||
|
|
||
| public override bool CanRead => _stream.CanRead; | ||
|
|
||
| public override bool CanSeek => _stream.CanSeek; | ||
|
|
||
| public override bool CanWrite => _stream.CanWrite; | ||
|
|
||
| public override long Length => _stream.Length; | ||
|
|
||
| public override long Position { get => _stream.Position; set => _stream.Position = value; } | ||
|
|
||
| public override void Flush() => _stream.Flush(); | ||
|
|
||
| public override int Read(byte[] buffer, int offset, int count) | ||
| { | ||
| Debug.Assert(_stream.Position != 0, "Expected the first byte to be read first"); | ||
| if (_stream.Position == 1) | ||
| { | ||
| Debug.Assert(_readFirstByte == true); | ||
| // Add the first byte read by ReadByte into buffer here | ||
| buffer[offset] = _firstByte; | ||
| return _stream.Read(buffer, offset + 1, count - 1) + 1; | ||
| } | ||
| return _stream.Read(buffer, offset, count); | ||
| } | ||
|
|
||
| public override long Seek(long offset, SeekOrigin origin) => _stream.Seek(offset, origin); | ||
|
|
||
| public override void SetLength(long value) => _stream.SetLength(value); | ||
|
|
||
| public override void Write(byte[] buffer, int offset, int count) => _stream.Write(buffer, offset, count); | ||
|
|
||
| public override int ReadByte() | ||
pgovind marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| byte read = (byte)_stream.ReadByte(); | ||
| _firstByte = read; | ||
| _readFirstByte = true; | ||
| return read; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// During deserialization, the stream passed in may be binary formatted or may have used binary writer. This is a quick test to discern between them. | ||
| /// </summary> | ||
| private static bool StreamIsBinaryFormatted(StreamWrapper stream) | ||
| { | ||
| // For binary formatter, the first byte is the SerializationHeaderRecord and has a value 0 | ||
| int firstByte = stream.ReadByte(); | ||
| if (firstByte != 0) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| [DynamicDependency(DynamicallyAccessedMemberTypes.NonPublicConstructors, typeof(Hashtable))] | ||
|
There was a problem hiding this comment. This means that the Hashtable constructors are going to be kept even when 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.
What do you mean by linker compatibility here? If I remove the There was a problem hiding this comment.
The Hashtable constructors will only be preserved if This follows the same pattern with other feature switches that are set by default for trimmed apps:
There was a problem hiding this comment. 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 Nice catch @jkotas! There was a problem hiding this comment.
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. It is why I am suggesting to just delete this There was a problem hiding this comment. 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.
I agree. And for 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.
We did clean up the linker warnings for this scenario, because this was so simple - it is just (de)serialization There was a problem hiding this comment.
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 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. 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 |
||
| [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", | ||
| Justification = "HashTable's Serialization ctor will be preserved by the DynamicDependency.")] | ||
| private static void DeserializeUsingBinaryFormatter(StreamWrapper wrappedStream, string cryptoKey, RuntimeLicenseContext context) | ||
| { | ||
| if (EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization) | ||
| { | ||
| #pragma warning disable SYSLIB0011 | ||
| IFormatter formatter = new BinaryFormatter(); | ||
|
|
||
| object obj = formatter.Deserialize(wrappedStream); | ||
| #pragma warning restore SYSLIB0011 | ||
|
|
||
| if (obj is object[] value) | ||
| { | ||
| if (value[0] is string && (string)value[0] == cryptoKey) | ||
| { | ||
| context._savedLicenseKeys = (Hashtable)value[1]; | ||
| } | ||
| } | ||
| } | ||
| else | ||
| { | ||
| throw new NotSupportedException(SR.BinaryFormatterMessage); | ||
| } | ||
| } | ||
|
|
||
| internal static void Deserialize(Stream o, string cryptoKey, RuntimeLicenseContext context) | ||
| { | ||
| StreamWrapper wrappedStream = new StreamWrapper(o); | ||
| if (StreamIsBinaryFormatted(wrappedStream)) | ||
pgovind marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| DeserializeUsingBinaryFormatter(wrappedStream, cryptoKey, context); | ||
| } | ||
| else | ||
| { | ||
| using (BinaryReader reader = new BinaryReader(wrappedStream, encoding: Text.Encoding.UTF8, leaveOpen: true)) | ||
| { | ||
| byte binaryWriterIdentifer = wrappedStream._firstByte; | ||
| Debug.Assert(binaryWriterIdentifer == BinaryWriterMagic, $"Expected the first byte to be {BinaryWriterMagic}"); | ||
| string streamCryptoKey = reader.ReadString(); | ||
| int numEntries = reader.ReadInt32(); | ||
| if (streamCryptoKey == cryptoKey) | ||
| { | ||
| context._savedLicenseKeys.Clear(); | ||
| for (int i = 0; i < numEntries; i++) | ||
| { | ||
| string key = reader.ReadString(); | ||
| string value = reader.ReadString(); | ||
| context._savedLicenseKeys.Add(key, value); | ||
pgovind marked this conversation as resolved.
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.
Don't forget to open a PR in dotnet/sdk to add this MSBuild property.