Add depth check for bad converters (dotnet/corefx#38956)

Commit migrated from https://github.com/dotnet/corefx/commit/1232838843670020a7128ca548abeafb13164e86

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs
index b5d9d4c..b01f874 100644 (file)
--- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs
+++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs
@@ -5,6 +5,7 @@
 using System.Collections;
 using System.Diagnostics;
 using System.Reflection;
+using System.Runtime.CompilerServices;
 using System.Text.Json.Serialization;
 using System.Text.Json.Serialization.Converters;
 
@@ -225,13 +226,6 @@ namespace System.Text.Json
 
         public abstract Type GetConcreteType(Type type);
 
-        private static void GetOriginalValues(ref Utf8JsonReader reader, out JsonTokenType tokenType, out int depth, out long bytesConsumed)
-        {
-            tokenType = reader.TokenType;
-            depth = reader.CurrentDepth;
-            bytesConsumed = reader.BytesConsumed;
-        }
-
         public virtual void GetPolicies()
         {
             DetermineSerializationCapabilities();
@@ -323,9 +317,13 @@ namespace System.Text.Json
             }
             else
             {
-                GetOriginalValues(ref reader, out JsonTokenType originalTokenType, out int originalDepth, out long bytesConsumed);
+                JsonTokenType originalTokenType = reader.TokenType;
+                int originalDepth = reader.CurrentDepth;
+                long originalBytesConsumed = reader.BytesConsumed;
+
                 OnRead(tokenType, ref state, ref reader);
-                VerifyRead(originalTokenType, originalDepth, bytesConsumed, ref state, ref reader);
+
+                VerifyRead(originalTokenType, originalDepth, originalBytesConsumed, ref state, ref reader);
             }
         }
 
@@ -333,9 +331,13 @@ namespace System.Text.Json
         {
             Debug.Assert(ShouldDeserialize);
 
-            GetOriginalValues(ref reader, out JsonTokenType originalTokenType, out int originalDepth, out long bytesConsumed);
+            JsonTokenType originalTokenType = reader.TokenType;
+            int originalDepth = reader.CurrentDepth;
+            long originalBytesConsumed = reader.BytesConsumed;
+
             OnReadEnumerable(tokenType, ref state, ref reader);
-            VerifyRead(originalTokenType, originalDepth, bytesConsumed, ref state, ref reader);
+
+            VerifyRead(originalTokenType, originalDepth, originalBytesConsumed, ref state, ref reader);
         }
 
         public JsonClassInfo RuntimeClassInfo
@@ -358,43 +360,49 @@ namespace System.Text.Json
         public bool ShouldSerialize { get; private set; }
         public bool ShouldDeserialize { get; private set; }
 
-        private void VerifyRead(JsonTokenType originalTokenType, int originalDepth, long bytesConsumed, ref ReadStack state, ref Utf8JsonReader reader)
+        private void VerifyRead(JsonTokenType tokenType, int depth, long bytesConsumed, ref ReadStack state, ref Utf8JsonReader reader)
         {
-            switch (originalTokenType)
+            switch (tokenType)
             {
                 case JsonTokenType.StartArray:
                     if (reader.TokenType != JsonTokenType.EndArray)
                     {
-                        // todo issue #38550 blocking this: originalDepth != reader.CurrentDepth + 1
                         ThrowHelper.ThrowJsonException_SerializationConverterRead(reader, state.JsonPath, ConverterBase.ToString());
                     }
+                    else if (depth != reader.CurrentDepth)
+                    {
+                        ThrowHelper.ThrowJsonException_SerializationConverterRead(reader, state.JsonPath, ConverterBase.ToString());
+                    }
+
+                    // Should not be possible to have not read anything.
+                    Debug.Assert(bytesConsumed < reader.BytesConsumed);
                     break;
 
                 case JsonTokenType.StartObject:
                     if (reader.TokenType != JsonTokenType.EndObject)
                     {
-                        // todo issue #38550 blocking this: originalDepth != reader.CurrentDepth + 1
                         ThrowHelper.ThrowJsonException_SerializationConverterRead(reader, state.JsonPath, ConverterBase.ToString());
                     }
+                    else if (depth != reader.CurrentDepth)
+                    {
+                        ThrowHelper.ThrowJsonException_SerializationConverterRead(reader, state.JsonPath, ConverterBase.ToString());
+                    }
+
+                    // Should not be possible to have not read anything.
+                    Debug.Assert(bytesConsumed < reader.BytesConsumed);
                     break;
 
                 default:
                     // Reading a single property value.
-                    if (reader.TokenType != originalTokenType || reader.BytesConsumed != bytesConsumed)
+                    if (reader.BytesConsumed != bytesConsumed)
                     {
                         ThrowHelper.ThrowJsonException_SerializationConverterRead(reader, state.JsonPath, ConverterBase.ToString());
                     }
 
-                    break;
-            }
-        }
+                    // Should not be possible to change token type.
+                    Debug.Assert(reader.TokenType == tokenType);
 
