Raising an error upon failure to deserialize overflowing number

[fredfortier]

I found this panic scenario with the Decimal::serde deserializer when fuzz testing.

[fredfortier]

Instead of catching the expected deserialization error, it panicked like this:

        thread 'serde::test::with_overflow' panicked at 'assertion failed: `(left == right)`
        left: `1`,
        right: `0`', src/str.rs:384:5

I would prefer being able to throw any string at Decimal::str rather than implementing a separate validation layer.

[fredfortier]

Since I'm expecting you to say that the use of debug_assert! was a deliberate performance optimization, I went ahead an added a crude validation heuristics at the client layer (in my app, not this library). Please let me know if I just reinvented an existing util, or feel free to comment on the approach. The intent is to capture any problems with the string, as efficiently as possible, before deserializing to Decimal.

// Calculated from Decimal::MAX display length
const MAX_DECIMAL_LEN: usize = 29;

lazy_static! {
    static ref DECIMAL_RE: Regex =
        Regex::new(r"[+-]?((?P<whole>\d+)?(?P<dot>[.]))?(?P<decimal>\d+)")
            .expect("Invalid decimal validation pattern");
}

impl FromStr for DecimalWrapper {
    type Err = anyhow::Error;

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        let cap = DECIMAL_RE
            .captures(s)
            .ok_or(anyhow!("Invalid decimal s={}", s))?;
        // Run basic heuristics on the regex captures before attempting any parsing.
        let whole;
        let fract;
        // Case: 1234.4444
        if let Some(whole_) = cap.name("whole") {
            whole = whole_.as_str();
            fract = &cap["decimal"];
        // Case: .4444
        } else if let Some(dot) = cap.name("dot") {
            whole = "";
            fract = &cap["decimal"];
        // Case: 1234
        } else {
            whole = &cap["decimal"];
            fract = "";
        }
        ensure!(
            fract.len() <= DEFAULT_CURRENCY_DECIMAL_PRECISION as usize,
            "Too many numbers after the decimal len({}) > {}",
            fract,
            DEFAULT_CURRENCY_DECIMAL_PRECISION
        );
        // Effectively reducing the Decimal capacity, which is fine here since our internal threshold is lower.
        ensure!(
            whole.len() < MAX_DECIMAL_LEN,
            "Preventing Decimal overflow len({}) > {}",
            whole,
            MAX_DECIMAL_LEN
        );
        Ok(DecimalWrapper::new(Decimal::from_str(s)?))
    }
}

[paupino]

Thanks for your contribution!

To describe previous functionality, the test:

let res: Result<Decimal, _> = Decimal::from_str(format!("{}.999999", Decimal::MAX).as_str());

would previously return Ok(0). Rounding upon underflow is expected however in this case, it wraps which is unexpected and quite frankly undesired. The bug isn't serde related but rather a generic issue with string parsing.

Regarding a fix: this is an interesting one - in theory it should be throwing an error already with the debug_assert simply providing a sanity check in debug builds. Obviously this is not the case however the question is then - why?

By the looks of it, what is happening is that this code is hitting handle_full_128 due to the large size of the number. Inside handle_full_128 we try to round the underflow however we fail to check the resulting mantissa which in this case has overflowed.

While I think this bug is legit, I'd perhaps fix this in a slightly different way to limit additional checks within handle_data (which is called from a few places). I'll put up an alternative PR shortly.

[paupino]

Closing due to fix in #530