Repository navigation
Contract metadata cached by the source generator are user-modifiable. #76535
Description
The inclusion of the contract customization feature exposes a number of APIs that makes it possible for users to modify aspects of the pre-existing JsonTypeInfo contract metadata model. However, it seems that we neglected to freeze modifications for instances instantiated and cached by the source generator:
JsonTypeInfo<MyPoco> metadata = MyContext.Default.MyPoco;
// Can modify metadata on the static `Default` context instance.
metadata.CreateObject = null;
metadata.Properties.Clear();
[JsonSerializable(typeof(MyPoco))]
public partial class MyContext : JsonSerializerContext { }
public class MyPoco
{
public int Id { get; set; }
}At first glance this problem might be considered benign, however it has the potential to create a couple of issues:
-
The source generator is aggressively caching metadata instances for performance, so this is effectively introducing global mutable state. Changes in one component can cause unforeseen changes in an unrelated context:
// Mutate the result of a GetTypeInfo call JsonTypeInfo metadata = MySerializerContext.Default.GetTypeInfo(typeof(MyClass)); metadata .Properties.Clear(); // Change leaks to the static property: Console.WriteLine(MySerializerContext.Default.MyClass.Properties.Count); // 0
or might be the cause of races when multiple threads are independently attempting to modify contracts:
Parallel.For(0, 100, i => { // Simulates multiple threads attempting to independently modify metadata: JsonTypeInfo typeInfo = MySerializerContext.Default.GetTypeInfo(typeof(MyPoco)); typeInfo.Properties[0].Name = typeInfo.Properties[0].Name + ".suffix"; }); // Changes on `GetTypeInfo` results mutate the static instance // Id.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix Console.WriteLine(MySerializerContext.Default.MyPoco.Properties[0].Name);
-
Direct mutation of source gen metadata breaks the fast-path invalidation logic:
MyContext.Default.MyPoco.Properties[0].Name = "alternative_name"; // Modification ignored because the serializer still calls into the fast path that cannot be modified. JsonSerializer.Serialize(new MyPoco { Id= 42 }, MyContext.Default.MyPoco); // { "Id" : 42 }
We should make sure that source generated metadata properties on JsonSerializerContext are locked for modification. Any change should ensure that the IJsonTypeInfoResolver implementation still returns mutable instances, so that contract customization scenaria over source gen are not broken.
Originally posted by @layomia in #76531 (comment)
Activity
krwq commentedon Oct 3, 2022
This is only true until first usage. Make sure to test recursive types here if we decide to fix this. Also I think only .Default JsonTypeInfos should be locked
eiriktsarpalis commentedon Oct 3, 2022
There's another issue: making the instances mutable effectively breaks fast-path delegate invalidation so you can end up with inconsistent serialization outputs. This was the original issue highlighted by @layomia's repro:
#76531 (comment)
eiriktsarpalis commentedon Oct 4, 2022
A more insidious instantiation of the bug above is that mutating results of
JsonSerializerContext.GetTypeInfocan result in corruption of static metadata properties, as illustrated by the example below:Bare minimum, we should make sure that modifications don't allow cross-contamination in .NET 7.
eiriktsarpalis commentedon Oct 6, 2022
Adding a caching layer specifically for
GetTypeInfoinJsonSerializerContextdoes not seem to be an adequate solution, as cross-contamination/races can happen when multiple threads callGetTypeInfo:[-]Static JsonTypeInfo properties in source generated JsonSerializerContext instances are mutable[/-][+]Contract metadata cached by the source generator are user-modifiable.[/+]