JamesNK / Newtonsoft.Json

Json.NET is a popular high-performance JSON framework for .NET
https://www.newtonsoft.com/json
MIT License
10.71k stars 3.24k forks source link

NRE in JsonValidatingReader() when invoked without a schema #2884

Closed smhmhmd closed 1 year ago

smhmhmd commented 1 year ago

Source JSON

Note: This is an edge case.

10

Expected behavior

Expected exception for bad input such as Error reading JProperty from JsonReader. from Newtonsoft.Json.Linq.JProperty.Load()

Actual behavior

System.NullReferenceException: 'Object reference not set to an instance of an object.'

Steps to reproduce


      System.Byte v2 = (System.Byte)10;
       Newtonsoft.Json.Linq.JToken v3 = (Newtonsoft.Json.Linq.JToken)(System.Byte)v2;

       System.String v4 = ((Newtonsoft.Json.Linq.JToken)v3).Path;
       Newtonsoft.Json.JsonReader v5 = ((Newtonsoft.Json.Linq.JToken)v3).CreateReader();

       Newtonsoft.Json.JsonValidatingReader v6 = new Newtonsoft.Json.JsonValidatingReader((Newtonsoft.Json.JsonReader)v5);

       Newtonsoft.Json.Linq.JsonLoadSettings v10 = new Newtonsoft.Json.Linq.JsonLoadSettings();
       Newtonsoft.Json.Linq.JProperty v11 = Newtonsoft.Json.Linq.JProperty.Load((Newtonsoft.Json.JsonReader)v6, (Newtonsoft.Json.Linq.JsonLoadSettings)v10);
elgonzo commented 1 year ago

The NRE is definitely not very helpful, and i agree that a more meaningful exception should be thrown.

But it's not an edge case related to JProperty.Load as you seem to suspect. It's rather because you use the JsonValidatingReader (which is obsolete, btw) without a schema -- that cannot work. And a meaningful exception other than this unhelpful NRE should hint at that fact.

Basically, the issue can be demonstrated without involving JProperty.Load:

Newtonsoft.Json.Linq.JToken jtoken = Newtonsoft.Json.Linq.JToken.Parse(@"{ ""foo"": ""bar"" }");
Newtonsoft.Json.JsonReader jsonRdr = jtoken.CreateReader();
Newtonsoft.Json.JsonValidatingReader validatingRdr = new Newtonsoft.Json.JsonValidatingReader(jsonRdr);

validatingRdr.Read();

(Dotnetfiddle: https://dotnetfiddle.net/5hXYOi)

...and there you got exactly the same NRE. It's all about reading from a JsonValidatingReader that is lacking a schema to validate against.

(If you would be so kind, it would be nice if you could edit the title of your issue report so as not to put emphasis on JProperty.Load that is distracting from the actual issue.)

smhmhmd commented 1 year ago

a meaningful exception other than this unhelpful NRE should hint at that fact.

If it is possible to add a meaningful exception that would be great, I found one more example where a meaningful exception would be helpful.

smhmhmd commented 1 year ago

(If you would be so kind, it would be nice if you could edit the title of your issue report so as not to put emphasis on JProperty.Load that is distracting from the actual issue.)

I hope the new title is good with you.

elgonzo commented 1 year ago

I hope the new title is good with you.

Thanks! Now keep our fingers crossed that the author and maintainer of Newtonsoft.Json sees and acts upon this issue report...

JamesNK commented 1 year ago

JsonValidatingReader is obsolete.