Guard against (de)serializing pointers & ref structs (#34777)
authorLayomi Akinrinade <laakinri@microsoft.com>
Mon, 27 Apr 2020 19:29:45 +0000 (15:29 -0400)
committerGitHub <noreply@github.com>
Mon, 27 Apr 2020 19:29:45 +0000 (12:29 -0700)
* Guard against (de)serializing pointers & ref structs

* Address review feedback

* Update isbyreflike check

* Address review feedback; fetch IsByRefLike with reflection

* Address review feedback

* Remove extra whitespace

* Add debug assert

* Add attribute-based fallback

* Ensure that property look-up with reflection is useful

* Remove IsByRefLike property reflection prob

* Move runtime type debug validation upstream

src/libraries/System.Text.Json/src/Resources/Strings.resx
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs
src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
src/libraries/System.Text.Json/tests/Serialization/InvalidTypeTests.cs [new file with mode: 0644]
src/libraries/System.Text.Json/tests/Serialization/OpenGenericTests.cs [deleted file]
src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj

index 599a56e..f6482d6 100644 (file)
   <data name="BufferMaximumSizeExceeded" xml:space="preserve">
     <value>Cannot allocate a buffer of size {0}.</value>
   </data>
-  <data name="CannotSerializeOpenGeneric" xml:space="preserve">
-    <value>The type '{0}' has generic parameters that have not been replaced by specific types, which is not valid during serialization or deserialization.</value>
+  <data name="CannotSerializeInvalidType" xml:space="preserve">
+    <value>The type '{0}' is invalid for serialization or deserialization because it is a pointer type, is a ref struct, or contains generic parameters that have not been replaced by specific types.</value>
   </data>
   <data name="SerializeTypeInstanceNotSupported" xml:space="preserve">
     <value>Serialization and deserialization of 'System.Type' instances are not supported and should be avoided since they can lead to security issues.</value>
   <data name="JsonIncludeOnNonPublicInvalid" xml:space="preserve">
     <value>The non-public property '{0}' on type '{1}' is annotated with 'JsonIncludeAttribute' which is invalid.</value>
   </data>
+  <data name="CannotSerializeInvalidMember" xml:space="preserve">
+    <value>The type '{0}' of property '{1}' on type '{2}' is invalid for serialization or deserialization because it is a pointer type, is a ref struct, or contains generic parameters that have not been replaced by specific types.</value>
+  </data>
 </root>
\ No newline at end of file
index e2cbb3b..12202e3 100644 (file)
@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Reflection;
+using System.Runtime.CompilerServices;
 using System.Text.Json.Serialization;
 
 namespace System.Text.Json
@@ -352,6 +353,7 @@ namespace System.Text.Json
             JsonSerializerOptions options)
         {
             Debug.Assert(type != null);
+            ValidateType(type, parentClassType, propertyInfo, options);
 
             JsonConverter converter = options.DetermineConverter(parentClassType, type, propertyInfo)!;
 
@@ -395,7 +397,46 @@ namespace System.Text.Json
                 }
             }
 
+            Debug.Assert(!IsInvalidForSerialization(runtimeType));
+
             return converter;
         }
+
+        private static void ValidateType(Type type, Type? parentClassType, PropertyInfo? propertyInfo, JsonSerializerOptions options)
+        {
+            if (!options.TypeIsCached(type) && IsInvalidForSerialization(type))
+            {
+                ThrowHelper.ThrowInvalidOperationException_CannotSerializeInvalidType(type, parentClassType, propertyInfo);
+            }
+        }
+
+        private static bool IsInvalidForSerialization(Type type)
+        {
+            return type.IsPointer || IsByRefLike(type) || type.ContainsGenericParameters;
+        }
+
+        private static bool IsByRefLike(Type type)
+        {
+#if BUILDING_INBOX_LIBRARY
+            return type.IsByRefLike;
+#else
+            if (!type.IsValueType)
+            {
+                return false;
+            }
+
+            object[] attributes = type.GetCustomAttributes(inherit: false);
+
+            for (int i = 0; i < attributes.Length; i++)
+            {
+                if (attributes[i].GetType().FullName == "System.Runtime.CompilerServices.IsByRefLikeAttribute")
+                {
+                    return true;
+                }
+            }
+
+            return false;
+#endif
+        }
     }
 }
index 7d4c82b..53d1a67 100644 (file)
@@ -6,6 +6,7 @@ using System.Collections.Concurrent;
 using System.Diagnostics;
 using System.Text.Json.Serialization;
 using System.Text.Encodings.Web;
