Minor refactoring in DataObject-related code (dotnet#12642)

Tanya-Solyanik · web-flow · commit 39c6b28eaf96 · 2024-12-13T15:33:17.000-08:00
Factored out code that reads formats that allow only restricted deserialization to run, such as OLE formats.
diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/Internal/TypeExtensions.cs b/src/System.Windows.Forms/src/System/Windows/Forms/Internal/TypeExtensions.cs
@@ -235,8 +235,10 @@ static bool AssemblyNamesMatch(AssemblyNameInfo? name1, AssemblyNameInfo? name2)
     }
 
     /// <summary>
-    ///  Convert <paramref name="type"/> to <see cref="TypeName"/>. This method removes nullability wrapper
-    ///  from the top level type only because <see cref="TypeName"/> in the serialization root record is not nullable.
+    ///  Convert <paramref name="type"/> to <see cref="TypeName"/>. Take into account type forwarding in order
+    ///  to create <see cref="TypeName"/> compatible with the type names serialized to the binary format.This
+    ///  method removes nullability wrapper from the top level type only because <see cref="TypeName"/> in the
+    ///  serialization root record is not nullable, but the generic types could be nullable.
     /// </summary>
     public static TypeName ToTypeName(this Type type)
     {
diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/OLE/DataObject.Composition.BinaryFormatUtilities.cs b/src/System.Windows.Forms/src/System/Windows/Forms/OLE/DataObject.Composition.BinaryFormatUtilities.cs
@@ -66,31 +66,24 @@ internal static void WriteObjectToStream(MemoryStream stream, object data, bool
 
             internal static object? ReadObjectFromStream<T>(
                 MemoryStream stream,
-                bool restrictDeserialization,
                 Func<TypeName, Type>? resolver,
                 bool legacyMode)
             {
                 long startPosition = stream.Position;
                 SerializationRecord? record;
-
                 SerializationBinder binder = new Binder(typeof(T), resolver, legacyMode);
-
                 IReadOnlyDictionary<SerializationRecordId, SerializationRecord> recordMap;
+
                 try
                 {
                     record = stream.Decode(out recordMap);
                 }
                 catch (Exception ex) when (!ex.IsCriticalException())
                 {
                     // Couldn't parse for some reason, let BinaryFormatter handle the legacy invocation.
-                    // The types APIs can't compare the specified type when the root record is not available.
+                    // The typed APIs can't compare the specified type when the root record is not available.
                     if (legacyMode && LocalAppContextSwitches.ClipboardDragDropEnableUnsafeBinaryFormatterSerialization)
                     {
-                        if (restrictDeserialization)
-                        {
-                            throw new RestrictedTypeDeserializationException(SR.UnexpectedClipboardType);
-                        }
-
                         stream.Position = startPosition;
                         return ReadObjectWithBinaryFormatter<T>(stream, binder);
                     }
@@ -108,7 +101,8 @@ record = stream.Decode(out recordMap);
 
                 // For the new TryGet APIs, ensure that the stream contains the requested type,
                 // or type that can be assigned to the requested type.
-                if (!legacyMode && !typeof(T).MatchExceptAssemblyVersion(record.TypeName))
+                Type type = typeof(T);
+                if (!legacyMode && !type.MatchExceptAssemblyVersion(record.TypeName))
                 {
 #if false // TODO (TanyaSo): - modify TryGetObjectFromJson to take a resolver and rename to HasJsonData???
                     // Return true if the payload contains valid JsonData<T> struct, type matches or not
@@ -119,7 +113,7 @@ record = stream.Decode(out recordMap);
                     }
 #endif
 
-                    if (!TypeNameIsAssignableToType(record.TypeName, typeof(T), (ITypeResolver)binder))
+                    if (!TypeNameIsAssignableToType(record.TypeName, type, (ITypeResolver)binder))
                     {
                         // If clipboard contains an exception from SetData, we will get its message and throw.
                         if (record.TypeName.FullName == typeof(NotSupportedException).FullName
@@ -138,11 +132,6 @@ record = stream.Decode(out recordMap);
                     return value;
                 }
 
-                if (restrictDeserialization)
-                {
-                    throw new RestrictedTypeDeserializationException(SR.UnexpectedClipboardType);
-                }
-
                 if (!LocalAppContextSwitches.ClipboardDragDropEnableUnsafeBinaryFormatterSerialization)
                 {
                     throw new NotSupportedException(string.Format(
@@ -170,6 +159,47 @@ record = stream.Decode(out recordMap);
                 return ReadObjectWithBinaryFormatter<T>(stream, binder);
             }
 
+            internal static object? ReadRestrictedObjectFromStream<T>(
+                MemoryStream stream,
+                Func<TypeName, Type>? resolver,
+                bool legacyMode)
+            {
+                long startPosition = stream.Position;
+                SerializationRecord? record;
+
+                try
+                {
+                    record = stream.Decode();
+                }
+                catch (Exception ex) when (!ex.IsCriticalException())
+                {
+                    throw new RestrictedTypeDeserializationException(SR.UnexpectedClipboardType);
+                }
+
+                // For the new TryGet APIs, ensure that the stream contains the requested type,
+                // or type that can be assigned to the requested type.
+                Type type = typeof(T);
+                if (!legacyMode && !type.MatchExceptAssemblyVersion(record.TypeName))
+                {
+                    if (!TypeNameIsAssignableToType(record.TypeName, type, new Binder(type, resolver, legacyMode)))
+                    {
+                        // If clipboard contains an exception from SetData, we will get its message and throw.
+                        if (record.TypeName.FullName == typeof(NotSupportedException).FullName
+                            && record.TryGetNotSupportedException(out object? @object)
+                            && @object is NotSupportedException exception)
+                        {
+                            throw new NotSupportedException(exception.Message);
+                        }
+
+                        return null;
+                    }
+                }
+
+                return record.TryGetCommonObject(out object? value)
+                    ? value
+                    : throw new RestrictedTypeDeserializationException(SR.UnexpectedClipboardType);
+            }
+
             private static bool TypeNameIsAssignableToType(TypeName typeName, Type type, ITypeResolver resolver)
             {
                 try
diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/OLE/DataObject.Composition.NativeToWinFormsAdapter.cs b/src/System.Windows.Forms/src/System/Windows/Forms/OLE/DataObject.Composition.NativeToWinFormsAdapter.cs
@@ -133,7 +133,9 @@ private static bool TryGetDataFromHGLOBAL<T>(
                     MemoryStream stream = ReadByteStreamFromHGLOBAL(hglobal, out bool isSerializedObject);
                     return !isSerializedObject
                         ? stream
-                        : BinaryFormatUtilities.ReadObjectFromStream<T>(stream, restrictDeserialization, resolver, legacyMode);
+                        : restrictDeserialization
+                            ? BinaryFormatUtilities.ReadRestrictedObjectFromStream<T>(stream, resolver, legacyMode)
+                            : BinaryFormatUtilities.ReadObjectFromStream<T>(stream, resolver, legacyMode);
                 }
             }
 
diff --git a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/BinaryFormatUtilitiesTests.cs b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/BinaryFormatUtilitiesTests.cs
@@ -22,16 +22,36 @@ public partial class BinaryFormatUtilitiesTests : IDisposable
     private void WriteObjectToStream(object value, bool restrictSerialization = false) =>
         Utilities.WriteObjectToStream(_stream, value, restrictSerialization);
 
-    private object? ReadObjectFromStream(bool restrictDeserialization = false)
+    private object? ReadObjectFromStream()
     {
         _stream.Position = 0;
-        return Utilities.ReadObjectFromStream<object>(_stream, restrictDeserialization, resolver: null, legacyMode: true);
+        return Utilities.ReadObjectFromStream<object>(_stream, resolver: null, legacyMode: true);
+    }
+
+    private object? ReadRestrictedObjectFromStream()
+    {
+        _stream.Position = 0;
+        return Utilities.ReadRestrictedObjectFromStream<object>(_stream, resolver: null, legacyMode: true);
     }
 
     private object? ReadObjectFromStream<T>(bool restrictDeserialization, Func<TypeName, Type>? resolver)
     {
         _stream.Position = 0;
-        return Utilities.ReadObjectFromStream<T>(_stream, restrictDeserialization, resolver, legacyMode: false);
+        return restrictDeserialization
+            ? Utilities.ReadRestrictedObjectFromStream<T>(_stream, resolver, legacyMode: false)
+            : Utilities.ReadObjectFromStream<T>(_stream, resolver, legacyMode: false);
+    }
+
+    private object? ReadObjectFromStream<T>(Func<TypeName, Type>? resolver)
+    {
+        _stream.Position = 0;
+        return Utilities.ReadObjectFromStream<T>(_stream, resolver, legacyMode: false);
+    }
+
+    private object? ReadRestrictedObjectFromStream<T>(Func<TypeName, Type>? resolver)
+    {
+        _stream.Position = 0;
+        return Utilities.ReadRestrictedObjectFromStream<T>(_stream, resolver, legacyMode: false);
     }
 
     private object? RoundTripObject(object value)
@@ -46,31 +66,31 @@ private void WriteObjectToStream(object value, bool restrictSerialization = fals
     {
         // This is equivalent to SetData/GetData methods using registered OLE formats, resolves only the known types
         WriteObjectToStream(value, restrictSerialization: true);
-        return ReadObjectFromStream(restrictDeserialization: true);
+        return ReadRestrictedObjectFromStream();
     }
 
     private object? RoundTripOfType<T>(object value)
     {
         // This is equivalent to SetData/TryGetData<T> methods using unbounded OLE formats,
         // and works with the BinaryFormat AppContext switches.
         WriteObjectToStream(value);
-        return ReadObjectFromStream<T>(restrictDeserialization: false, NotSupportedResolver);
+        return ReadObjectFromStream<T>(NotSupportedResolver);
     }
 
     private object? RoundTripOfType_RestrictedFormat<T>(object value)
     {
         // This is equivalent to SetData/TryGetData<T> methods using OLE formats. Deserialization is restricted
         // to known types.
         WriteObjectToStream(value, restrictSerialization: true);
-        return ReadObjectFromStream<T>(restrictDeserialization: true, NotSupportedResolver);
+        return ReadRestrictedObjectFromStream<T>(NotSupportedResolver);
     }
 
     private object? RoundTripOfType<T>(object value, Func<TypeName, Type>? resolver)
     {
         // This is equivalent to SetData/TryGetData<T> methods using unbounded formats,
         // serialization is restricted by the resolver and BinaryFormat AppContext switches.
         WriteObjectToStream(value);
-        return ReadObjectFromStream<T>(restrictDeserialization: false, resolver);
+        return ReadObjectFromStream<T>(resolver);
     }
 
     // Primitive types as defined by the NRBF spec.
@@ -311,14 +331,14 @@ public void RoundTrip_RestrictedFormat_ImageList()
     public void RoundTrip_Bitmap()
     {
         using Bitmap value = new(10, 10);
-        RoundTripObject(value).Should().BeOfType<Bitmap>().Subject.Size.Should().Be(value.Size);
+        RoundTripObject(value).Should().BeOfType<Bitmap>().Which.Size.Should().Be(value.Size);
     }
 
     [Fact]
     public void RoundTrip_RestrictedFormat_Bitmap()
     {
         using Bitmap value = new(10, 10);
-        RoundTripObject_RestrictedFormat(value).Should().BeOfType<Bitmap>().Subject.Size.Should().Be(value.Size);
+        RoundTripObject_RestrictedFormat(value).Should().BeOfType<Bitmap>().Which.Size.Should().Be(value.Size);
     }
 
     [Theory]
@@ -402,12 +422,12 @@ public void RoundTripOfType_Unsupported()
             ReadAndValidate();
         }
 
-        Action read = () => ReadObjectFromStream<List<object>>(restrictDeserialization: false, ObjectListResolver);
+        Action read = () => ReadObjectFromStream<List<object>>(ObjectListResolver);
         read.Should().Throw<NotSupportedException>();
 
         void ReadAndValidate()
         {
-            var result = ReadObjectFromStream<List<object>>(restrictDeserialization: false, ObjectListResolver)
+            var result = ReadObjectFromStream<List<object>>(ObjectListResolver)
                 .Should().BeOfType<List<object>>().Subject;
             result.Count.Should().Be(1);
             result[0].Should().Be("text");
@@ -450,10 +470,10 @@ public void RoundTripOfType_RestrictedFormat_AsUnmatchingType_Simple()
         // but in this case requested type will not match the payload type.
         WriteObjectToStream(value);
 
-        ReadObjectFromStream<Control>(restrictDeserialization: true, NotSupportedResolver).Should().BeNull();
+        ReadRestrictedObjectFromStream<Control>(NotSupportedResolver).Should().BeNull();
 
         using BinaryFormatterFullCompatScope scope = new();
-        ReadObjectFromStream<Control>(restrictDeserialization: true, NotSupportedResolver).Should().BeNull();
+        ReadRestrictedObjectFromStream<Control>(NotSupportedResolver).Should().BeNull();
     }
 
     [Fact]
@@ -471,7 +491,7 @@ public void RoundTripOfType_RestrictedFormat_intNullableArray_NotSupportedResolv
 
         using BinaryFormatterFullCompatScope scope = new();
         WriteObjectToStream(value);
-        Action read = () => ReadObjectFromStream<int?[]>(restrictDeserialization: true, NotSupportedResolver);
+        Action read = () => ReadRestrictedObjectFromStream<int?[]>(NotSupportedResolver);
 
         // nullable struct requires a custom resolver.
         // RestrictedTypeDeserializationException
@@ -485,10 +505,11 @@ public void RoundTripOfType_intNullableArray_NotSupportedResolver()
 
         using BinaryFormatterFullCompatScope scope = new();
         WriteObjectToStream(value);
-        Action read = () => ReadObjectFromStream<int?[]>(restrictDeserialization: false, NotSupportedResolver);
+        Action read = () => ReadObjectFromStream<int?[]>(NotSupportedResolver);
 
         // nullable struct requires a custom resolver.
-        read.Should().Throw<SerializationException>();
+        // This is either NotSupportedException of RestrictedTypeDeserializationException, depending on format.
+        read.Should().Throw<Exception>();
     }
 
     [Theory]
@@ -507,7 +528,7 @@ public void RoundTripOfType_OffsetArray_NotSupportedResolver(bool restrictDeseri
         WriteObjectToStream(value);
         Action read = () => ReadObjectFromStream<uint[,]>(restrictDeserialization, NotSupportedResolver);
 
-        read.Should().Throw<NotSupportedException>();
+        read.Should().Throw<Exception>();
     }
 
     [Fact]
@@ -583,7 +604,7 @@ public void RoundTripOfType_TestData_InvalidResolver()
         WriteObjectToStream(value);
 
         // Resolver that returns a null is blocked in our SerializationBinder wrapper.
-        Action read = () => ReadObjectFromStream<TestData>(restrictDeserialization: false, InvalidResolver);
+        Action read = () => ReadObjectFromStream<TestData>(InvalidResolver);
 
         read.Should().Throw<SerializationException>();
 
@@ -649,7 +670,6 @@ public void ReadFontSerializedOnNet481()
             stream.Position = 0;
             Action getData = () => Utilities.ReadObjectFromStream<object>(
                stream,
-               restrictDeserialization: false,
                resolver: null,
                legacyMode: true);
 
@@ -664,7 +684,6 @@ public void ReadFontSerializedOnNet481()
         stream.Position = 0;
         var result = Utilities.ReadObjectFromStream<object>(
            stream,
-           restrictDeserialization: false,
            resolver: null,
            legacyMode: true).Should().BeOfType<Font>().Subject;
 
@@ -679,7 +698,6 @@ static void TryGetData(MemoryStream stream)
             stream.Position = 0;
             var result = Utilities.ReadObjectFromStream<Font>(
                 stream,
-                restrictDeserialization: false,
                 resolver: FontResolver,
                 legacyMode: false).Should().BeOfType<Font>().Subject;
 
@@ -721,7 +739,7 @@ public void Sample_GetData_UseBinaryFormatter()
 
         // legacyMode == true follows the GetData path.
         _stream.Position = 0;
-        Utilities.ReadObjectFromStream<MyClass1>(_stream, restrictDeserialization: false, resolver: null, legacyMode: true)
+        Utilities.ReadObjectFromStream<MyClass1>(_stream, resolver: null, legacyMode: true)
             .Should().BeEquivalentTo(value);
     }
 
@@ -736,7 +754,7 @@ public void Sample_GetData_UseNrbfDeserialize()
 
         // This works because GetData falls back to the BinaryFormatter deserializer, NRBF deserializer fails because it requires a resolver.
         _stream.Position = 0;
-        Utilities.ReadObjectFromStream<MyClass1>(_stream, restrictDeserialization: false, resolver: null, legacyMode: true)
+        Utilities.ReadObjectFromStream<MyClass1>(_stream, resolver: null, legacyMode: true)
             .Should().BeEquivalentTo(value);
     }
 

Original file line number	Diff line number	Diff line change
`@@ -235,8 +235,10 @@ static bool AssemblyNamesMatch(AssemblyNameInfo? name1, AssemblyNameInfo? name2)`
`235`	`235`	`}`
`236`	`236`
`237`	`237`	`/// <summary>`
`238`		`- /// Convert <paramref name="type"/> to <see cref="TypeName"/>. This method removes nullability wrapper`
`239`		`- /// from the top level type only because <see cref="TypeName"/> in the serialization root record is not nullable.`
	`238`	`+ /// Convert <paramref name="type"/> to <see cref="TypeName"/>. Take into account type forwarding in order`
	`239`	`+ /// to create <see cref="TypeName"/> compatible with the type names serialized to the binary format.This`
	`240`	`+ /// method removes nullability wrapper from the top level type only because <see cref="TypeName"/> in the`
	`241`	`+ /// serialization root record is not nullable, but the generic types could be nullable.`
`240`	`242`	`/// </summary>`
`241`	`243`	`public static TypeName ToTypeName(this Type type)`
`242`	`244`	`{`
Original file line number	Diff line number	Diff line change
`@@ -133,7 +133,9 @@ private static bool TryGetDataFromHGLOBAL<T>(`
`133`	`133`	`MemoryStream stream = ReadByteStreamFromHGLOBAL(hglobal, out bool isSerializedObject);`
`134`	`134`	`return !isSerializedObject`
`135`	`135`	`? stream`
`136`		`- : BinaryFormatUtilities.ReadObjectFromStream<T>(stream, restrictDeserialization, resolver, legacyMode);`
	`136`	`+ : restrictDeserialization`
	`137`	`+ ? BinaryFormatUtilities.ReadRestrictedObjectFromStream<T>(stream, resolver, legacyMode)`
	`138`	`+ : BinaryFormatUtilities.ReadObjectFromStream<T>(stream, resolver, legacyMode);`
`137`	`139`	`}`
`138`	`140`	`}`
`139`	`141`