Hook BinaryFormatter replacement in Resx / Clipboard (#9178)

This uses the BinaryFormat support classes to handle common framework types before falling back to the BinaryFormatter.

There is some cleanup in the Resx code and simplification in DataObject by adding internal constant strings for known DataFormats.

Hook in the ActiveXImpl formatter path. Remove an unnecessary copy in its save path.

Also move MultitargetUtil to the normal CriticalException handler.

Co-authored-by: Klaus Löffelmann <9663150+KlausLoeffelmann@users.noreply.github.com>

src/System.Windows.Forms.Primitives/tests/UnitTests/System/Windows/Forms/BinaryFormat/HashTableTests.cs

@@ -155,12 +155,6 @@ public void RoundTripHashtables(Hashtable hashtable)
  { "That", "This" }
  },
  new Hashtable()
- {
- { "This", "That" },
- { "TheOther", "This" },
- { "That", "This" }
- },
- new Hashtable()
  {
  { "Yowza", null },
  { "Youza", null },

src/System.Windows.Forms/src/System/Resources/ResXSerializationBinder.cs

@@ -7,24 +7,31 @@
 
 namespace System.Resources;
 
-// This class implements a partial type resolver for the BinaryFormatter.
-// This is needed to be able to read binary serialized content from older
-// NDP types and map them to newer versions.
-
+/// <summary>
+/// This class implements a partial type resolver for the BinaryFormatter can provide custom type name binding to
+/// or from types.
+/// </summary>
+/// <remarks>
+/// <para>
+/// The key usage of this type is to attempt to redirect to / from .NET Framework type names.
+/// </para>
+/// </remarks>
 internal class ResXSerializationBinder : SerializationBinder
 {
  private readonly ITypeResolutionService? _typeResolver;
  private readonly Func<Type?, string>? _typeNameConverter;
 
- internal ResXSerializationBinder(ITypeResolutionService? typeResolver)
- {
- _typeResolver = typeResolver;
- }
+ /// <param name="typeResolver">
+ /// The custom type resolution service used to bind names to a specific <see cref="Type"/>. Only
+ /// <see cref="ITypeResolutionService.GetType(string)"/> is called by this binder.
+ /// </param>
+ internal ResXSerializationBinder(ITypeResolutionService? typeResolver) => _typeResolver = typeResolver;
 
- internal ResXSerializationBinder(Func<Type?, string>? typeNameConverter)
- {
- _typeNameConverter = typeNameConverter;
- }
+ /// <param name="typeNameConverter">
+ /// The type name converter to use for binding a <see cref="Type"/> to a custom name. This is passed in through
+ /// constructors on <see cref="ResXDataNode"/> such as <see cref="ResXDataNode(string, object?, Func{Type?, string}?)"/>
+ /// </param>
+ internal ResXSerializationBinder(Func<Type?, string>? typeNameConverter) => _typeNameConverter = typeNameConverter;
 
  public override Type? BindToType(
  string assemblyName,
@@ -35,69 +42,66 @@ internal ResXSerializationBinder(Func<Type?, string>? typeNameConverter)
  return null;
  }
 
+ // Try the fully-qualified name first.
  typeName = $"{typeName}, {assemblyName}";
 
  Type? type = _typeResolver.GetType(typeName);
- if (type is null)
+ if (type is not null)
  {
- string[] typeParts = typeName.Split(',');
+ return type;
+ }
 
- // Break up the assembly name from the rest of the assembly strong name.
- // we try 1) FQN 2) FQN without a version 3) just the short name.
- if (typeParts is not null && typeParts.Length > 2)
- {
- string partialName = typeParts[0].Trim();
+ string[] typeParts = typeName.Split(',');
 
- for (int i = 1; i < typeParts.Length; ++i)
+ if (typeParts is not null && typeParts.Length > 2)
+ {
+ string partialName = typeParts[0].Trim();
+
+ // Strip out the version.
+ for (int i = 1; i < typeParts.Length; ++i)
+ {
+ string typePart = typeParts[i].Trim();
+ if (!typePart.StartsWith("Version=", StringComparison.Ordinal)
+ && !typePart.StartsWith("version=", StringComparison.Ordinal))
  {
- string typePart = typeParts[i].Trim();
- if (!typePart.StartsWith("Version=", StringComparison.Ordinal)
- && !typePart.StartsWith("version=", StringComparison.Ordinal))
- {
- partialName = $"{partialName}, {typePart}";
- }
+ partialName = $"{partialName}, {typePart}";
  }
-
- type = _typeResolver.GetType(partialName);
- type ??= _typeResolver.GetType(typeParts[0].Trim());
  }
+
+ // Try the name without the version.
+ type = _typeResolver.GetType(partialName);
+
+ // If that didn't work, try the simple name.
+ type ??= _typeResolver.GetType(typeParts[0].Trim());
  }
 
- // Binder couldn't handle it, let the default loader take over.
+ // Hand back what we found or null to let the default loader take over.
  return type;
  }
 
- // Get the multitarget-aware string representation for the give type.
  public override void BindToName(Type serializedType, out string? assemblyName, out string? typeName)
  {
- // Normally we don't change typeName when changing the target framework,
- // only assembly version or assembly name might change, thus we are setting
- // typeName only if it changed with the framework version.
- // If binder passes in a null, BinaryFormatter will use the original value or
- // for un-serializable types will redirect to another type.
- // For example:
- //
- // Encoding = Encoding.GetEncoding("shift_jis");
- // public Encoding Encoding { get; set; }
- // property type (Encoding) is abstract, but the value is instantiated to a specific class,
- // and should be serialized as a specific class in order to be able to instantiate the result.
- //
- // another example are singleton objects like DBNull.Value which are serialized by System.UnitySerializationHolder
- typeName = null;
+ // Normally, we don't change the type name when changing the target framework, only the assembly name.
+ // Setting the out values to null indicates that we want default handling.
+
  if (_typeNameConverter is not null)
  {
+ // Allow the specified type name converter to modify the type name.
  string? assemblyQualifiedTypeName = MultitargetUtil.GetAssemblyQualifiedName(serializedType, _typeNameConverter);
  if (!string.IsNullOrEmpty(assemblyQualifiedTypeName))
  {
+ // Split the assembly name from the type name.
  int pos = assemblyQualifiedTypeName.IndexOf(',');
  if (pos > 0 && pos < assemblyQualifiedTypeName.Length - 1)
  {
+ // Set the custom assembly name.
  assemblyName = assemblyQualifiedTypeName[(pos + 1)..].TrimStart();
- string newTypeName = assemblyQualifiedTypeName.Substring(0, pos);
- if (!string.Equals(newTypeName, serializedType.FullName, StringComparison.InvariantCulture))
- {
- typeName = newTypeName;
- }
+
+ // Customize the type name only if it changed.
+ string newTypeName = assemblyQualifiedTypeName[..pos];
+ typeName = string.Equals(newTypeName, serializedType.FullName, StringComparison.Ordinal)
+ ? null
+ : newTypeName;
 
  return;
  }

src/System.Windows.Forms/src/System/Windows/Forms/Control.ActiveXImpl.cs

@@ -11,6 +11,7 @@
 using System.Runtime.Serialization;
 using System.Runtime.Serialization.Formatters.Binary;
 using System.Text;
+using System.Windows.Forms.BinaryFormat;
 using Windows.Win32.System.Com;
 using Windows.Win32.System.Com.StructuredStorage;
 using Windows.Win32.System.Ole;
@@ -260,8 +261,7 @@ internal bool EventsFrozen
  internal HWND HWNDParent { get; private set; }
 
  /// <summary>
- /// Retrieves the number of logical pixels per inch on the
- /// primary monitor.
+ /// Retrieves the number of logical pixels per inch on the primary monitor.
  /// </summary>
  private static Point LogPixels
  {
@@ -1019,8 +1019,7 @@ internal HRESULT InPlaceDeactivate()
  internal HRESULT IsDirty() => _activeXState[s_isDirty] ? HRESULT.S_OK : HRESULT.S_FALSE;
 
  /// <summary>
- /// Looks at the property to see if it should be loaded / saved as a resource or
- /// through a type converter.
+ /// Looks at the property to see if it should be loaded / saved as a resource or through a type converter.
  /// </summary>
  private bool IsResourceProperty(PropertyDescriptor property)
  {
@@ -1033,8 +1032,15 @@ private bool IsResourceProperty(PropertyDescriptor property)
  return false;
  }
 
- // Otherwise we require the type explicitly implements ISerializable.
- return property.GetValue(_control) is ISerializable;
+ // Otherwise we require the type explicitly implements ISerializable. Strangely, in the past this only
+ // worked off of the current value. If the current value was null checking it for ISerializable would always
+ // fail. This means properties would never load into a reference type property if it's current value was null.
+ //
+ // While we could always just check the property type for serializable this would break derived class scenarios
+ // where it adds ISerializable but the property type doesn't have it. In this scenario it would still not work
+ // if the value is null on load. Not enabling that scenario for now as it would require more refactoring.
+ return property.PropertyType.IsAssignableTo(typeof(ISerializable))
+ || property.GetValue(_control) is ISerializable;
  }
 
  /// <summary>
@@ -1185,12 +1191,27 @@ bool SetValue(PropertyDescriptor currentProperty, object data)
 
  // Resource property. We encode these as base 64 strings. To load them, we convert
  // to a binary blob and then de-serialize.
- byte[] bytes = Convert.FromBase64String(value);
- using MemoryStream stream = new MemoryStream(bytes);
+ using MemoryStream stream = new(Convert.FromBase64String(value), writable: false);
+ bool success = false;
+ object? deserialized = null;
+ try
+ {
+ BinaryFormattedObject format = new(stream, leaveOpen: true);
+ success = format.TryGetFrameworkObject(out deserialized);
+ }
+ catch (Exception ex) when (!ex.IsCriticalException())
+ {
+ }
+
 #pragma warning disable SYSLIB0011 // Type or member is obsolete
- currentProperty.SetValue(_control, new BinaryFormatter().Deserialize(stream));
-#pragma warning restore SYSLIB0011 // Type or member is obsolete
+ if (!success)
+ {
+ stream.Position = 0;
+ deserialized = new BinaryFormatter().Deserialize(stream);
+ }
+#pragma warning restore
 
+ currentProperty.SetValue(_control, deserialized);
  return true;
  }
 
@@ -1570,15 +1591,31 @@ internal void Save(IPropertyBag* propertyBag, BOOL clearDirty, BOOL saveAllPrope
  if (IsResourceProperty(currentProperty))
  {
  // Resource property. Save this to the bag as a 64bit encoded string.
- using MemoryStream stream = new MemoryStream();
+ using MemoryStream stream = new();
+ object sourceValue = currentProperty.GetValue(_control)!;
+ bool success = false;
+
+ try
+ {
+ success = BinaryFormatWriter.TryWriteFrameworkObject(stream, sourceValue);
+ }
+ catch (Exception ex) when (!ex.IsCriticalException())
+ {
+ Debug.Fail($"Failed to write with BinaryFormatWriter: {ex.Message}");
+ }
+
+ if (!success)
+ {
+ stream.SetLength(0);
+
 #pragma warning disable SYSLIB0011 // Type or member is obsolete
- new BinaryFormatter().Serialize(stream, props[i].GetValue(_control)!);
-#pragma warning restore SYSLIB0011 // Type or member is obsolete
- byte[] bytes = new byte[(int)stream.Length];
- stream.Position = 0;
- stream.Read(bytes, 0, bytes.Length);
- using VARIANT data = (VARIANT)new BSTR(Convert.ToBase64String(bytes));
- propertyBag->Write(props[i].Name, data);
+  new BinaryFormatter().Serialize(stream, sourceValue);
+#pragma warning restore
+ }
+
+ using VARIANT data = (VARIANT)new BSTR(Convert.ToBase64String(
+  new ReadOnlySpan<byte>(stream.GetBuffer(), 0, (int)stream.Length)));
+ propertyBag->Write(currentProperty.Name, data);
  continue;
  }
 
@@ -1597,7 +1634,7 @@ internal void Save(IPropertyBag* propertyBag, BOOL clearDirty, BOOL saveAllPrope
  byte[] data = (byte[])converter.ConvertTo(
  context: null,
  CultureInfo.InvariantCulture,
- props[i].GetValue(_control),
+ currentProperty.GetValue(_control),
  typeof(byte[]))!;
 
  value = Convert.ToBase64String(data);
@@ -1606,7 +1643,7 @@ internal void Save(IPropertyBag* propertyBag, BOOL clearDirty, BOOL saveAllPrope
  if (value is not null)
  {
  using VARIANT variant = (VARIANT)(new BSTR(value));
- fixed (char* pszPropName = props[i].Name)
+ fixed (char* pszPropName = currentProperty.Name)
  {
  propertyBag->Write(pszPropName, &variant);
  }