Contract metadata cached by the source generator are user-modifiable.

[eiriktsarpalis]

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:

1. 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);

2. 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)

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

---

    That would be this:

JsonTypeInfo<Person> typeInfo = SourceGenContext.Default.Person;

foreach (JsonPropertyInfo property in typeInfo.Properties)
{
    property.Name = property.Name.ToUpperInvariant();
}

string json = JsonSerializer.Serialize(person, typeInfo);
// Fail: regular-case is serialized instead (because fast-path is taken; metadata should be used instead since we have it).
JsonTestHelper.AssertJsonEqual(@"{""FIRSTNAME"":""Jane"",""LASTNAME"":""Doe""}", json); 
Person person = JsonSerializer.Deserialize(json, typeInfo); 
Assert.Equal("Jane", person.FirstName);
Assert.Equal("Doe", person.LastName);

Originally posted by @layomia in #76531 (comment)

Author:	eiriktsarpalis
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[krwq]

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]

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]

A more insidious instantiation of the bug above is that mutating results of JsonSerializerContext.GetTypeInfo can result in corruption of static metadata properties, as illustrated by the example below:

JsonTypeInfo foo = MySerializerContext.Default.GetTypeInfo(typeof(MyClass));
foo.Properties.Clear();

Console.WriteLine(MySerializerContext.Default.MyClass.Properties.Count); // 0, properties dropped from a seemingly unrelated instance

[JsonSerializable(typeof(MyClass))]
public partial class MySerializerContext : JsonSerializerContext
{
}
public class MyClass
{
    public int A0 { get; set; }
}

Bare minimum, we should make sure that modifications don't allow cross-contamination in .NET 7.

[eiriktsarpalis]

Adding a caching layer specifically for GetTypeInfo in JsonSerializerContext does not seem to be an adequate solution, as cross-contamination/races can happen when multiple threads call GetTypeInfo:

Parallel.For(0, 100, i =>
{
    // Simulates multiple threads attempting to independently modify metadata:
    JsonTypeInfo typeInfo = MySerializerContext.Default.GetTypeInfo(typeof(MyClass));
    typeInfo.Properties[0].Name = typeInfo.Properties[0].Name + ".suffix";
});

// Bar.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix.suffix
Console.WriteLine(MySerializerContext.Default.GetTypeInfo(typeof(MyClass)).Properties[0].Name);

[JsonSerializable(typeof(MyClass))]
public partial class MySerializerContext : JsonSerializerContext
{
}

public class MyClass
{
    public int Bar { get; set; }
}