[JsonSerializer] Prevent arbitrary properties from being cached when deserializing...

* [JsonSerializer] Prevent arbitrary properties from being cached when deserializing with parameterized ctors

* Address review feedback

* Correct check on when to grow property pool in sync code paths

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs
index f001c86..6e2f251 100644 (file)
--- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs
+++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs
@@ -7,8 +7,8 @@ using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.Runtime.CompilerServices;
 
-using FoundProperties = System.ValueTuple<System.Text.Json.JsonPropertyInfo, System.Text.Json.JsonReaderState, long, byte[]?, string?>;
-using FoundPropertiesAsync = System.ValueTuple<System.Text.Json.JsonPropertyInfo, object?, string?>;
+using FoundProperty = System.ValueTuple<System.Text.Json.JsonPropertyInfo, System.Text.Json.JsonReaderState, long, byte[]?, string?>;
+using FoundPropertyAsync = System.ValueTuple<System.Text.Json.JsonPropertyInfo, object?, string?>;
 
 namespace System.Text.Json.Serialization.Converters
 {
@@ -21,6 +21,7 @@ namespace System.Text.Json.Serialization.Converters
         internal sealed override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, [MaybeNullWhen(false)] out T value)
         {
             object obj;
+            ArgumentState argumentState = state.Current.CtorArgumentState!;
 
             if (state.UseFastPath)
             {
@@ -32,21 +33,24 @@ namespace System.Text.Json.Serialization.Converters
 
                 obj = CreateObject(ref state.Current);
 
-                if (state.Current.PropertyIndex > 0)
+                if (argumentState.FoundPropertyCount > 0)
                 {
                     Utf8JsonReader tempReader;
 
-                    for (int i = 0; i < state.Current.PropertyIndex; i++)
+                    FoundProperty[]? properties = argumentState.FoundProperties;
+                    Debug.Assert(properties != null);
+
+                    for (int i = 0; i < argumentState.FoundPropertyCount; i++)
                     {
-                        JsonPropertyInfo jsonPropertyInfo = state.Current.CtorArgumentState!.FoundProperties![i].Item1;
-                        long resumptionByteIndex = state.Current.CtorArgumentState.FoundProperties[i].Item3;
-                        byte[]? propertyNameArray = state.Current.CtorArgumentState.FoundProperties[i].Item4;
-                        string? dataExtKey = state.Current.CtorArgumentState.FoundProperties[i].Item5;
+                        JsonPropertyInfo jsonPropertyInfo = properties[i].Item1;
+                        long resumptionByteIndex = properties[i].Item3;
+                        byte[]? propertyNameArray = properties[i].Item4;
+                        string? dataExtKey = properties[i].Item5;
 
                         tempReader = new Utf8JsonReader(
                             originalSpan.Slice(checked((int)resumptionByteIndex)),
                             isFinalBlock: true,
-                            state: state.Current.CtorArgumentState.FoundProperties[i].Item2);
+                            state: properties[i].Item2);
 
                         Debug.Assert(tempReader.TokenType == JsonTokenType.PropertyName);
 
@@ -65,7 +69,8 @@ namespace System.Text.Json.Serialization.Converters
                         ReadPropertyValue(obj, ref state, ref tempReader, jsonPropertyInfo, useExtensionProperty);
                     }
 
-                    ArrayPool<FoundProperties>.Shared.Return(state.Current.CtorArgumentState!.FoundProperties!, clearArray: true);
+                    ArrayPool<FoundProperty>.Shared.Return(argumentState.FoundProperties!, clearArray: true);
+                    argumentState.FoundProperties = null;
                 }
             }
             else
@@ -86,14 +91,13 @@ namespace System.Text.Json.Serialization.Converters
 
                 obj = CreateObject(ref state.Current);
 