-        private void VerifyWrite(int originalDepth, ref WriteStack state, ref Utf8JsonWriter writer)
-        {
-            // todo issue #38550 blocking this: originalDepth != reader.CurrentDepth
-            if (originalDepth != writer.CurrentDepth)
-            {
-                ThrowHelper.ThrowJsonException_SerializationConverterWrite(state.PropertyPath, ConverterBase.ToString());
+                    break;
             }
         }
 
@@ -411,27 +419,40 @@ namespace System.Text.Json
             else
             {
                 int originalDepth = writer.CurrentDepth;
+
                 OnWrite(ref state.Current, writer);
-                VerifyWrite(originalDepth, ref state, ref writer);
+
+                if (originalDepth != writer.CurrentDepth)
+                {
+                    ThrowHelper.ThrowJsonException_SerializationConverterWrite(state.PropertyPath, ConverterBase.ToString());
+                }
             }
         }
 
         public void WriteDictionary(ref WriteStack state, Utf8JsonWriter writer)
         {
             Debug.Assert(ShouldSerialize);
-
             int originalDepth = writer.CurrentDepth;
+
             OnWriteDictionary(ref state.Current, writer);
-            VerifyWrite(originalDepth, ref state, ref writer);
+
+            if (originalDepth != writer.CurrentDepth)
+            {
+                ThrowHelper.ThrowJsonException_SerializationConverterWrite(state.PropertyPath, ConverterBase.ToString());
+            }
         }
 
         public void WriteEnumerable(ref WriteStack state, Utf8JsonWriter writer)
         {
             Debug.Assert(ShouldSerialize);
-
             int originalDepth = writer.CurrentDepth;
+
             OnWriteEnumerable(ref state.Current, writer);
-            VerifyWrite(originalDepth, ref state, ref writer);
+
+            if (originalDepth != writer.CurrentDepth)
+            {
+                ThrowHelper.ThrowJsonException_SerializationConverterWrite(state.PropertyPath, ConverterBase.ToString());
+            }
         }
     }
 }

diff --git a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests.BadConverters.cs b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests.BadConverters.cs
index 2df6def..a6aed82 100644 (file)
--- a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests.BadConverters.cs
+++ b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests.BadConverters.cs
@@ -121,28 +121,77 @@ namespace System.Text.Json.Serialization.Tests
 
         private class Level3
         {
+            // If true, read\write too much instead of too little.
+            public bool ReadWriteTooMuch { get; set; }
         }
 
