Skip to content

Commit 5a8d172

Browse files
JeremyKuhneJeremyKuhne
authored
Refine type binding for deserialization (#12950)
This change refines deserialization binding behavior for the BinaryFormatUtilites class. - Untyped requests must be requesting the object type (as the outer APIs all do) - Removes delegate in TypeBinder - TypeBinder returns typeof(object) for untyped requests - TypeBinder does not fall back to implicit INrbfSerializer binding when missing a resolver - Move JsonSerializer serialization to static interface - Reorder TryGetObjectFromJson to fail out appropriately when binding the root type - Reset the stream position in TryReadObjectFromStream - Create the binder up front in TryReadObjectFromStream to normalize usage - Don't allow implicit nullable conversion without an explicit binder - Write a number of new tests Plan is to continue to write tests with further test cleanup
1 parent 7c06baf commit 5a8d172

File tree

12 files changed

+556
-237
lines changed

12 files changed

+556
-237
lines changed

‎src/System.Private.Windows.Core/src/System/Private/Windows/JsonData.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,4 +151,7 @@ internal interface IJsonData
151151
/// </summary>
152152
[RequiresUnreferencedCode("Calls System.Text.Json.JsonSerializer.Deserialize<TValue>(ReadOnlySpan<Byte>, JsonSerializerOptions)")]
153153
object Deserialize();
154+
155+
[RequiresUnreferencedCode("Calls System.Text.Json.JsonSerializer.SerializeToUtf8Bytes<TValue>(TValue, JsonSerializerOptions)")]
156+
internal static IJsonData Create<T>(T data) => new JsonData<T>() { JsonBytes = JsonSerializer.SerializeToUtf8Bytes(data) };
154157
}

‎src/System.Private.Windows.Core/src/System/Private/Windows/Nrbf/INrbfSerializer.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ internal interface INrbfSerializer
3131
/// <para>
3232
/// This should only return <see langword="true"/> for types that are expected to be safe to deserialize. It is
3333
/// used to indicate that we should consider the type "corrupted" and should not make a second attempt through
34-
/// the <see cref="BinaryFormatter"/>. It is also used as the "blessed" list of types that we'll auto-bind when
34+
/// the <see cref="BinaryFormatter"/>. It is also used as the allowed list of types that we'll auto-bind when
3535
/// user type resolvers return <see langword="null"/>.
3636
/// </para>
3737
/// <para>

