JamesNK / Newtonsoft.Json

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

Divide by zero exception in Newtonsoft.Json.Utilities.BoxedPrimitives.Get() method #2869

Open aurax opened 1 year ago

aurax commented 1 year ago

Source/destination types

[Serializable]
internal class ImportData {
        public string FilePath { get; set; }
        public double Scale { get; set; }
        public Point Origin { get; set; }
        public string Name { get; set; }
}
[Serializable]
public sealed class Point {
        [JsonConstructor]
        public Point(double x, double y, double z) {
            _x = x;
            _y = y;
            _z = z;
        }
        private readonly double _x;
        private readonly double _y;
        private readonly double _z;

        public double X => _x;
        public double Y => _y;
        public double Z => _z;
}

Source/destination JSON

{ "aaa": {
        "FilePath":"file:///C:/temp/aaa.txt",
        "Scale":2.0,
        "Origin": {
            "X":0.0,
            "Y":0.0,
            "Z":0.0
        },
        "Name":"aaa"
    }
}

Expected behavior

Version 13.0.2 correctly deserializes this.

Actual behavior

System.DivideByZeroException : Attempted to divide by zero in Newtonsoft.Json\Utilities\BoxedPrimitives.cs, line 135

image

JamesNK commented 1 year ago

I couldn't replicate this in a test. See below.

Also, I don't see how this error is possible. You can't get DivideByZeroException when dividing by a zero double. The result is either positive or negative infinity.

        [Test]
        public void DeserializeZeroIssue()
        {
            var json = @"{ ""aaa"": {
        ""FilePath"":""file:///C:/temp/aaa.txt"",
        ""Scale"":2.0,
        ""Origin"": {
            ""X"":0.0,
            ""Y"":0.0,
            ""Z"":0.0
        },
        ""Name"":""aaa""
    }
}";

            JsonConvert.DeserializeObject<ImportData>(json);
        }

        [Serializable]
        internal class ImportData
        {
            public string FilePath { get; set; }
            public double Scale { get; set; }
            public Point Origin { get; set; }
            public string Name { get; set; }
        }
        [Serializable]
        public sealed class Point
        {
            [JsonConstructor]
            public Point(double x, double y, double z)
            {
                _x = x;
                _y = y;
                _z = z;
            }
            private readonly double _x;
            private readonly double _y;
            private readonly double _z;

            public double X => _x;
            public double Y => _y;
            public double Z => _z;
        }
elgonzo commented 1 year ago

Did you built Newtonsoft.Json from source? Or did you use the nuget package (or DLL) of the release build of Newtonsoft.Json 13.0.and then tried to reference it with the respective source code? It almost looks to me you did the latter. What is the actual full stacktrace of the exception you got?

aurax commented 1 year ago

Hello @elgonzo , I use Newtonsoft.Json as a nuget package and add it as a reference: <PackageReference Include="Newtonsoft.Json" Version="13.0.3" /> Yep, I didn't add the call stack, sorry, here it is::

