Fix AccessViolation/InvalidProgramException in custom nullable converters. (#56577)

* Fix AccessViolation/InvalidProgramException in custom nullable converters.

* address feedback

* add comments to new test converters.

diff --git a/src/libraries/System.Text.Json/src/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx
index 434708f..b3c5cc4 100644 (file)
--- a/src/libraries/System.Text.Json/src/Resources/Strings.resx
+++ b/src/libraries/System.Text.Json/src/Resources/Strings.resx
@@ -524,7 +524,7 @@
   <data name="NumberHandlingOnPropertyTypeMustBeNumberOrCollection" xml:space="preserve">
     <value>When 'JsonNumberHandlingAttribute' is placed on a property or field, the property or field must be a number or a collection of numbers. See member '{0}' on type '{1}'.</value>
   </data>
-  <data name="ConverterCanConvertNullableRedundant" xml:space="preserve">
+  <data name="ConverterCanConvertMultipleTypes" xml:space="preserve">
     <value>The converter '{0}' handles type '{1}' but is being asked to convert type '{2}'. Either create a separate converter for type '{2}' or change the converter's 'CanConvert' method to only return 'true' for a single type.</value>
   </data>
   <data name="MetadataReferenceOfTypeCannotBeAssignedToType" xml:space="preserve">

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
index dc11589..e394d9a 100644 (file)
--- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
+++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
@@ -129,16 +129,18 @@ namespace System.Text.Json
                 Debug.Assert(converter != null);
             }
 
-            // User indicated that non-nullable-struct-handling converter should handle a nullable struct type.
-            // The serializer would have picked that converter up by default and wrapped it in NullableConverter<T>;
-            // throw so that user can modify or remove their unnecessary CanConvert method override.
+            // User has indicated that either:
+            //   a) a non-nullable-struct handling converter should handle a nullable struct type or
+            //   b) a nullable-struct handling converter should handle a non-nullable struct type.
+            // User should implement a custom converter for the underlying struct and remove the unnecessary CanConvert method override.
+            // The serializer will automatically wrap the custom converter with NullableConverter<T>.
             //
             // We also throw to avoid passing an invalid argument to setters for nullable struct properties,
             // which would cause an InvalidProgramException when the generated IL is invoked.
-            // This is not an issue of the converter is wrapped in NullableConverter<T>.
-            if (runtimePropertyType.CanBeNull() && !converter.TypeToConvert.CanBeNull())
+            if (runtimePropertyType.IsValueType && converter.IsValueType &&
+                (runtimePropertyType.IsNullableOfT() ^ converter.TypeToConvert.IsNullableOfT()))
             {
-                ThrowHelper.ThrowInvalidOperationException_ConverterCanConvertNullableRedundant(runtimePropertyType, converter);
+                ThrowHelper.ThrowInvalidOperationException_ConverterCanConvertMultipleTypes(runtimePropertyType, converter);
             }
 
             return converter;

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitMemberAccessor.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitMemberAccessor.cs
index d2ce031..4357b7d 100644 (file)
--- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitMemberAccessor.cs
+++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitMemberAccessor.cs
@@ -258,6 +258,10 @@ namespace System.Text.Json.Serialization.Metadata
 
             if (declaredPropertyType != runtimePropertyType && declaredPropertyType.IsValueType)
             {
+                // Not supported scenario: possible if declaredPropertyType == int? and runtimePropertyType == int
+                // We should catch that particular case earlier in converter generation.
+                Debug.Assert(!runtimePropertyType.IsValueType);
+
                 generator.Emit(OpCodes.Box, declaredPropertyType);
             }
 
@@ -291,6 +295,10 @@ namespace System.Text.Json.Serialization.Metadata
 
             if (declaredPropertyType != runtimePropertyType && declaredPropertyType.IsValueType)
             {
+                // Not supported scenario: possible if e.g. declaredPropertyType == int? and runtimePropertyType == int
+                // We should catch that particular case earlier in converter generation.
+                Debug.Assert(!runtimePropertyType.IsValueType);
+
                 generator.Emit(OpCodes.Unbox_Any, declaredPropertyType);
             }
 

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs b/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
index 0ed88d8..04da3b9 100644 (file)
--- a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
+++ b/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
@@ -258,9 +258,9 @@ namespace System.Text.Json
 
         [DoesNotReturn]
         [MethodImpl(MethodImplOptions.NoInlining)]
