Skip to content
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

Remove hardcoded limit in deserialization constructor arguments #75982

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/libraries/System.Text.Json/Common/JsonConstants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@ namespace System.Text.Json
{
internal static partial class JsonConstants
{
// The maximum number of parameters a constructor can have where it can be supported by the serializer.
public const int MaxParameterCount = 64;

// Standard format for double and single on non-inbox frameworks.
public const string DoubleFormatString = "G17";
public const string SingleFormatString = "G9";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -951,11 +951,6 @@ private static string GetParameterizedCtorInvocationFunc(TypeGenerationSpec type
int paramCount = parameters.Length;
Debug.Assert(paramCount != 0);

if (paramCount > JsonConstants.MaxParameterCount)
{
return "null";
}

const string ArgsVarName = "args";
int lastIndex = paramCount - 1;

Expand Down
3 changes: 0 additions & 3 deletions src/libraries/System.Text.Json/src/Resources/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -485,9 +485,6 @@
<data name="ConstructorParamIncompleteBinding" xml:space="preserve">
<value>Each parameter in the deserialization constructor on type '{0}' must bind to an object property or field on deserialization. Each parameter name must match with a property or field on the object. Fields are only considered when 'JsonSerializerOptions.IncludeFields' is enabled. The match can be case-insensitive.</value>
</data>
<data name="ConstructorMaxOf64Parameters" xml:space="preserve">
<value>The deserialization constructor on type '{0}' may not have more than 64 parameters for deserialization.</value>
</data>
<data name="ObjectWithParameterizedCtorRefMetadataNotSupported" xml:space="preserve">
<value>Reference metadata is not supported when deserializing constructor parameters. See type '{0}'.</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,13 @@ protected sealed override bool ReadAndCacheConstructorArgument(scoped ref ReadSt

protected sealed override object CreateObject(ref ReadStackFrame frame)
{
object[] arguments = (object[])frame.CtorArgumentState!.Arguments;
frame.CtorArgumentState.Arguments = null!;
Debug.Assert(frame.CtorArgumentState != null);
Debug.Assert(frame.JsonTypeInfo.CreateObjectWithArgs != null);

var createObject = (Func<object[], T>?)frame.JsonTypeInfo.CreateObjectWithArgs;
object[] arguments = (object[])frame.CtorArgumentState.Arguments;
frame.CtorArgumentState.Arguments = null!;

if (createObject == null)
{
// This means this constructor has more than 64 parameters.
ThrowHelper.ThrowNotSupportedException_ConstructorMaxOf64Parameters(TypeToConvert);
}
Func<object[], T> createObject = (Func<object[], T>)frame.JsonTypeInfo.CreateObjectWithArgs;

object obj = createObject(arguments);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ internal abstract class MemberAccessor
public abstract Func<object>? CreateConstructor(
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type classType);

public abstract Func<object[], T>? CreateParameterizedConstructor<T>(ConstructorInfo constructor);
public abstract Func<object[], T> CreateParameterizedConstructor<T>(ConstructorInfo constructor);

public abstract JsonTypeInfo.ParameterizedConstructorDelegate<T, TArg0, TArg1, TArg2, TArg3>?
CreateParameterizedConstructor<T, TArg0, TArg1, TArg2, TArg3>(ConstructorInfo constructor);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public override Func<IEnumerable<TElement>, TCollection> CreateImmutableEnumerab
=> s_cache.GetOrAdd((nameof(CreateImmutableEnumerableCreateRangeDelegate), typeof((TCollection, TElement)), null),
static (_) => s_sourceAccessor.CreateImmutableEnumerableCreateRangeDelegate<TCollection, TElement>());

public override Func<object[], T>? CreateParameterizedConstructor<T>(ConstructorInfo constructor)
public override Func<object[], T> CreateParameterizedConstructor<T>(ConstructorInfo constructor)
=> s_cache.GetOrAdd((nameof(CreateParameterizedConstructor), typeof(T), constructor), static key => s_sourceAccessor.CreateParameterizedConstructor<T>((ConstructorInfo)key.member!));

public override JsonTypeInfo.ParameterizedConstructorDelegate<T, TArg0, TArg1, TArg2, TArg3>? CreateParameterizedConstructor<T, TArg0, TArg1, TArg2, TArg3>(ConstructorInfo constructor)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,13 @@ internal sealed class ReflectionEmitMemberAccessor : MemberAccessor

generator.Emit(OpCodes.Ret);

return (Func<object>)dynamicMethod.CreateDelegate(typeof(Func<object>));
return CreateDelegate<Func<object>>(dynamicMethod);
}

public override Func<object[], T>? CreateParameterizedConstructor<T>(ConstructorInfo constructor) =>
public override Func<object[], T> CreateParameterizedConstructor<T>(ConstructorInfo constructor) =>
CreateDelegate<Func<object[], T>>(CreateParameterizedConstructor(constructor));

private static DynamicMethod? CreateParameterizedConstructor(ConstructorInfo constructor)
private static DynamicMethod CreateParameterizedConstructor(ConstructorInfo constructor)
{
Type? type = constructor.DeclaringType;

Expand All @@ -76,11 +76,6 @@ internal sealed class ReflectionEmitMemberAccessor : MemberAccessor
ParameterInfo[] parameters = constructor.GetParameters();
int parameterCount = parameters.Length;

if (parameterCount > JsonConstants.MaxParameterCount)
{
return null;
}

var dynamicMethod = new DynamicMethod(
ConstructorInfo.ConstructorName,
type,
Expand All @@ -95,7 +90,7 @@ internal sealed class ReflectionEmitMemberAccessor : MemberAccessor
Type paramType = parameters[i].ParameterType;

generator.Emit(OpCodes.Ldarg_0);
generator.Emit(OpCodes.Ldc_I4_S, i);
generator.Emit(OpCodes.Ldc_I4, i);
generator.Emit(OpCodes.Ldelem_Ref);
generator.Emit(OpCodes.Unbox_Any, paramType);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public ConstructorContext([DynamicallyAccessedMembers(DynamicallyAccessedMemberT
return new ConstructorContext(type).CreateInstance!;
}

public override Func<object[], T>? CreateParameterizedConstructor<T>(ConstructorInfo constructor)
public override Func<object[], T> CreateParameterizedConstructor<T>(ConstructorInfo constructor)
{
Type type = typeof(T);

Expand All @@ -50,11 +50,6 @@ public ConstructorContext([DynamicallyAccessedMembers(DynamicallyAccessedMemberT

int parameterCount = constructor.GetParameters().Length;

if (parameterCount > JsonConstants.MaxParameterCount)
{
return null;
}

return (arguments) =>
{
// The input array was rented from the shared ArrayPool, so its size is likely to be larger than the param count.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,6 @@ public static void ThrowNotSupportedException_TypeRequiresAsyncSerialization(Typ
throw new NotSupportedException(SR.Format(SR.TypeRequiresAsyncSerialization, propertyType));
}

[DoesNotReturn]
public static void ThrowNotSupportedException_ConstructorMaxOf64Parameters(Type type)
{
throw new NotSupportedException(SR.Format(SR.ConstructorMaxOf64Parameters, type));
}

[DoesNotReturn]
public static void ThrowNotSupportedException_DictionaryKeyTypeNotSupported(Type keyType, JsonConverter converter)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -726,7 +726,7 @@ async Task RunTestAsync<T>()
}

[Fact]
public async Task Cannot_Deserialize_ObjectWith_Ctor_With_65_Params()
public async Task Can_Deserialize_ObjectWith_Ctor_With_65_Params()
{
async Task RunTestAsync<T>()
{
Expand All @@ -752,11 +752,8 @@ async Task RunTestAsync<T>()
sb.Append("Int32");
sb.Append(")");

NotSupportedException ex = await Assert.ThrowsAsync<NotSupportedException>(() => Serializer.DeserializeWrapper<T>(input));
Assert.Contains(type.ToString(), ex.ToString());

ex = await Assert.ThrowsAsync<NotSupportedException>(() => Serializer.DeserializeWrapper<T>("{}"));
Assert.Contains(type.ToString(), ex.ToString());
T value = await Serializer.DeserializeWrapper<T>(input);
value = await Serializer.DeserializeWrapper<T>("{}");
}

await RunTestAsync<Class_With_Ctor_With_65_Params>();
Expand Down Expand Up @@ -1426,6 +1423,17 @@ public async Task TestClassWithDefaultCtorParams()
JsonTestHelper.AssertJsonEqual(json, await Serializer.SerializeWrapper(obj));
}

[Fact]
public async Task TestClassWithManyConstructorParameters()
{
ClassWithManyConstructorParameters value = ClassWithManyConstructorParameters.Create();
string json = JsonSerializer.Serialize(value);

ClassWithManyConstructorParameters result = await Serializer.DeserializeWrapper<ClassWithManyConstructorParameters>(json);

Assert.Equal(value, result); // Type is C# record that implements structural equality.
}

public class ClassWithDefaultCtorParams
{
public Point_2D_Struct_WithAttribute Struct { get; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ async Task RunTestAsync<T>(string testData)
}

[Fact]
public async Task Cannot_DeserializeAsync_ObjectWith_Ctor_With_65_Params()
public async Task Can_DeserializeAsync_ObjectWith_Ctor_With_65_Params()
{
if (StreamingSerializer is null)
{
Expand All @@ -137,6 +137,7 @@ async Task RunTestAsync<T>()
sb.Append("}");

string input = sb.ToString();
T value;

using (MemoryStream stream = new MemoryStream(Encoding.UTF8.GetBytes(input)))
{
Expand All @@ -145,7 +146,7 @@ async Task RunTestAsync<T>()
DefaultBufferSize = 1
};

await Assert.ThrowsAsync<NotSupportedException>(async () => await StreamingSerializer.DeserializeWrapper<T>(stream, options));
value = await StreamingSerializer.DeserializeWrapper<T>(stream, options);
}

using (MemoryStream stream = new MemoryStream("{}"u8.ToArray()))
Expand All @@ -155,7 +156,7 @@ async Task RunTestAsync<T>()
DefaultBufferSize = 1
};

await Assert.ThrowsAsync<NotSupportedException>(async () => await StreamingSerializer.DeserializeWrapper<T>(stream, options));
value = await StreamingSerializer.DeserializeWrapper<T>(stream, options);
Copy link
Member

@krwq krwq Sep 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should you add at least some tiny sanitization for couple of properties of value that they look good? (same above)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that the test method is generic on the deserialized type, so you can't really check anything other than validate that the JSON is nontrivial. I'm validating that roundtripping works as expected in the new test.

}
}

Expand Down
Loading