Unexpected exceptions during deserialization due to missing constructor name

NullReferenceException

Sample code

using Newtonsoft.Json;

try {
    Console.WriteLine(JsonConvert.DeserializeObject("[new \0("));
} catch (JsonException) {
    Console.WriteLine("invalid JSON");
}

Expected/actual behavior

Should print "invalid JSON". Instead crashes with a NullReferenceException.

Analysis

The crash happens on the expression _parent!.Parent, so the damnit operator here is inaccurate.

The sequence of events is:

JTokenWriter.WriteStartConstructor invokes AddParent(new JConstructor(name)) with an empty name. This throws an ArgumentException, so AddParent is not invoked, even though WriteStartConstructor has successfully been called
JsonWriter.Dispose is invoked, which does Close → AutoCompleteAll → WriteEnd → WriteEndConstructor → InternalWriteEnd → AutoCompleteClose → JTokenWriter.WriteEnd → JTokenWriter.RemoveParent, removing a level of parenting that doesn't exist

Proposed fixes?

Moving the new JConstructor call to the start of WriteStartConstructor validates the name before WriteStartConstructor is invoked. This stops the NRE but still raises an ArgumentException instead of a JsonException. Probably the validation of the constructor name should happen much earlier, during reading not writing.

Also, should JsonWriter be completing its work if there is an exception in-flight?

ArgumentException

It is also possible to surface the ArgumentException itself via:

using Newtonsoft.Json;

try {
    Console.WriteLine(JsonConvert.DeserializeObject("{' ':new \0("));
} catch (JsonException) {
    Console.WriteLine("invalid JSON");
}

Expected/actual behavior

Should print "invalid JSON". Instead crashes with a ArgumentException ("Constructor name cannot be empty").

Analysis

Underlying cause is the same as previous but this one avoids the call to Dispose so it doesn’t trigger the NRE.

Credit: Found via LibFuzzer & SharpFuzz.

JamesNK / Newtonsoft.Json