Newtonsoft.Json icon indicating copy to clipboard operation
Newtonsoft.Json copied to clipboard

Deserialization of decimal allows overflowing large numbers

Open pholodniak opened this issue 1 year ago • 2 comments

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}");
    }
}

pholodniak avatar Apr 11 '23 14:04 pholodniak

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 avatar Apr 11 '23 17:04 elgonzo

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;
}

elgonzo avatar Apr 11 '23 17:04 elgonzo