Json serialize when use custom nullable JsonConverter always thrown AccessViolationException

[Cricle]

Description

When use nullable JsonConverter to custom serialize double? and double, always thrown AccessViolationException.

Configuration

Item	Version
.NET Runtime	5.0.301
TargetFramework	5.0
System.Text.Json	5.0.1

Regression?

Never normal

Other information

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

When use nullable JsonConverter to custom serialize double? and double, always thrown AccessViolationException.

Configuration

Item	Version
.NET Runtime	5.0.301
TargetFramework	5.0
System.Text.Json	5.0.1

Regression?

Never normal

Other information

Author:	Cricle
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[eiriktsarpalis]

I can reproduce the issue in both .NET 5 and 6. Transcribing the minimal repro:

using System;
using System.Text.Json;
using System.Text.Json.Serialization;

JsonSerializer.Serialize(new NullableD(), new JsonSerializerOptions { Converters = { new NullableDouble() } });

public class NullableD
{
    public double B { get; set; }
}

public class NullableDouble : JsonConverter<double?>
{
    public override bool CanConvert(Type typeToConvert) => typeToConvert == typeof(double);

    public override double? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => 0;

    public override void Write(Utf8JsonWriter writer, double? value, JsonSerializerOptions options) => writer.WriteNullValue();
}

which throws

Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at DynamicClass.BGetter(System.Object)
   at System.Text.Json.Serialization.Metadata.JsonPropertyInfo`1[[System.Nullable`1[[System.Double, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].GetMemberAndWriteJson(System.Object, System.Text.Json.WriteStack ByRef, System.Text.Json.Utf8JsonWriter)
   at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].OnTryWrite(System.Text.Json.Utf8JsonWriter, System.__Canon, System.Text.Json.JsonSerializerOptions, System.Text.Json.WriteStack ByRef)
   at System.Text.Json.Serialization.JsonConverter`1[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].TryWrite(System.Text.Json.Utf8JsonWriter, System.__Canon ByRef, System.Text.Json.JsonSerializerOptions, System.Text.Json.WriteStack ByRef)
   at System.Text.Json.Serialization.JsonConverter`1[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].WriteCore(System.Text.Json.Utf8JsonWriter, System.__Canon ByRef, System.Text.Json.JsonSerializerOptions, System.Text.Json.WriteStack ByRef)
   at System.Text.Json.JsonSerializer.WriteCore[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.Text.Json.Serialization.JsonConverter, System.Text.Json.Utf8JsonWriter, System.__Canon ByRef, System.Text.Json.JsonSerializerOptions, System.Text.Json.WriteStack ByRef)
   at System.Text.Json.JsonSerializer.WriteUsingMetadata[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.Text.Json.Utf8JsonWriter, System.__Canon ByRef, System.Text.Json.Serialization.Metadata.JsonTypeInfo)
   at System.Text.Json.JsonSerializer.WriteUsingMetadata[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.__Canon ByRef, System.Text.Json.Serialization.Metadata.JsonTypeInfo)
   at System.Text.Json.JsonSerializer.Write[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.__Canon ByRef, System.Type, System.Text.Json.JsonSerializerOptions)
   at System.Text.Json.JsonSerializer.Serialize[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](System.__Canon, System.Text.Json.JsonSerializerOptions)
   at <Program>$.<Main>$(System.String[])
Segmentation fault

I believe the issue is caused by invalid IL emitted by the Reflection.Emit member accessor. The converter implementation is incorrect, however we should detecting this error earlier and fail gracefully with a helpful error message.

[wzchua]

Should this be an || here?

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs

Lines 253 to 257 in 90b8ddd

	if (!converterTypeToConvert.IsAssignableFromInternal(typeToConvert)
	&& !typeToConvert.IsAssignableFromInternal(converterTypeToConvert))
	{
	ThrowHelper.ThrowInvalidOperationException_SerializationConverterNotCompatible(converter.GetType(), typeToConvert);
	}

[eiriktsarpalis]

Should this be an || here?

I think that this check is intentional, apparently it is meant to support "covariant" and "contravariant" converters and will only fail if there is no subtype relationship between the two types. Here are some tests that validate this:

runtime/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.BadConverters.cs

Lines 46 to 57 in a8dacd5

	// Contravariant to Customer.
	Assert.Throws<SuccessException>(() => JsonSerializer.Deserialize<DerivedCustomer>("{}", options));
	Assert.Throws<SuccessException>(() => JsonSerializer.Serialize(new DerivedCustomer(), options));
	// Covariant to Customer.
	Assert.Throws<SuccessException>(() => JsonSerializer.Deserialize<Customer>("{}", options));
	Assert.Throws<SuccessException>(() => JsonSerializer.Serialize(new Customer(), options));
	Assert.Throws<SuccessException>(() => JsonSerializer.Serialize<Customer>(new DerivedCustomer(), options));
	Assert.Throws<SuccessException>(() => JsonSerializer.Deserialize<Person>("{}", options));
	Assert.Throws<SuccessException>(() => JsonSerializer.Serialize<Person>(new Customer(), options));
	Assert.Throws<SuccessException>(() => JsonSerializer.Serialize<Person>(new DerivedCustomer(), options));

I find that this mode is flawed since serialization is contravariant and deserialization is covariant, meaning that covariant serialization and contravariant deserialization will necessarily manifest as various kinds of runtime type errors. On top of that, much of the serialization infrastructure is built on the assumption that JsonConverter<T> is invariant, see for example:

• IEnumerableDefaultConverter
• DictionaryDefaultConverter
• JsonPropertyInfoOfT
• ObjectWithParameterizedConstructorConverter

and therefore any "contraviariant"/"covariant" converter will necessarily manifest as an InvalidCastException when composed with collection converters or constructor arguments.

Ultimately, that is the root cause for the reported issue: JsonPropertyInfo instances are constructed based on the converter type and not the PropertyInfo type. This in turn results in us emitting dynamic method accessors that contain type errors, which causes the AccessViolationException.

One was of addressing this issue as well as #46522 is to force converter invariance everywhere, namely changing the check in GetConverterInternal to say

            if (converter.TypeToConvert != typeToConvert)
            {
                ThrowHelper.ThrowInvalidOperationException_SerializationConverterNotCompatible(converter.GetType(), typeToConvert);
            }

This will ensure consistent behavior across the board (regardless of whether the converter is used as a root-level object or collection element) and any type mismatches will manifest as the same user-friendly error messages occurring early in the serialization process. Perhaps something to consider for .NET 7.

@layomia @steveharter @ericstj thoughts?

[steveharter]

We should not be causing AccessViolation. Ideally we improve the error detection upfront. If that is not feasible, we could add some castclass opcodes in the IL generation that will throw InvalidCastException, but that will have a small impact on runtime perf.

The converter model was not intended to be covariant - the JsonConverterFactory should be used to instantiate the correct converter.

[steveharter]

This could be moved to 7.0; I don't believe the semantics changed in 6.0.

[eiriktsarpalis]

I already have a PR open that fixes this: #56577