System.DivideByZeroException
  HResult=0x80020012
  Message=Attempted to divide by zero.
  Source=Newtonsoft.Json
  StackTrace:
   at Newtonsoft.Json.Utilities.BoxedPrimitives.Get(Double value)
   at Newtonsoft.Json.JsonTextReader.ParseReadNumber(ReadType readType, Char firstChar, Int32 initialPosition)
   at Newtonsoft.Json.JsonTextReader.ParseNumber(ReadType readType)
   at Newtonsoft.Json.JsonTextReader.ReadNumberValue(ReadType readType)
   at Newtonsoft.Json.JsonTextReader.ReadAsDouble()
   at Newtonsoft.Json.JsonReader.ReadForType(JsonContract contract, Boolean hasConverter)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.ResolvePropertyAndCreatorValues(JsonObjectContract contract, JsonProperty containerProperty, JsonReader reader, Type objectType)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObjectUsingCreatorWithParameters(JsonReader reader, JsonObjectContract contract, JsonProperty containerProperty, ObjectConstructor`1 creator, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateNewObject(JsonReader reader, JsonObjectContract objectContract, JsonProperty containerMember, JsonProperty containerProperty, String id, Boolean& createdFromNonDefaultCreator)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.SetPropertyValue(JsonProperty property, JsonConverter propertyConverter, JsonContainerContract containerContract, JsonProperty containerProperty, JsonReader reader, Object target)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateObject(Object newObject, JsonReader reader, JsonObjectContract contract, JsonProperty member, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateDictionary(IDictionary dictionary, JsonReader reader, JsonDictionaryContract contract, JsonProperty containerProperty, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
   at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonSerializer.Deserialize(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonConvert.DeserializeObject(String value, Type type, JsonSerializerSettings settings)
   at Newtonsoft.Json.JsonConvert.DeserializeObject[T](String value, JsonSerializerSettings settings)

  This exception was originally thrown at this call stack:
    Newtonsoft.Json.Utilities.BoxedPrimitives.Get(double)
    Newtonsoft.Json.JsonTextReader.ParseReadNumber(Newtonsoft.Json.ReadType, char, int)
    Newtonsoft.Json.JsonTextReader.ParseNumber(Newtonsoft.Json.ReadType)
    Newtonsoft.Json.JsonTextReader.ReadNumberValue(Newtonsoft.Json.ReadType)
    Newtonsoft.Json.JsonTextReader.ReadAsDouble()
    Newtonsoft.Json.JsonReader.ReadForType(Newtonsoft.Json.Serialization.JsonContract, bool)
    Newtonsoft.Json.Serialization.JsonSerializerInternalReader.ResolvePropertyAndCreatorValues(Newtonsoft.Json.Serialization.JsonObjectContract, Newtonsoft.Json.Serialization.JsonProperty, Newtonsoft.Json.JsonReader, System.Type)
    Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObjectUsingCreatorWithParameters(Newtonsoft.Json.JsonReader, Newtonsoft.Json.Serialization.JsonObjectContract, Newtonsoft.Json.Serialization.JsonProperty, Newtonsoft.Json.Serialization.ObjectConstructor<object>, string)
    Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateNewObject(Newtonsoft.Json.JsonReader, Newtonsoft.Json.Serialization.JsonObjectContract, Newtonsoft.Json.Serialization.JsonProperty, Newtonsoft.Json.Serialization.JsonProperty, string, out bool)
    Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(Newtonsoft.Json.JsonReader, System.Type, Newtonsoft.Json.Serialization.JsonContract, Newtonsoft.Json.Serialization.JsonProperty, Newtonsoft.Json.Serialization.JsonContainerContract, Newtonsoft.Json.Serialization.JsonProperty, object)
    ...
    [Call Stack Truncated]
aurax commented 1 year ago

Hello @JamesNK ,

I've tested more. An exception is not thrown if the JsonConvert.DeserializeObject method is called from a NUnit based project but the exception is raised if this method is called from a library. I tried to make a test solution with similar settings but can't reproduce the problem in this environment. Unfortunately, the problem appears only in my solution.

sungam3r commented 1 year ago

What is target framework where you see exception?

akaranta commented 1 year ago

@JamesNK is correct in that dividing by zero double does not result in an exception normally. However, in the environment where @aurax encountered the issue we have set floating point error flags on the processor to on to catch problems in floating point calculations. This unfortunately occasionally results in exceptions from libraries, as most projects do not have the said checks on.

To turn the checks on, you can do like so (on Windows):

// see the documentation for _controlfp_s at https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/controlfp-s?view=vs-2019
[System.Runtime.InteropServices.DllImport("msvcrt.dll")] private static extern int _controlfp_s(ref uint currentControl, uint newControl, uint mask);

public const uint ERROR_MASK = _EM_INVALID | _EM_ZERODIVIDE | _EM_OVERFLOW;

public static uint TurnOnFpExceptions(uint mask = ERROR_MASK) { var originalState = GetFpState(); ControlFp(0, mask); return originalState; }

And then call TurnOnFpExceptions before running the test.

akaranta commented 1 year ago

Sorry, forgot these: public const uint _EM_INVALID = 0x00000010, _EM_DENORMAL = 0x00080000, _EM_ZERODIVIDE = 0x00000008, _EM_OVERFLOW = 0x00000004, _EM_UNDERFLOW = 0x00000002, _EM_INEXACT = 0x00000001;

akaranta commented 1 year ago

And these. Seems I'm bad at trying to copy a minimal set of code...

    private static uint ControlFp(uint state, uint mask) {
        uint currentControl = 0;
        int err = _controlfp_s(ref currentControl, state, mask);
        if (err == 0) return currentControl;
        throw new Exception("Native call to _controlfp_s resulted in error " + err);
    }

    public static uint GetFpState() => ControlFp(0, 0);
elgonzo commented 1 year ago

@akaranta

However, in the environment where @aurax encountered the issue we have set floating point error flags on the processor to on to catch problems in floating point calculations. This unfortunately occasionally results in exceptions from libraries, as most projects do not have the said checks on.

I haven't verified myself, but if what you described has the observed effect, then this essentially enables behavior that is in conflict with the C# language spec (ECMA-334) and the CLR Infrastructure spec (ECMA-335). I don't know whether this would be a case for the C# language and CLR team(s) to consider. However, as of now, doing what you described apparently resulting in a runtime environment that is in violation of the C# language and CLR spec is not something a 3rd-party library should be expected to deal with, unless it was specifically made to operate in such spec-violating environments.

I do not know what project "we" is, but under the assumption @aurax is using this unknown referred to as "we", they either need to prevent "we" from doing this (at least with respect to directly or indirectly calling into any Newtonsoft.Json library code), or -- if they need to keep using Newtonsoft.Json and they can't prevent "we" from doing this -- refrain from using "we".

Baardi commented 1 year ago

I'm getting the exact same problems, when using newtonsoft.json 13.0.3 as a nuget package in a .net framework 4.8 project. 13.0.2 runs fine

elgonzo commented 1 year ago

@Baardi that's not a problem with Newtonsoft.Json, though.

Either your program or some other 3rd-party library you are using calls some native function (directly or indirectly) that twiddles with CPU configuration registers (or something similar to this effect). And if that is done in a manner that turns the runtime environment of your program non-conforming with respect to the CLR Infrastructure specification and the C# language spec, then that's not a problem with Newtonsoft.Json but rather with whatever code that does this twiddling. As per CLR Infrastructure spec (ECMA-335, section III.3.31) and C# language spec (ECMA-334, section 11.9.3), a floating point division by zero is not supposed to throw any exception.

Baardi commented 1 year ago

In this case, the program is a native program, interfacing with CLR using COM. Changing the integration CLI/C++ doesn't change anything.

elgonzo commented 1 year ago

@Baardi try registering your .NET-based COM component not as an in-process COM server, but as an out-of-process COM server (thus separating and isolating your .NET component from the runtime environment of the application). If your COM component is well-behaved, a simple DLL surrogate might be sufficient. Don't ask me for technical details, though, it's too many years since i so much as glanced at anything COM-related...

putto72 commented 1 month ago

We are experiencing a similar problem with a legacy C# Net Framework 4.7.2 application; divide by zero when calling JObject.Parse on a perfectly valid json. We are using Newtonsoft 13.0.3 through NuGet. When using 13.0.2, we get an overflow exception. Everything works correctly when using 13.0.1. The strange thing is that the exception happens only on the first Parse; later calls have no problem. Our application include lot of 3rd party assemblies and even COM components that we cannot troubleshoot to find the culprit. If the fix is just adding a check in the BoxedPrimitives Get function I believe that, even if it is not really necessary "by the book" it would be nice for people out there that are having troubles getting around this exception and need to stick to previous versions.