‎src/System.Private.Windows.Core/src/System/Private/Windows/Nrbf/SerializationRecordExtensions.cs

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -582,6 +582,11 @@ private static bool IsPrimitiveArrayRecord(SerializationRecord serializationReco
582582
/// If the data was supposed to be our <see cref="JsonData{T}"/>, but was serialized incorrectly.
583583
/// </exception>
584584
/// <exception cref="NotSupportedException">If an exception occurred while JSON deserializing.</exception>
585+
/// <devdoc>
586+
/// We don't try to resolve all types in the graph of a type when deserializing JSON as the default options
587+
/// should not present the same risks as BinaryFormatter deserialization. JSON binding is just for the root
588+
/// type.
589+
/// </devdoc>
585590
internal static (bool isJsonData, bool isValidType) TryGetObjectFromJson<T>(
586591
this SerializationRecord record,
587592
ITypeResolver resolver,
@@ -604,39 +609,38 @@ internal static (bool isJsonData, bool isValidType) TryGetObjectFromJson<T>(
604609
throw new SerializationException(SR.ClipboardOrDragDrop_JsonDeserializationFailed);
605610
}
606611

607-
if (typeof(T) == typeof(object))
612+
Type? boundType = resolver.BindToType(typeName);
613+
if (!boundType.IsAssignableTo(typeof(T)))
614+
{
615+
// Not the type the caller asked for.
616+
return (isJsonData: true, isValidType: false);
617+
}
618+
619+
if (boundType == typeof(object))
608620
{
609621
// Special case for deserializing to object. JsonSerializer.Deserializer<object> gives back a JsonElement.
610622
// We want to do this for the untyped APIs as downlevel apps would be able to read the object via
611623
// GetData (via the IObjectReference behavior in BinaryFormatter). Preventing it here would be difficult
612624
// to explain. Doing this also facilitates moving to the typed APIs for existing data that is JSON
613625
// serializable. You can simply switch to SerializeAsJson and know existing consumers will not be broken.
614626

615-
Type? getType = Type.GetType(
627+
boundType = Type.GetType(
616628
assemblyQualifiedTypeName,
617629
throwOnError: false);
618630

619-
// Full name didn't work, try the full
620-
getType ??= Type.GetType(
631+
// Fall back to just the full name.
632+
boundType ??= Type.GetType(
621633
typeName.FullName,
622-
throwOnError: false);
623-
624-
if (getType is not null)
625-
{
626-
Utf8JsonReader reader = new(byteData.GetArray());
627-
@object = (T?)JsonSerializer.Deserialize(ref reader, getType);
628-
return (isJsonData: true, isValidType: @object is not null);
629-
}
634+
throwOnError: true);
630635
}
631636

632-
if (!resolver.TryBindToType(typeName, out Type? resolvedType) || !resolvedType.IsAssignableTo(typeof(T)))
637+
// Let the original exception bubble up if deserialization fails.
638+
if (boundType is not null)
633639
{
634-
// Not the type the caller asked for.
635-
return (isJsonData: true, isValidType: false);
640+
Utf8JsonReader reader = new(byteData.GetArray());
641+
@object = (T?)JsonSerializer.Deserialize(ref reader, boundType);
636642
}
637643

638-
// Let the original exception bubble up if deserialization fails.
639-
@object = JsonSerializer.Deserialize<T>(byteData.GetArray());
640-
return (isJsonData: true, isValidType: true);
644+
return (isJsonData: true, isValidType: @object is not null);
641645
}
642646
}

‎src/System.Private.Windows.Core/src/System/Private/Windows/Ole/BinaryFormatUtilities.cs

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,8 @@ internal static bool TryReadObjectFromStream<T>(
7676
ref readonly DataRequest request,
7777
[NotNullWhen(true)] out T? @object)
7878
{
79+
Debug.Assert(request.TypedRequest || typeof(T) == typeof(object), "Untyped requests should always be asking for object");
80+
7981
@object = default;
8082
object? value;
8183

@@ -104,30 +106,38 @@ record = stream.DecodeNrbf();
104106
{
105107
// Allow falling through for unsupported Nrbf data (such as non-zero based arrays).
106108
}
109+
finally
110+
{
111+
stream.Position = startPosition;
112+
}
107113

108-
TypeBinder<TNrbfSerializer>? binder = null;
114+
TypeBinder<TNrbfSerializer> binder = new(typeof(T), in request);
109115

110116
if (record is not null)
111117
{
112118
// Try our implicit deserialization.
113-
if (TNrbfSerializer.TryBindToType(record.TypeName, out Type? type)
114-
&& type.IsAssignableTo(typeof(T)))
119+
if (TNrbfSerializer.TryBindToType(record.TypeName, out Type? type))
115120
{
121+
if (request.TypedRequest
122+
// If we can't match the root exactly, then we fall back to the binder.
123+
&& !(type == typeof(T) || binder.BindToType(record.TypeName).IsAssignableTo(typeof(T))))
124+
{
125+
return false;
126+
}
127+
116128
if (TNrbfSerializer.TryGetObject(record, out value))
117129
{
118130
@object = (T)value;
119131
return true;
120132
}
121-
else if (TNrbfSerializer.IsFullySupportedType(typeof(T)))
133+
else if (TNrbfSerializer.IsFullySupportedType(type))
122134
{
123135
// The serializer fully supports this type, but can't deserialize it.
124136
// Don't let it fall through to the BinaryFormatter.
125137
return false;
126138
}
127139
}
128140

129-
binder = new(typeof(T), in request);
130-
131141
if (type is null)
132142
{
133143
// Serializer didn't recognize the type, look for and deserialize a JSON object.
@@ -139,11 +149,9 @@ record = stream.DecodeNrbf();
139149
}
140150

141151
// JSON type info is nested, so this has to come after the JSON attempt.
142-
if (request.TypedRequest)
152+
if (request.TypedRequest && !typeof(T).Matches(record.TypeName, TypeNameComparison.AllButAssemblyVersion))
143153
{
144-
if (!(typeof(T).Matches(record.TypeName, TypeNameComparison.AllButAssemblyVersion)
145-
|| (binder.TryBindToType(record.TypeName, out Type? resolvedType)
146-
&& resolvedType.IsAssignableTo(typeof(T)))))
154+
if (!binder.BindToType(record.TypeName).IsAssignableTo(typeof(T)))
147155
{
148156
// Typed request where the root type is not what was requested.
149157
// Untyped requests are allowed to deserialize any type.
@@ -181,9 +189,8 @@ record = stream.DecodeNrbf();
181189

182190
// If we want to attempt our own general NRBF deserialization, we would do it here.
183191
// If we do, we should never fall back to the BinaryFormatter if the binder fails
184-
// to bind the type.
192+
// to bind a type.
185193

186-
stream.Position = startPosition;
187194
binder ??= new(typeof(T), in request);
188195

189196
#pragma warning disable SYSLIB0011, SYSLIB0050 // Type or member is obsolete
@@ -203,6 +210,10 @@ record = stream.DecodeNrbf();
203210
// Rethrow the inner exception to allow the exception to flow up through TryGetHGLOBALData.
204211
throw nse;
205212
}
213+
finally
214+
{
215+
stream.Position = startPosition;
216+
}
206217
#pragma warning restore CA2300
207218
#pragma warning restore CA2302
208219
#pragma warning restore SYSLIB0050, SYSLIB0011

‎src/System.Private.Windows.Core/src/System/Private/Windows/Ole/DataObjectCore.cs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4-
using System.Text.Json;
5-
64
namespace System.Private.Windows.Ole;
75

86
internal static unsafe class DataObjectCore<TDataObject>
@@ -31,6 +29,6 @@ internal static IJsonData TryJsonSerialize<T>(string format, T data)
3129
throw new ArgumentException(SR.ClipboardOrDragDrop_CannotJsonSerializeDataObject, nameof(data));
3230
}
3331

34-
return new JsonData<T>() { JsonBytes = JsonSerializer.SerializeToUtf8Bytes(data) };
32+
return IJsonData.Create(data);
3533
}
3634
}

‎src/System.Private.Windows.Core/src/System/Private/Windows/Ole/TypeBinder.cs

Lines changed: 31 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,8 @@ internal sealed class TypeBinder<TNrbfSerializer> : SerializationBinder, ITypeRe
3232
where TNrbfSerializer : INrbfSerializer
3333
{
3434
private readonly Type _rootType;
35-
private Func<TypeName, Type?>? _resolver;
35+
private readonly Func<TypeName, Type?>? _resolver;
3636
private readonly bool _isTypedRequest;
37-
private readonly bool _hasCustomResolver;
3837

3938
private Dictionary<FullyQualifiedTypeName, TypeName>? _cachedTypeNames;
4039

@@ -45,31 +44,14 @@ internal sealed class TypeBinder<TNrbfSerializer> : SerializationBinder, ITypeRe
4544
/// <param name="rootType"><see cref="Type"/> that the user expects to read from the binary formatted stream.</param>
4645
public TypeBinder(Type rootType, ref readonly DataRequest request)
4746
{
48-
Debug.Assert(
49-
request.TypedRequest || (!request.TypedRequest && request.Resolver is null),
50-
"Untyped methods should not provide a resolver.");
47+
Debug.Assert(request.TypedRequest || request.Resolver is null, "Untyped methods should not provide a resolver.");
48+
Debug.Assert(request.TypedRequest || rootType == typeof(object), "Untyped requests should always be asking for object");
5149

5250
_isTypedRequest = request.TypedRequest;
53-
_hasCustomResolver = request.Resolver is not null;
5451
_rootType = rootType;
5552
_resolver = request.Resolver;
5653
}
5754

58-
private Func<TypeName, Type?> Resolver
59-
{
60-
get
61-
{
62-
// Lazily create a default resolver when needed to potentially avoid the cost of parsing the TypeName.
63-
return _resolver ??= CreateDefaultResolver(_rootType);
64-
65-
static Func<TypeName, Type?> CreateDefaultResolver(Type type)
66-
{
67-
TypeName knownType = type.ToTypeName();
68-
return typeName => knownType.Matches(typeName, TypeNameComparison.AllButAssemblyVersion) ? type : null;
69-
}
70-
}
71-
}
72-
7355
public override Type? BindToType(string assemblyName, string typeName)
7456
{
7557
ArgumentException.ThrowIfNullOrWhiteSpace(assemblyName);
@@ -86,7 +68,7 @@ public TypeBinder(Type rootType, ref readonly DataRequest request)
8668
}
8769

8870
// Should never fall through to the BinaryFormatter from a typed API without an explicit resolver.
89-
if (!_hasCustomResolver)
71+
if (_resolver is null)
9072
{
9173
throw new InvalidOperationException();
9274
}
@@ -106,23 +88,48 @@ public TypeBinder(Type rootType, ref readonly DataRequest request)
10688
public Type BindToType(TypeName typeName) => TryBindToType(typeName, out Type? type)
10789
? type
10890
: throw new NotSupportedException(string.Format(
109-
_hasCustomResolver ? SR.ClipboardOrDragDrop_TypedAPI_InvalidResolver : SR.ClipboardOrDragDrop_UseTypedAPI,
91+
_resolver is null ? SR.ClipboardOrDragDrop_UseTypedAPI : SR.ClipboardOrDragDrop_TypedAPI_InvalidResolver,
11092
typeName.AssemblyQualifiedName));
11193

11294
public bool TryBindToType(TypeName typeName, [NotNullWhen(true)] out Type? type)
11395
{
11496
ArgumentNullException.ThrowIfNull(typeName);
11597

98+
if (!_isTypedRequest)
99+
{
100+
type = typeof(object);
101+
return true;
102+
}
103+
104+
if (_resolver is null)
105+
{
106+
if (_rootType.Matches(typeName, TypeNameComparison.AllButAssemblyVersion))
107+
{
108+
type = _rootType;
109+
return true;
110+
}
111+
112+
// If we fall back here to the TNrbfSerializer all types would match for the root. This has the side
113+
// effect of asking for `int?` and matching `int` data. We need to do `IsAssignableTo` for resolved
114+
// types so we can allow resolvers to bind to derived classes- `int` is assignable to `int?`.
115+
//
116+
// Asking for `List<int?>` will not match `List<int>` data, even though the CoreNrbfSerializer supports
117+
// `List<int>`. `List<int>` is not assignable to `List<int?>`.
118+
type = null;
119+
return false;
120+
}
121+
116122
try
117123
{
118-
type = Resolver(typeName);
124+
type = _resolver(typeName);
119125
}
120126
catch (Exception ex) when (!ex.IsCriticalException())
121127
{
122128
type = null;
123129
return false;
124130
}
125131

132+
// Explicit resolver returned null, fall back to implicit support.
126133
if (type is null
127134
&& _rootType != typeof(object)
128135
&& !_rootType.IsInterface

‎src/System.Private.Windows.Core/tests/System.Private.Windows.Core.Tests/System/Private/Windows/Ole/BinaryFormatUtilitesTestsBase.cs

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,35 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33

44
using System.Diagnostics.CodeAnalysis;
5+
using System.Private.Windows.BinaryFormat;
56
using System.Reflection.Metadata;
67

78
namespace System.Private.Windows.Ole.Tests;
89

910
public abstract class BinaryFormatUtilitesTestsBase : IDisposable
1011
{
12+
public enum DataType
13+
{
14+
Json,
15+
BinaryFormat
16+
}
17+
18+
public MemoryStream CreateStream<T>(DataType dataType, T data) where T : notnull
19+
{
20+
MemoryStream stream = new();
21+
if (dataType == DataType.Json)
22+
{
23+
BinaryFormatWriter.TryWriteJsonData(stream, IJsonData.Create(data));
24+
}
25+
else
26+
{
27+
WriteObjectToStream(stream, data, "test");
28+
}
29+
30+
stream.Position = 0;
31+
return stream;
32+
}
33+
1134
protected MemoryStream Stream { get; }
1235

1336
public BinaryFormatUtilitesTestsBase() => Stream = new();
@@ -32,7 +55,7 @@ protected bool TryReadObjectFromStream<T>(out T? @object)
3255
return TryReadObjectFromStream(Stream, untypedRequest: true, "test", resolver: null, out @object);
3356
}
3457

35-
protected bool TryReadRestrictedObjectFromStream<T>(out T? @object)
58+
protected bool TryReadPredefinedObjectFromStream<T>(out T? @object)
3659
{
3760
Stream.Position = 0;
3861
return TryReadObjectFromStream(Stream, untypedRequest: true, DataFormatNames.String, resolver: null, out @object);
@@ -56,7 +79,7 @@ protected bool TryReadObjectFromStream<T>(Func<TypeName, Type>? resolver, out T?
5679
return TryReadObjectFromStream(Stream, untypedRequest: false, "test", resolver, out @object);
5780
}
5881

59-
protected bool ReadRestrictedObjectFromStream<T>(Func<TypeName, Type>? resolver, out T? @object)
82+
protected bool ReadPredefinedObjectFromStream<T>(Func<TypeName, Type>? resolver, out T? @object)
6083
{
6184
return TryReadObjectFromStream(Stream, untypedRequest: false, DataFormatNames.String, resolver, out @object);
6285
}
@@ -69,11 +92,11 @@ protected bool RoundTripObject<T>(object value, out T? @object)
6992
return TryReadObjectFromStream(out @object);
7093
}
7194

72-
protected bool RoundTripObject_RestrictedFormat<T>(object value, out T? @object)
95+
protected bool RoundTripObject_PredefinedFormat<T>(object value, out T? @object)
7396
{
7497
// This is equivalent to SetData/GetData methods using registered OLE formats, resolves only the known types
7598
WriteObjectToStream(value, restrictSerialization: true);
76-
return TryReadRestrictedObjectFromStream(out @object);
99+
return TryReadPredefinedObjectFromStream(out @object);
77100
}
78101

79102
protected bool RoundTripOfType<T>(object value, out T? @object)
@@ -84,19 +107,19 @@ protected bool RoundTripOfType<T>(object value, out T? @object)
84107
return TryReadObjectFromStream(NotSupportedResolver, out @object);
85108
}
86109

87-
protected bool RoundTripOfType_RestrictedFormat<T>(object value, out T? @object)
110+
protected bool RoundTripOfType_PredefinedFormat<T>(object value, out T? @object)
88111
{
89-
// This is equivalent to SetData/TryGetData<T> methods using OLE formats. Deserialization is restricted
112+
// This is equivalent to SetData/TryGetData<T> methods using OLE formats. Deserialization is Predefined
90113
// to known types.
91114
WriteObjectToStream(value, restrictSerialization: true);
92115
Stream.Position = 0;
93-
return ReadRestrictedObjectFromStream(NotSupportedResolver, out @object);
116+
return ReadPredefinedObjectFromStream(NotSupportedResolver, out @object);
94117
}
95118

96119
protected bool RoundTripOfType<T>(object value, Func<TypeName, Type>? resolver, out T? @object)
97120
{
98121
// This is equivalent to SetData/TryGetData<T> methods using unbounded formats,
99-
// serialization is restricted by the resolver and BinaryFormat AppContext switches.
122+
// serialization is Predefined by the resolver and BinaryFormat AppContext switches.
100123
WriteObjectToStream(value);
101124
return TryReadObjectFromStream(resolver, out @object);
102125
}

0 commit comments

Comments
 (0)