From ffa06471f6e5fac939533c7541ad31321089cf0a Mon Sep 17 00:00:00 2001 From: Layomi Akinrinade Date: Fri, 7 Aug 2020 10:52:37 -0700 Subject: [PATCH] [JsonSerializer] Prevent arbitrary properties from being cached when deserializing with parameterized ctors (#40495) * [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 --- .../ObjectWithParameterizedConstructorConverter.cs | 107 ++++++++++++--------- .../ConstructorTests.ParameterMatching.cs | 4 +- 2 files changed, 61 insertions(+), 50 deletions(-) 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 --- 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; -using FoundPropertiesAsync = System.ValueTuple; +using FoundProperty = System.ValueTuple; +using FoundPropertyAsync = System.ValueTuple; 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.Shared.Return(state.Current.CtorArgumentState!.FoundProperties!, clearArray: true); + ArrayPool.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.Shared.Return(state.Current.CtorArgumentState!.FoundPropertiesAsync!, clearArray: true); + ArrayPool.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.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.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.Shared.Rent(state.Current.CtorArgumentState.FoundProperties!.Length * 2); + var newCache = ArrayPool.Shared.Rent(argumentState.FoundProperties.Length * 2); - state.Current.CtorArgumentState.FoundProperties!.CopyTo(newCache, 0); + argumentState.FoundProperties.CopyTo(newCache, 0); - ArrayPool.Shared.Return(state.Current.CtorArgumentState.FoundProperties!, clearArray: true); + ArrayPool.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.Shared.Rent(Math.Max(1, state.Current.JsonClassInfo.PropertyCache!.Count)); + argumentState.FoundPropertiesAsync = ArrayPool.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.Shared.Rent( - state.Current.CtorArgumentState.FoundPropertiesAsync!.Length * 2); + var newCache = ArrayPool.Shared.Rent(argumentState.FoundPropertiesAsync!.Length * 2); - state.Current.CtorArgumentState.FoundPropertiesAsync!.CopyTo(newCache, 0); + argumentState.FoundPropertiesAsync!.CopyTo(newCache, 0); - ArrayPool.Shared.Return( - state.Current.CtorArgumentState.FoundPropertiesAsync!, clearArray: true); + ArrayPool.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 --- 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(@"{""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"", -- 2.7.4