-        public static void ThrowInvalidOperationException_ConverterCanConvertNullableRedundant(Type runtimePropertyType, JsonConverter jsonConverter)
+        public static void ThrowInvalidOperationException_ConverterCanConvertMultipleTypes(Type runtimePropertyType, JsonConverter jsonConverter)
         {
-            throw new InvalidOperationException(SR.Format(SR.ConverterCanConvertNullableRedundant, jsonConverter.GetType(), jsonConverter.TypeToConvert, runtimePropertyType));
+            throw new InvalidOperationException(SR.Format(SR.ConverterCanConvertMultipleTypes, jsonConverter.GetType(), jsonConverter.TypeToConvert, runtimePropertyType));
         }
 
         [DoesNotReturn]

diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.BadConverters.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.BadConverters.cs
index b62d855..062a7e9 100644 (file)
--- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.BadConverters.cs
+++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.BadConverters.cs
@@ -419,5 +419,88 @@ namespace System.Text.Json.Serialization.Tests
                 throw new NotImplementedException();
             }
         }
+
+
+        [Fact]
+        public static void BadDoubleConverter_Serialization()
+        {
+            var options = new JsonSerializerOptions
+            {
+                Converters = { new BadDoubleConverter() }
+            };
+
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize<double?>(3.14, options));
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(new { value = (double?)3.14 }, options));
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(new double?[] { 3.14 }, options));
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(new Dictionary<string, double?> { ["key"] = 3.14 }, options));
+        }
+
+        [Fact]
+        public static void BadDoubleConverter_Deserialization()
+        {
+            var options = new JsonSerializerOptions
+            {
+                Converters = { new BadDoubleConverter() }
+            };
+
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize<double?>("3.14", options));
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize<PocoWithGenericProperty<double?>>(@"{""Property"":3.14}", options));
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize<double?[]>("[3.14]", options));
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize<Dictionary<string, double?>>(@"{""key"":3.14}", options));
+        }
+
+
+        [Fact]
+        public static void BadNullableDoubleConverter_Serialization()
+        {
+            var options = new JsonSerializerOptions
+            {
+                Converters = { new BadNullableDoubleConverter() }
+            };
+
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(3.14, options));
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(new { value = 3.14 }, options));
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(new[] { 3.14 }, options));
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(new Dictionary<string, double> { ["key"] = 3.14 }, options));
+        }
+
+        [Fact]
+        public static void BadNullableDoubleConverter_Deserialization()
+        {
+            var options = new JsonSerializerOptions
+            {
+                Converters = { new BadNullableDoubleConverter() }
+            };
+
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize<double>("3.14", options));
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize<PocoWithGenericProperty<double>>(@"{""Property"":3.14}", options));
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize<double[]>("[3.14]", options));
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize<Dictionary<string, double>>(@"{""key"":3.14}", options));
+        }
+
+        private class PocoWithGenericProperty<T>
+        {
+            public T Property { get; set; }
+        }
+
+        /// <summary>
+        /// A double converter that incorrectly claims to support double? types.
+        /// </summary>
+        private class BadDoubleConverter : JsonConverter<double>
+        {
+            public override bool CanConvert(Type typeToConvert) => typeToConvert == typeof(double) || typeToConvert == typeof(double?);
+            public override double Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => reader.GetDouble();
+            public override void Write(Utf8JsonWriter writer, double value, JsonSerializerOptions options) => writer.WriteNumberValue(value);
+        }
+
+        /// <summary>
+        /// A double? converter that incorrectly claims to support double types.
+        /// </summary>
+        private class BadNullableDoubleConverter : JsonConverter<double?>
+        {
+            public override bool CanConvert(Type typeToConvert) => typeToConvert == typeof(double) || typeToConvert == typeof(double?);
+            public override double? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => reader.GetDouble();
+            public override void Write(Utf8JsonWriter writer, double? value, JsonSerializerOptions options) => writer.WriteNumberValue(value.Value);
+        }
     }
 }