Why the JsonSerializerProxy's override PopulateInternal method doesn't call the SetupReader method like its base one

[orrindeng]

https://github.com/JamesNK/Newtonsoft.Json/blob/0a2e291c0d9c0c7675d445703e51750363a549ef/Src/Newtonsoft.Json/Serialization/JsonSerializerProxy.cs#L265

What I met: We use JsonConverter to do something when deserializing. But when I debug into the ReadJson method.

public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer)

I found the serializer is actually an instance of JsonSerializerProxy. And its PopulateInternal doesn't call the SetupReader as its base class JsonSerializer.

Base PopulateInternal method:

Override PopulateInternal method:

What we need to do is every time when I write a new JsonConverter I have to manually set the reader's MaxDepth property. It seems easy to forget to do that.

So why not we call SetupReader method in the override PopulateInternal method?

[elgonzo]

(Disclaimer: I am not involved with the Newtonsoft.Json project or its author and maintainer, i am just a user. Thus, i am not speaking with authority.)

It's more of a tangent and not directly addressing your question regarding JsonSerializerProxy, but i am a bit confused about your last code screenshot/snippet, which seems to be the origin or motiviation for your question / issue report here.

It looks like you got a JObject instance already. Where did you get the JObject data from? To me, it makes little sense to account for MaxDepth here.

Your real-world situation is probably more complicated than the short last code snippet you have shown here, but take note that MaxDepth's purpose is to provide a mechanism to mitigate DOS and other attack attempts or failure scenarios by avoiding stack overflow and possibly out-of-memory situations occuring within the Newtonsoft.Json library itself (see here: https://github.com/JamesNK/Newtonsoft.Json/issues/2457). The critical situations MaxDepth addresses aren't an issue with JToken's. The possible critical situations MaxDepth addresses are at the point when (untrusted/unknown) json data from external/untrusted/unreliable sources is being read (that would also apply to reading the json data for creating the JObject instance jsonObject), not after the json data has been read already.

[orrindeng]

@elgonzo Thanks for you explanation. I've learned much about the design of MaxDepth setting.

It looks like you got a JObject instance already. Where did you get the JObject data from? To me, it makes little sense to account for MaxDepth here.

I'm not sure it is a correct way to use JsonConverter or not. But during the Json deserialization, sometimes we want to do some special things to avoid unexpected deserialization. Just like the following JsonConverter doing.

    public class CommandListConverter : JsonConverter
    {
        public static CommandListConverter Instance { get; set; } = new CommandListConverter();

        public override bool CanConvert(Type objectType)
        {
            return typeof(IEnumerable<Command>).IsAssignableFrom(objectType);
        }

        public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer)
        {
            // Here we must pass the MaxDepth to the reader every time
            reader.MaxDepth = serializer.MaxDepth;

            var tokenArray = (JArray)JToken.Load(reader);

            var genericType = objectType.GenericTypeArguments.First();

            var listType = typeof(List<>).MakeGenericType(genericType);

            var list = existingValue as IList ?? (IList)Activator.CreateInstance(listType);

            foreach (var childToken in tokenArray)
            {
                if (childToken is JObject objToken)
                {
                    if (objToken.TryGetValue(nameof(Command.Disabled), out var isDisabled) && isDisabled.ToObject<bool>())
                    {
                        continue;
                    }

                    if (objToken.ContainsKey("$type"))
                    {
                        var obj = serializer.Deserialize(childToken.CreateReader());
                        list.Add(obj);
                    }
                    else
                    {
                        var obj = serializer.Deserialize(childToken.CreateReader(), genericType);
                        list.Add(obj);
                    }
                }
            }

            return list;
        }

        public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
        {
            throw new NotImplementedException();
        }
    }

This converter is doing something to pruning things to avoid unnecessary (in this case it's 'Disabled' property) deserialization. You can see every time when ReadJson, I must set the MaxDepth to avoid the MaxDepth limit. Because the serializer with the type JsonSerializerProxy doesn't use SetupReader method to pass the MaxDepth setting to the JsonReader.