+using System.Reflection;
 
 namespace System.Text.Json
 {
@@ -377,17 +378,17 @@ namespace System.Text.Json
             // https://github.com/dotnet/runtime/issues/32357
             if (!_classes.TryGetValue(type, out JsonClassInfo? result))
             {
-                if (type.ContainsGenericParameters)
-                {
-                    ThrowHelper.ThrowInvalidOperationException_CannotSerializeOpenGeneric(type);
-                }
-
                 result = _classes.GetOrAdd(type, new JsonClassInfo(type, this));
             }
 
             return result;
         }
 
+        internal bool TypeIsCached(Type type)
+        {
+            return _classes.ContainsKey(type);
+        }
+
         internal JsonReaderOptions GetReaderOptions()
         {
             return new JsonReaderOptions
index f36130c..2be227d 100644 (file)
@@ -94,9 +94,16 @@ namespace System.Text.Json
 
         [DoesNotReturn]
         [MethodImpl(MethodImplOptions.NoInlining)]
-        public static void ThrowInvalidOperationException_CannotSerializeOpenGeneric(Type type)
+        public static void ThrowInvalidOperationException_CannotSerializeInvalidType(Type type, Type? parentClassType, PropertyInfo? propertyInfo)
         {
-            throw new InvalidOperationException(SR.Format(SR.CannotSerializeOpenGeneric, type));
+            if (parentClassType == null)
+            {
+                Debug.Assert(propertyInfo == null);
+                throw new InvalidOperationException(SR.Format(SR.CannotSerializeInvalidType, type));
+            }
+
+            Debug.Assert(propertyInfo != null);
+            throw new InvalidOperationException(SR.Format(SR.CannotSerializeInvalidMember, type, propertyInfo.Name, parentClassType));
         }
 
         [DoesNotReturn]
diff --git a/src/libraries/System.Text.Json/tests/Serialization/InvalidTypeTests.cs b/src/libraries/System.Text.Json/tests/Serialization/InvalidTypeTests.cs
new file mode 100644 (file)
index 0000000..edad8a0
--- /dev/null
@@ -0,0 +1,167 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System.Collections.Generic;
+using Xunit;
+
+namespace System.Text.Json.Serialization.Tests
+{
+    public class InvalidTypeTests_Span : InvalidTypeTests
+    {
+        public InvalidTypeTests_Span() : base(SerializationWrapper.SpanSerializer) { }
+    }
+
+    public class InvalidTypeTests_String : InvalidTypeTests
+    {
+        public InvalidTypeTests_String() : base(SerializationWrapper.StringSerializer) { }
+    }
+
+    public class InvalidTypeTests_Stream : InvalidTypeTests
+    {
+        public InvalidTypeTests_Stream() : base(SerializationWrapper.StreamSerializer) { }
+    }
+
+    public class InvalidTypeTests_StreamWithSmallBuffer : InvalidTypeTests
+    {
+        public InvalidTypeTests_StreamWithSmallBuffer() : base(SerializationWrapper.StreamSerializerWithSmallBuffer) { }
+    }
+
+    public class InvalidTypeTests_Writer : InvalidTypeTests
+    {
+        public InvalidTypeTests_Writer() : base(SerializationWrapper.WriterSerializer) { }
+    }
+
+    public abstract class InvalidTypeTests
+    {
+        private SerializationWrapper Serializer { get; }
+
+        public InvalidTypeTests(SerializationWrapper serializer)
+        {
+            Serializer = serializer;
+        }
+
+        [Theory]
+        [MemberData(nameof(OpenGenericTypes))]
+        [MemberData(nameof(RefStructTypes))]
+        [MemberData(nameof(PointerTypes))]
+        public void DeserializeInvalidType(Type type)
+        {
+            InvalidOperationException ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize("", type));
+            Assert.Contains(type.ToString(), ex.ToString());
+        }
+
+        [Theory]
+        [MemberData(nameof(TypesWithInvalidMembers_WithMembers))]
+        public void TypeWithInvalidMember(Type classType, Type invalidMemberType, string invalidMemberName)
+        {
+            static void ValidateException(InvalidOperationException ex, Type classType, Type invalidMemberType, string invalidMemberName)
+            {
+                string exAsStr = ex.ToString();
+                Assert.Contains(invalidMemberType.ToString(), exAsStr);
+                Assert.Contains(invalidMemberName, exAsStr);
+                Assert.Contains(classType.ToString(), exAsStr);
+            }
+
+            object obj = Activator.CreateInstance(classType);
+            InvalidOperationException ex = Assert.Throws<InvalidOperationException>(() => Serializer.Serialize(obj, classType));
+            ValidateException(ex, classType, invalidMemberType, invalidMemberName);
+
+            ex = Assert.Throws<InvalidOperationException>(() => Serializer.Serialize(null, classType));
+            ValidateException(ex, classType, invalidMemberType, invalidMemberName);
+
+            ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize("", classType));
+            ValidateException(ex, classType, invalidMemberType, invalidMemberName);
+        }
+
+        [Theory]
+        [MemberData(nameof(OpenGenericTypes_ToSerialize))]
+        public void SerializeOpenGeneric(Type type)
+        {
+            object obj;
+
+            if (type.GetGenericArguments().Length == 1)
+            {
+                obj = Activator.CreateInstance(type.MakeGenericType(typeof(int)));
+            }
+            else
+            {
+                obj = Activator.CreateInstance(type.MakeGenericType(typeof(string), typeof(int)));
+            }
+
+            Assert.Throws<ArgumentException>(() => Serializer.Serialize(obj, type));
+        }
+
+        [Theory]
+        [MemberData(nameof(OpenGenericTypes))]
+        public void SerializeInvalidTypes_NullValue(Type type)
+        {
+            InvalidOperationException ex = Assert.Throws<InvalidOperationException>(() => Serializer.Serialize(null, type));
+            Assert.Contains(type.ToString(), ex.ToString());
+        }
+
+        [Fact]
+        public void SerializeOpenGeneric_NullableOfT()
+        {
+            Type openNullableType = typeof(Nullable<>);
+            object obj = Activator.CreateInstance(openNullableType.MakeGenericType(typeof(int)));
+
+            InvalidOperationException ex = Assert.Throws<InvalidOperationException>(() => Serializer.Serialize(obj, openNullableType));
+            Assert.Contains(openNullableType.ToString(), ex.ToString());
+        }
+
+        private class Test<T> { }
+
+        private static IEnumerable<object[]> OpenGenericTypes()
+        {
+            yield return new object[] { typeof(Test<>) };
+            yield return new object[] { typeof(Nullable<>) };
+            yield return new object[] { typeof(IList<>) };
+            yield return new object[] { typeof(List<>) };
+            yield return new object[] { typeof(List<>).MakeGenericType(typeof(Test<>)) };
+            yield return new object[] { typeof(Test<>).MakeGenericType(typeof(List<>)) };
+            yield return new object[] { typeof(Dictionary<,>) };
+            yield return new object[] { typeof(Dictionary<,>).MakeGenericType(typeof(string), typeof(Nullable<>)) };
+            yield return new object[] { typeof(Dictionary<,>).MakeGenericType(typeof(Nullable<>), typeof(string)) };
+        }
+
+        private static IEnumerable<object[]> OpenGenericTypes_ToSerialize()
+        {
+            yield return new object[] { typeof(Test<>) };
+            yield return new object[] { typeof(List<>) };
+            yield return new object[] { typeof(Dictionary<,>) };
+        }
+
+        private static IEnumerable<object[]> RefStructTypes()
+        {
+            yield return new object[] { typeof(Span<int>) };
+            yield return new object[] { typeof(Utf8JsonReader) };
+            yield return new object[] { typeof(MyRefStruct) };
+        }
+
+        private static readonly Type s_intPtrType = typeof(int*); // Unsafe code may not appear in iterators.
+
+        private static IEnumerable<object[]> PointerTypes()
+        {
+            yield return new object[] { s_intPtrType };
+        }
+
+        // Instances of the types of the invalid members cannot be passed directly
+        // to the serializer on serialization due to type constraints,
+        // e.g. int* can't be boxed and passed to the non-generic overload,
+        // and typeof(int*) can't be a generic parameter to the generic overload.
+        private static IEnumerable<object[]> TypesWithInvalidMembers_WithMembers()
+        {
+            yield return new object[] { typeof(Memory<byte>), typeof(Span<byte>), "Span" }; // Contains Span<byte> property.
+
+            yield return new object[] { typeof(ClassWithIntPtr), s_intPtrType, "IntPtr" };
+        }
+
+        private class ClassWithIntPtr
+        {
+            public unsafe int* IntPtr { get; }
+        }
+
+        private ref struct MyRefStruct { }
+    }
+}
diff --git a/src/libraries/System.Text.Json/tests/Serialization/OpenGenericTests.cs b/src/libraries/System.Text.Json/tests/Serialization/OpenGenericTests.cs
deleted file mode 100644 (file)
index 15351f7..0000000
+++ /dev/null
@@ -1,109 +0,0 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
-
-using System.Collections.Generic;
-using Xunit;
-
-namespace System.Text.Json.Serialization.Tests
-{
-    public class OpenGenericTests_Span : OpenGenericTests
-    {
-        public OpenGenericTests_Span() : base(SerializationWrapper.SpanSerializer) { }
-    }
-
-    public class OpenGenericTests_String : OpenGenericTests
-    {
-        public OpenGenericTests_String() : base(SerializationWrapper.StringSerializer) { }
-    }
-
-    public class OpenGenericTests_Stream : OpenGenericTests
-    {
-        public OpenGenericTests_Stream() : base(SerializationWrapper.StreamSerializer) { }
-    }
-
-    public class OpenGenericTests_StreamWithSmallBuffer : OpenGenericTests
-    {
-        public OpenGenericTests_StreamWithSmallBuffer() : base(SerializationWrapper.StreamSerializerWithSmallBuffer) { }
-    }
-
-    public class OpenGenericTests_Writer : OpenGenericTests
-    {
-        public OpenGenericTests_Writer() : base(SerializationWrapper.WriterSerializer) { }
-    }
-
-    public abstract class OpenGenericTests
-    {
-        private SerializationWrapper Serializer { get; }
-
-        public OpenGenericTests(SerializationWrapper serializer)
-        {
-            Serializer = serializer;
-        }
-
-        [Theory]
-        [MemberData(nameof(TypesWithOpenGenerics))]
-        public void DeserializeOpenGeneric(Type type)
-        {
-            InvalidOperationException ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize("", type));
-            Assert.Contains(type.ToString(), ex.ToString());
-        }
-
-        [Theory]
-        [MemberData(nameof(TypesWithOpenGenerics_ToSerialize))]
-        public void SerializeOpenGeneric(Type type)
-        {
-            object obj;
-
-            if (type.GetGenericArguments().Length == 1)
-            {
-                obj = Activator.CreateInstance(type.MakeGenericType(typeof(int)));
-            }
-            else
-            {
-                obj = Activator.CreateInstance(type.MakeGenericType(typeof(string), typeof(int)));
-            }
-
-            Assert.Throws<ArgumentException>(() => Serializer.Serialize(obj, type));
-        }
-
-        [Theory]
-        [MemberData(nameof(TypesWithOpenGenerics))]
-        public void SerializeOpenGeneric_NullValue(Type type)
-        {
-            InvalidOperationException ex = Assert.Throws<InvalidOperationException>(() => Serializer.Serialize(null, type));
-            Assert.Contains(type.ToString(), ex.ToString());
-        }
-
-        [Fact]
-        public void SerializeOpenGeneric_NullableOfT()
-        {
-            Type openNullableType = typeof(Nullable<>);
-            object obj = Activator.CreateInstance(openNullableType.MakeGenericType(typeof(int)));
-
-            InvalidOperationException ex = Assert.Throws<InvalidOperationException>(() => Serializer.Serialize(obj, openNullableType));
-            Assert.Contains(openNullableType.ToString(), ex.ToString());
-        }
-
-        private class Test<T> { }
-
-        private static IEnumerable<object[]> TypesWithOpenGenerics()
-        {
-            yield return new object[] { typeof(Test<>) };
-            yield return new object[] { typeof(Nullable<>) };
-            yield return new object[] { typeof(List<>) };
-            yield return new object[] { typeof(List<>).MakeGenericType(typeof(Test<>)) };
-            yield return new object[] { typeof(Test<>).MakeGenericType(typeof(List<>)) };
-            yield return new object[] { typeof(Dictionary<,>) };
-            yield return new object[] { typeof(Dictionary<,>).MakeGenericType(typeof(string), typeof(Nullable<>)) };
-            yield return new object[] { typeof(Dictionary<,>).MakeGenericType(typeof(Nullable<>), typeof(string)) };
-        }
-
-        private static IEnumerable<object[]> TypesWithOpenGenerics_ToSerialize()
-        {
-            yield return new object[] { typeof(Test<>) };
-            yield return new object[] { typeof(List<>) };
-            yield return new object[] { typeof(Dictionary<,>) };
-        }
-    }
-}
index c0ed35c..fc7bc4d 100644 (file)
@@ -70,6 +70,7 @@
     <Compile Include="Serialization\EnumTests.cs" />
     <Compile Include="Serialization\ExceptionTests.cs" />
     <Compile Include="Serialization\ExtensionDataTests.cs" />
+    <Compile Include="Serialization\InvalidTypeTests.cs" />
     <Compile Include="Serialization\JsonElementTests.cs" />
     <Compile Include="Serialization\JsonDocumentTests.cs" />
     <Compile Include="Serialization\Null.ReadTests.cs" />
@@ -77,7 +78,6 @@
     <Compile Include="Serialization\NullableTests.cs" />
     <Compile Include="Serialization\Object.ReadTests.cs" />
     <Compile Include="Serialization\Object.WriteTests.cs" />
-    <Compile Include="Serialization\OpenGenericTests.cs" />
     <Compile Include="Serialization\OptionsTests.cs" />
     <Compile Include="Serialization\PolymorphicTests.cs" />
     <Compile Include="Serialization\PropertyNameTests.cs" />