-        private class Level3ConverterThatDoesntReadOrWrite : JsonConverter<Level3>
+        private class Level3ConverterThatsBad: JsonConverter<Level3>
         {
             public override Level3 Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
             {
+                Assert.Equal(JsonTokenType.StartObject, reader.TokenType);
+
+                reader.Read();
+                Assert.Equal(JsonTokenType.PropertyName, reader.TokenType);
+
+                reader.Read();
+                Assert.True(reader.TokenType == JsonTokenType.True || reader.TokenType == JsonTokenType.False);
+
+                // Determine if we should read too much.
+                if (reader.TokenType == JsonTokenType.True)
+                {
+                    // Do the correct read.
+                    reader.Read();
+                    Assert.Equal(JsonTokenType.EndObject, reader.TokenType);
+
+                    // Do an extra read.
+                    reader.Read();
+                    Assert.Equal(JsonTokenType.EndArray, reader.TokenType);
+
+                    // End on EndObject token so it looks good, but wrong depth.
+                    reader.Read();
+                    Assert.Equal(JsonTokenType.EndObject, reader.TokenType);
+                }
+
                 return new Level3();
             }
 
             public override void Write(Utf8JsonWriter writer, Level3 value, JsonSerializerOptions options)
             {
-                writer.WriteStartObject();
+                if (value.ReadWriteTooMuch)
+                {
+                    writer.WriteStartObject();
+                }
             }
         }
 
         [Fact]
         public static void ConverterReadTooLittle()
         {
-            const string json = @"{""Level2"":{""Level3s"":[{}]}}";
+            const string json = @"{""Level2"":{""Level3s"":[{""ReadWriteTooMuch"":false}]}}";
+
+            var options = new JsonSerializerOptions();
+            options.Converters.Add(new Level3ConverterThatsBad());
+
+            try
+            {
+                JsonSerializer.Deserialize<Level1>(json, options);
+                Assert.True(false, "Expected exception");
+            }
+            catch (JsonException ex)
+            {
+                Assert.Contains("$.Level2.Level3s[1]", ex.ToString());
+                Assert.Equal(ex.Path, "$.Level2.Level3s[1]");
+            }
+        }
+
+        [Fact]
+        public static void ConverterReadTooMuch()
+        {
+            const string json = @"{""Level2"":{""Level3s"":[{""ReadWriteTooMuch"":true}]}}";
 
             var options = new JsonSerializerOptions();
-            options.Converters.Add(new Level3ConverterThatDoesntReadOrWrite());
+            options.Converters.Add(new Level3ConverterThatsBad ());
 
             try
             {
@@ -157,14 +206,28 @@ namespace System.Text.Json.Serialization.Tests
         }
 
         [Fact]
-        public static void ConverterWroteTooLittle()
+        public static void ConverterWroteNothing()
         {
             var options = new JsonSerializerOptions();
-            options.Converters.Add(new Level3ConverterThatDoesntReadOrWrite());
+            options.Converters.Add(new Level3ConverterThatsBad());
+
+            // Not writing is allowed.
+            string str = JsonSerializer.Serialize(new Level1(), options);
+            Assert.False(string.IsNullOrEmpty(str));
+        }
+
+        [Fact]
+        public static void ConverterWroteTooMuch()
+        {
+            var options = new JsonSerializerOptions();
+            options.Converters.Add(new Level3ConverterThatsBad());
 
             try
             {
-                JsonSerializer.Serialize(new Level1(), options);
+                var l1 = new Level1();
+                l1.Level2.Level3s[0].ReadWriteTooMuch = true;
+
+                JsonSerializer.Serialize(l1, options);
                 Assert.True(false, "Expected exception");
             }
             catch (JsonException ex)

diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs
index de7b9ab..676b215 100644 (file)
--- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs
+++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs
@@ -223,7 +223,7 @@ namespace System.Text.Json.Serialization.Tests
             Case2 = 1,
         }
 
-        private struct ClassWithOverride
+        private struct StructWithOverride
         {
             [JsonIgnore]
             public MyEnum EnumValue { get; set; }
@@ -238,7 +238,7 @@ namespace System.Text.Json.Serialization.Tests
                     {
                         EnumValue = MyEnum.Case1;
                     }
-                    if (value == "Case2")
+                    else if (value == "Case2")
                     {
                         EnumValue = MyEnum.Case2;
                     }
@@ -255,7 +255,7 @@ namespace System.Text.Json.Serialization.Tests
         {
             const string json = @"{""EnumValue"":""Case2""}";
 
-            ClassWithOverride obj = JsonSerializer.Deserialize<ClassWithOverride>(json);
+            StructWithOverride obj = JsonSerializer.Deserialize<StructWithOverride>(json);
 
             Assert.Equal(MyEnum.Case2, obj.EnumValue);
             Assert.Equal("Case2", obj.EnumString);