-                if (state.Current.CtorArgumentState!.FoundPropertyCount > 0)
+                if (argumentState.FoundPropertyCount > 0)
                 {
-                    // Set the properties we've parsed so far.
-                    for (int i = 0; i < state.Current.CtorArgumentState!.FoundPropertyCount; i++)
+                    for (int i = 0; i < argumentState.FoundPropertyCount; i++)
                     {
-                        JsonPropertyInfo jsonPropertyInfo = state.Current.CtorArgumentState!.FoundPropertiesAsync![i].Item1;
-                        object? propValue = state.Current.CtorArgumentState!.FoundPropertiesAsync![i].Item2;
-                        string? dataExtKey = state.Current.CtorArgumentState!.FoundPropertiesAsync![i].Item3;
+                        JsonPropertyInfo jsonPropertyInfo = argumentState.FoundPropertiesAsync![i].Item1;
+                        object? propValue = argumentState.FoundPropertiesAsync![i].Item2;
+                        string? dataExtKey = argumentState.FoundPropertiesAsync![i].Item3;
 
                         if (dataExtKey == null)
                         {
@@ -117,7 +121,8 @@ namespace System.Text.Json.Serialization.Converters
                         }
                     }
 
-                    ArrayPool<FoundPropertiesAsync>.Shared.Return(state.Current.CtorArgumentState!.FoundPropertiesAsync!, clearArray: true);
+                    ArrayPool<FoundPropertyAsync>.Shared.Return(argumentState.FoundPropertiesAsync!, clearArray: true);
+                    argumentState.FoundPropertiesAsync = null;
                 }
             }
 
@@ -128,7 +133,7 @@ namespace System.Text.Json.Serialization.Converters
             }
 
             // Check if we are trying to build the sorted parameter cache.
-            if (state.Current.CtorArgumentState!.ParameterRefCache != null)
+            if (argumentState.ParameterRefCache != null)
             {
                 state.Current.JsonClassInfo.UpdateSortedParameterCache(ref state.Current);
             }
@@ -195,31 +200,36 @@ namespace System.Text.Json.Serialization.Converters
                         out _,
                         createExtensionProperty: false);
 
-                    if (state.Current.CtorArgumentState!.FoundProperties == null)
-                    {
-                        state.Current.CtorArgumentState.FoundProperties =
-                            ArrayPool<FoundProperties>.Shared.Rent(Math.Max(1, state.Current.JsonClassInfo.PropertyCache!.Count));
-                    }
-                    else if (state.Current.PropertyIndex - 1 == state.Current.CtorArgumentState.FoundProperties!.Length)
+                    if (jsonPropertyInfo.ShouldDeserialize)
                     {
-                        // Rare case where we can't fit all the JSON properties in the rented pool; we have to grow.
-                        // This could happen if there are duplicate properties in the JSON.
+                        ArgumentState argumentState = state.Current.CtorArgumentState!;
+
+                        if (argumentState.FoundProperties == null)
+                        {
+                            argumentState.FoundProperties =
+                                ArrayPool<FoundProperty>.Shared.Rent(Math.Max(1, state.Current.JsonClassInfo.PropertyCache!.Count));
+                        }
+                        else if (argumentState.FoundPropertyCount == argumentState.FoundProperties.Length)
+                        {
+                            // Rare case where we can't fit all the JSON properties in the rented pool; we have to grow.
+                            // This could happen if there are duplicate properties in the JSON.
 
-                        var newCache = ArrayPool<FoundProperties>.Shared.Rent(state.Current.CtorArgumentState.FoundProperties!.Length * 2);
+                            var newCache = ArrayPool<FoundProperty>.Shared.Rent(argumentState.FoundProperties.Length * 2);
 
-                        state.Current.CtorArgumentState.FoundProperties!.CopyTo(newCache, 0);
+                            argumentState.FoundProperties.CopyTo(newCache, 0);
 
-                        ArrayPool<FoundProperties>.Shared.Return(state.Current.CtorArgumentState.FoundProperties!, clearArray: true);
+                            ArrayPool<FoundProperty>.Shared.Return(argumentState.FoundProperties, clearArray: true);
 
-                        state.Current.CtorArgumentState.FoundProperties = newCache!;
-                    }
+                            argumentState.FoundProperties = newCache!;
+                        }
 
