-
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
Changes from 15 commits
217b242
0c28c37
046372f
7fdaf7e
7002e82
4c4bb95
570a875
773bb77
9f78bcb
78a3c76
3d08042
43a49f4
712bfbd
66c052f
2b465b8
9b630a7
558a91d
cd7e114
8dfe20a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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 // Issue https://github.com/dotnet/runtime/issues/39293 tracks finding an alternative to BinaryFormatter | ||
#pragma warning disable SYSLIB0011 | ||
formatter.Serialize(o, new object[] { cryptoKey, context._savedLicenseKeys }); | ||
#pragma warning restore SYSLIB0011 | ||
} | ||
|
||
internal static void Deserialize(Stream o, string cryptoKey, RuntimeLicenseContext context) | ||
private class StreamWrapper : Stream | ||
{ | ||
#pragma warning disable SYSLIB0011 // Issue https://github.com/dotnet/runtime/issues/39293 tracks finding an alternative to BinaryFormatter | ||
IFormatter formatter = new BinaryFormatter(); | ||
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); | ||
|
||
object obj = formatter.Deserialize(o); | ||
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. Choose a reason for hiding this commentThe 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 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 commentThe reason will be displayed to describe this comment to others. Learn more.
What do you mean by linker compatibility here? If I remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe 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 Nice catch @jkotas! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe 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 |
||
[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 (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 | ||
{ | ||
if (value[0] is string && (string)value[0] == cryptoKey) | ||
using (BinaryReader reader = new BinaryReader(wrappedStream, encoding: Text.Encoding.UTF8, leaveOpen: true)) | ||
{ | ||
context._savedLicenseKeys = (Hashtable)value[1]; | ||
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.