JamesNK / Newtonsoft.Json

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

Deserialization of decimal allows overflowing large numbers #2848

Open pholodniak opened 1 year ago

pholodniak commented 1 year ago

Source/destination JSON

{"Number":"99999999999999999999999999999"}

Expected behavior

Should throw JsonReaderException with an invalid field name Unhandled exception. Newtonsoft.Json.JsonReaderException: Input string '99999999999999999999999999999' is not a valid decimal. Path 'Number', line 1, position 40.

Actual behavior

Throws Unhandled exception. System.OverflowException: Value was either too large or too small for a Decimal.

Steps to reproduce

public class Example
{
    public decimal Number { get; set; }
}

class Test
{
    static void Main()
    {
        JsonConvert.DeserializeObject<Example>("{\"Number\": 99999999999999999999999999999}");
    }
}
elgonzo commented 1 year ago

Nice catch.

I believe the issue is with the condition in this if statement:

https://github.com/JamesNK/Newtonsoft.Json/blob/0a2e291c0d9c0c7675d445703e51750363a549ef/Src/Newtonsoft.Json/Utilities/ConvertUtils.cs#L1488-L1491

with decimalMaxValueHi28 and decimalMaxValueLo1 defined as:

https://github.com/JamesNK/Newtonsoft.Json/blob/0a2e291c0d9c0c7675d445703e51750363a549ef/Src/Newtonsoft.Json/Utilities/ConvertUtils.cs#L1294

https://github.com/JamesNK/Newtonsoft.Json/blob/0a2e291c0d9c0c7675d445703e51750363a549ef/Src/Newtonsoft.Json/Utilities/ConvertUtils.cs#L1297

Consider that the max value for the decimal type is 79228162514264337593543950335, which is a 29-digit number. The upper/higer 28 digits of this max value is what decimalMaxValueHi28 represents. Therefore, the expression in the if statement attempts to test whether the value from the json exceeds the max value for decimal.

But note how it does an equality comparison with decimalMaxValueHi28. Which looks wrong, as it won't detect any 29-digit number whose upper/higher 28 digits are larger than decimalMaxValueHi28, thus not returning ParseResult.Overflow.

I believe changing the if condition to something like this would fix the issue:

else if ( value > decimalMaxValueHi28 || (value == decimalMaxValueHi28 && digit29 > decimalMaxValueLo1) )
{ 
    return ParseResult.Overflow; 
} 
elgonzo commented 1 year ago

And there is an related issue when trying to consume numbers from 79228162514264337593543950335.5 to 79228162514264337593543950335.9* (or their respective negative counterparts). These should result in a JsonReaderException, but instead also result in a OverflowException.

I believe the issue is with:

https://github.com/JamesNK/Newtonsoft.Json/blob/0a2e291c0d9c0c7675d445703e51750363a549ef/Src/Newtonsoft.Json/Utilities/ConvertUtils.cs#L1501-L1504

which i believe should be changed to something like this:

if (digit29 >= '5' && exponent >= -28)
{
    if (value == decimal.MaxValue)
    {
        return ParseResult.Overflow;
    }

    ++value;
}