-                    state.Current.CtorArgumentState!.FoundProperties![state.Current.PropertyIndex - 1] = (
-                        jsonPropertyInfo,
-                        reader.CurrentState,
-                        reader.BytesConsumed,
-                        state.Current.JsonPropertyName,
-                        state.Current.JsonPropertyNameAsString);
+                        argumentState.FoundProperties[argumentState.FoundPropertyCount++] = (
+                            jsonPropertyInfo,
+                            reader.CurrentState,
+                            reader.BytesConsumed,
+                            state.Current.JsonPropertyName,
+                            state.Current.JsonPropertyNameAsString);
+                    }
 
                     reader.Skip();
 
@@ -387,30 +397,31 @@ namespace System.Text.Json.Serialization.Converters
                 }
             }
 
+            Debug.Assert(jsonPropertyInfo.ShouldDeserialize);
+
             // Ensure that the cache has enough capacity to add this property.
 
-            if (state.Current.CtorArgumentState!.FoundPropertiesAsync == null)
+            ArgumentState argumentState = state.Current.CtorArgumentState!;
+
+            if (argumentState.FoundPropertiesAsync == null)
             {
-                state.Current.CtorArgumentState.FoundPropertiesAsync =
-                    ArrayPool<FoundPropertiesAsync>.Shared.Rent(Math.Max(1, state.Current.JsonClassInfo.PropertyCache!.Count));
+                argumentState.FoundPropertiesAsync = ArrayPool<FoundPropertyAsync>.Shared.Rent(Math.Max(1, state.Current.JsonClassInfo.PropertyCache!.Count));
             }
-            else if (state.Current.CtorArgumentState.FoundPropertyCount == state.Current.CtorArgumentState.FoundPropertiesAsync!.Length)
+            else if (argumentState.FoundPropertyCount == argumentState.FoundPropertiesAsync!.Length)
             {
                 // Rare case where we can't fit all the JSON properties in the rented pool; we have to grow.
                 // This could happen if there are duplicate properties in the JSON.
-                var newCache = ArrayPool<FoundPropertiesAsync>.Shared.Rent(
-                    state.Current.CtorArgumentState.FoundPropertiesAsync!.Length * 2);
+                var newCache = ArrayPool<FoundPropertyAsync>.Shared.Rent(argumentState.FoundPropertiesAsync!.Length * 2);
 
-                state.Current.CtorArgumentState.FoundPropertiesAsync!.CopyTo(newCache, 0);
+                argumentState.FoundPropertiesAsync!.CopyTo(newCache, 0);
 
-                ArrayPool<FoundPropertiesAsync>.Shared.Return(
-                    state.Current.CtorArgumentState.FoundPropertiesAsync!, clearArray: true);
+                ArrayPool<FoundPropertyAsync>.Shared.Return(argumentState.FoundPropertiesAsync!, clearArray: true);
 
-                state.Current.CtorArgumentState.FoundPropertiesAsync = newCache!;
+                argumentState.FoundPropertiesAsync = newCache!;
             }
 
             // Cache the property name and value.
-            state.Current.CtorArgumentState.FoundPropertiesAsync[state.Current.CtorArgumentState.FoundPropertyCount++] = (
+            argumentState.FoundPropertiesAsync![argumentState.FoundPropertyCount++] = (
                 jsonPropertyInfo,
                 propValue,
                 state.Current.JsonPropertyNameAsString);

diff --git a/src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.ParameterMatching.cs b/src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.ParameterMatching.cs
index b3315fc..91c224d 100644 (file)
--- a/src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.ParameterMatching.cs
+++ b/src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.ParameterMatching.cs
@@ -779,7 +779,7 @@ namespace System.Text.Json.Serialization.Tests
         }
 
         [Fact]
-        public async Task FirstParameterWins()
+        public async Task LastParameterWins()
         {
             Point_2D point = await Serializer.DeserializeWrapper<Point_2D>(@"{""X"":1,""Y"":2,""X"":4}");
             Assert.Equal(4, point.X); // Not 1.
@@ -787,7 +787,7 @@ namespace System.Text.Json.Serialization.Tests
         }
 
         [Fact]
-        public async Task SubsequentParameter_GoesToExtensionData()
+        public async Task LastParameterWins_DoesNotGoToExtensionData()
         {
             string json = @"{
                 ""FirstName"":""Jet"",