From bf77c67151170ae394574d86dd98c3bf87085bfb Mon Sep 17 00:00:00 2001 From: Caio Date: Wed, 13 Oct 2021 11:05:42 -0300 Subject: [PATCH 1/5] Prevent Display implementation overflow --- src/constants.rs | 7 +++++-- src/db.rs | 9 +++++---- src/decimal.rs | 14 +++++++------- src/error.rs | 4 ++-- src/ops/legacy.rs | 20 ++++++++++---------- src/ops/mul.rs | 12 ++++++------ src/str.rs | 17 +++++++++++++++-- 7 files changed, 50 insertions(+), 33 deletions(-) diff --git a/src/constants.rs b/src/constants.rs index ec62ffe7..fcff4d93 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -21,9 +21,12 @@ pub const SIGN_SHIFT: u32 = 31; pub const MAX_STR_BUFFER_SIZE: usize = 32; // The maximum supported precision -pub const MAX_PRECISION: u32 = 28; +pub const MAX_PRECISION: u8 = 28; #[cfg(not(feature = "legacy-ops"))] -pub const MAX_PRECISION_I32: i32 = 28; +// u8 to i32 is infallible, therefore, this cast will never overflow +pub const MAX_PRECISION_I32: i32 = MAX_PRECISION as _; +// u8 to u32 is infallible, therefore, this cast will never overflow +pub const MAX_PRECISION_U32: u32 = MAX_PRECISION as _; // 79,228,162,514,264,337,593,543,950,335 pub const MAX_I128_REPR: i128 = 0x0000_0000_FFFF_FFFF_FFFF_FFFF_FFFF_FFFF; diff --git a/src/db.rs b/src/db.rs index d2be8a8d..63a35f11 100644 --- a/src/db.rs +++ b/src/db.rs @@ -1,4 +1,4 @@ -use crate::constants::MAX_PRECISION; +use crate::constants::MAX_PRECISION_U32; use crate::{ ops::array::{div_by_u32, is_all_zero, mul_by_u32}, Decimal, @@ -64,12 +64,13 @@ impl Decimal { let start_fractionals = if weight < 0 { (-weight as u32) - 1 } else { 0 }; for (i, digit) in digits.into_iter().enumerate() { let fract_pow = 4 * (i as u32 + 1 + start_fractionals); - if fract_pow <= MAX_PRECISION { + if fract_pow <= MAX_PRECISION_U32 { result += Decimal::new(digit as i64, 0) / Decimal::from_i128_with_scale(10i128.pow(fract_pow), 0); - } else if fract_pow == MAX_PRECISION + 4 { + } else if fract_pow == MAX_PRECISION_U32 + 4 { // rounding last digit if digit >= 5000 { - result += Decimal::new(1_i64, 0) / Decimal::from_i128_with_scale(10i128.pow(MAX_PRECISION), 0); + result += + Decimal::new(1_i64, 0) / Decimal::from_i128_with_scale(10i128.pow(MAX_PRECISION_U32), 0); } } } diff --git a/src/decimal.rs b/src/decimal.rs index 8d1b7e20..c5c1d3ec 100644 --- a/src/decimal.rs +++ b/src/decimal.rs @@ -1,5 +1,5 @@ use crate::constants::{ - MAX_I128_REPR, MAX_PRECISION, POWERS_10, SCALE_MASK, SCALE_SHIFT, SIGN_MASK, SIGN_SHIFT, U32_MASK, U8_MASK, + MAX_I128_REPR, MAX_PRECISION_U32, POWERS_10, SCALE_MASK, SCALE_SHIFT, SIGN_MASK, SIGN_SHIFT, U32_MASK, U8_MASK, UNSIGN_MASK, }; use crate::ops; @@ -263,7 +263,7 @@ impl Decimal { /// assert!(max.is_err()); /// ``` pub const fn try_new(num: i64, scale: u32) -> crate::Result { - if scale > MAX_PRECISION { + if scale > MAX_PRECISION_U32 { return Err(Error::ScaleExceedsMaximumPrecision(scale)); } let flags: u32 = scale << SCALE_SHIFT; @@ -323,7 +323,7 @@ impl Decimal { /// assert!(max.is_err()); /// ``` pub const fn try_from_i128_with_scale(num: i128, scale: u32) -> crate::Result { - if scale > MAX_PRECISION { + if scale > MAX_PRECISION_U32 { return Err(Error::ScaleExceedsMaximumPrecision(scale)); } let mut neg = false; @@ -382,7 +382,7 @@ impl Decimal { } else { negative }, - scale % (MAX_PRECISION + 1), + scale % (MAX_PRECISION_U32 + 1), ), } } @@ -442,7 +442,7 @@ impl Decimal { // we've parsed 1.2 as the base and 10 as the exponent. To represent this within a // Decimal type we effectively store the mantissa as 12,000,000,000 and scale as // zero. - if exp > MAX_PRECISION { + if exp > MAX_PRECISION_U32 { return Err(Error::ScaleExceedsMaximumPrecision(exp)); } let mut exp = exp as usize; @@ -606,7 +606,7 @@ impl Decimal { /// assert_eq!(one.to_string(), "0.00001"); /// ``` pub fn set_scale(&mut self, scale: u32) -> Result<(), Error> { - if scale > MAX_PRECISION { + if scale > MAX_PRECISION_U32 { return Err(Error::ScaleExceedsMaximumPrecision(scale)); } self.flags = (scale << SCALE_SHIFT) | (self.flags & SIGN_MASK); @@ -1454,7 +1454,7 @@ impl Decimal { // In order to bring exponent up to -MAX_PRECISION, the mantissa should // be divided by 10 to compensate. If the exponent10 is too small, this // will cause the mantissa to underflow and become 0. - while exponent10 < -(MAX_PRECISION as i32) { + while exponent10 < -(MAX_PRECISION_U32 as i32) { let rem10 = ops::array::div_by_u32(bits, 10); exponent10 += 1; if ops::array::is_all_zero(bits) { diff --git a/src/error.rs b/src/error.rs index b0d65036..0e7613f5 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,4 +1,4 @@ -use crate::constants::MAX_PRECISION; +use crate::constants::MAX_PRECISION_U32; use alloc::string::String; use core::fmt; @@ -38,7 +38,7 @@ impl fmt::Display for Error { write!( f, "Scale exceeds the maximum precision allowed: {} > {}", - scale, MAX_PRECISION + scale, MAX_PRECISION_U32 ) } } diff --git a/src/ops/legacy.rs b/src/ops/legacy.rs index fa137b44..4bb2924e 100644 --- a/src/ops/legacy.rs +++ b/src/ops/legacy.rs @@ -1,5 +1,5 @@ use crate::{ - constants::{MAX_PRECISION, POWERS_10, U32_MASK}, + constants::{MAX_PRECISION_U32, POWERS_10, U32_MASK}, decimal::{CalculationResult, Decimal}, ops::array::{ add_by_internal, cmp_internal, div_by_u32, is_all_zero, mul_by_u32, mul_part, rescale_internal, shl1_internal, @@ -171,16 +171,16 @@ pub(crate) fn div_impl(d1: &Decimal, d2: &Decimal) -> CalculationResult { // Check for underflow let mut final_scale: u32 = quotient_scale as u32; - if final_scale > MAX_PRECISION { + if final_scale > MAX_PRECISION_U32 { let mut remainder = 0; // Division underflowed. We must remove some significant digits over using // an invalid scale. - while final_scale > MAX_PRECISION && !is_all_zero("ient) { + while final_scale > MAX_PRECISION_U32 && !is_all_zero("ient) { remainder = div_by_u32(&mut quotient, 10); final_scale -= 1; } - if final_scale > MAX_PRECISION { + if final_scale > MAX_PRECISION_U32 { // Result underflowed so set to zero final_scale = 0; quotient_negative = false; @@ -228,8 +228,8 @@ pub(crate) fn mul_impl(d1: &Decimal, d2: &Decimal) -> CalculationResult { let mut u64_result = u64_to_array(u64::from(my[0]) * u64::from(ot[0])); // If we're above max precision then this is a very small number - if final_scale > MAX_PRECISION { - final_scale -= MAX_PRECISION; + if final_scale > MAX_PRECISION_U32 { + final_scale -= MAX_PRECISION_U32; // If the number is above 19 then this will equate to zero. // This is because the max value in 64 bits is 1.84E19 @@ -258,7 +258,7 @@ pub(crate) fn mul_impl(d1: &Decimal, d2: &Decimal) -> CalculationResult { u64_result[0] += 1; } - final_scale = MAX_PRECISION; + final_scale = MAX_PRECISION_U32; } return CalculationResult::Ok(Decimal::from_parts( u64_result[0], @@ -350,17 +350,17 @@ pub(crate) fn mul_impl(d1: &Decimal, d2: &Decimal) -> CalculationResult { // If we're still above max precision then we'll try again to // reduce precision - we may be dealing with a limit of "0" - if final_scale > MAX_PRECISION { + if final_scale > MAX_PRECISION_U32 { // We're in an underflow situation // The easiest way to remove precision is to divide off the result - while final_scale > MAX_PRECISION && !is_all_zero(&product) { + while final_scale > MAX_PRECISION_U32 && !is_all_zero(&product) { div_by_u32(&mut product, 10); final_scale -= 1; } // If we're still at limit then we can't represent any // significant decimal digits and will return an integer only // Can also be invoked while representing 0. - if final_scale > MAX_PRECISION { + if final_scale > MAX_PRECISION_U32 { final_scale = 0; } } else if !(product[3] == 0 && product[4] == 0 && product[5] == 0) { diff --git a/src/ops/mul.rs b/src/ops/mul.rs index 972b3a04..b3672959 100644 --- a/src/ops/mul.rs +++ b/src/ops/mul.rs @@ -1,4 +1,4 @@ -use crate::constants::{BIG_POWERS_10, MAX_I64_SCALE, MAX_PRECISION, U32_MAX}; +use crate::constants::{BIG_POWERS_10, MAX_I64_SCALE, MAX_PRECISION_U32, U32_MAX}; use crate::decimal::{CalculationResult, Decimal}; use crate::ops::common::Buf24; @@ -18,15 +18,15 @@ pub(crate) fn mul_impl(d1: &Decimal, d2: &Decimal) -> CalculationResult { if d2.hi() | d2.mid() == 0 { // We're multiplying two 32 bit integers, so we can take some liberties to optimize this. let mut low64 = d1.lo() as u64 * d2.lo() as u64; - if scale > MAX_PRECISION { + if scale > MAX_PRECISION_U32 { // We've exceeded maximum scale so we need to start reducing the precision (aka // rounding) until we have something that fits. // If we're too big then we effectively round to zero. - if scale > MAX_PRECISION + MAX_I64_SCALE { + if scale > MAX_PRECISION_U32 + MAX_I64_SCALE { return CalculationResult::Ok(Decimal::ZERO); } - scale -= MAX_PRECISION + 1; + scale -= MAX_PRECISION_U32 + 1; let mut power = BIG_POWERS_10[scale as usize]; let tmp = low64 / power; @@ -39,7 +39,7 @@ pub(crate) fn mul_impl(d1: &Decimal, d2: &Decimal) -> CalculationResult { low64 += 1; } - scale = MAX_PRECISION; + scale = MAX_PRECISION_U32; } // Early exit @@ -129,7 +129,7 @@ pub(crate) fn mul_impl(d1: &Decimal, d2: &Decimal) -> CalculationResult { // We may want to "rescale". This is the case if the mantissa is > 96 bits or if the scale // exceeds the maximum precision. let upper_word = product.upper_word(); - if upper_word > 2 || scale > MAX_PRECISION { + if upper_word > 2 || scale > MAX_PRECISION_U32 { scale = if let Some(new_scale) = product.rescale(upper_word, scale) { new_scale } else { diff --git a/src/str.rs b/src/str.rs index 9b683728..9b327048 100644 --- a/src/str.rs +++ b/src/str.rs @@ -1,5 +1,5 @@ use crate::{ - constants::MAX_STR_BUFFER_SIZE, + constants::{MAX_PRECISION, MAX_STR_BUFFER_SIZE}, error::Error, ops::array::{add_by_internal, add_one_internal, div_by_u32, is_all_zero, mul_by_10, mul_by_u32}, Decimal, @@ -31,7 +31,7 @@ pub(crate) fn to_str_internal( } let prec = match precision { - Some(prec) => prec, + Some(prec) => prec.min(MAX_PRECISION.into()), None => scale, }; @@ -531,3 +531,16 @@ pub(crate) fn parse_str_radix_n(str: &str, radix: u32) -> Result Date: Wed, 13 Oct 2021 11:11:04 -0300 Subject: [PATCH 2/5] Fix no_std test --- src/str.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/str.rs b/src/str.rs index 9b327048..7dc5cc00 100644 --- a/src/str.rs +++ b/src/str.rs @@ -536,11 +536,12 @@ pub(crate) fn parse_str_radix_n(str: &str, radix: u32) -> Result::new(); let _ = buffer.write_fmt(format_args!("{:.31}", num)); } } From f8d95877d0f2b8849b5b4a258d3ecbf2a0833225 Mon Sep 17 00:00:00 2001 From: Caio Date: Wed, 13 Oct 2021 11:12:40 -0300 Subject: [PATCH 3/5] Rustfmt --- src/str.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/str.rs b/src/str.rs index 7dc5cc00..95c871fa 100644 --- a/src/str.rs +++ b/src/str.rs @@ -535,8 +535,8 @@ pub(crate) fn parse_str_radix_n(str: &str, radix: u32) -> Result Date: Fri, 15 Oct 2021 11:02:36 -0700 Subject: [PATCH 4/5] Branch display logic to allow precision greater than 28 to be used --- src/decimal.rs | 9 +++++++-- src/serde.rs | 3 ++- src/str.rs | 20 ++++++++++++++------ 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/decimal.rs b/src/decimal.rs index c5c1d3ec..8661ff5f 100644 --- a/src/decimal.rs +++ b/src/decimal.rs @@ -2053,8 +2053,13 @@ impl core::convert::TryFrom for f64 { impl fmt::Display for Decimal { fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { - let rep = crate::str::to_str_internal(self, false, f.precision()); - f.pad_integral(self.is_sign_positive(), "", rep.as_str()) + let (rep, additional) = crate::str::to_str_internal(self, false, f.precision()); + if let Some(additional) = additional { + let value = [rep.as_str(), "0".repeat(additional).as_str()].concat(); + f.pad_integral(self.is_sign_positive(), "", value.as_str()) + } else { + f.pad_integral(self.is_sign_positive(), "", rep.as_str()) + } } } diff --git a/src/serde.rs b/src/serde.rs index 73b1601e..e3882ffa 100644 --- a/src/serde.rs +++ b/src/serde.rs @@ -174,7 +174,8 @@ impl serde::Serialize for Decimal { where S: serde::Serializer, { - serializer.serialize_str(crate::str::to_str_internal(self, true, None).as_ref()) + let value = crate::str::to_str_internal(self, true, None); + serializer.serialize_str(value.0.as_ref()) } } diff --git a/src/str.rs b/src/str.rs index 95c871fa..6903b482 100644 --- a/src/str.rs +++ b/src/str.rs @@ -15,7 +15,7 @@ pub(crate) fn to_str_internal( value: &Decimal, append_sign: bool, precision: Option, -) -> ArrayString<[u8; MAX_STR_BUFFER_SIZE]> { +) -> (ArrayString<[u8; MAX_STR_BUFFER_SIZE]>, Option) { // Get the scale - where we need to put the decimal point let scale = value.scale() as usize; @@ -30,9 +30,16 @@ pub(crate) fn to_str_internal( chars.push('0'); } - let prec = match precision { - Some(prec) => prec.min(MAX_PRECISION.into()), - None => scale, + let (prec, additional) = match precision { + Some(prec) => { + let max: usize = MAX_PRECISION.into(); + if prec > max { + (max, Some(prec - max)) + } else { + (prec, None) + } + } + None => (scale, None), }; let len = chars.len(); @@ -66,7 +73,7 @@ pub(crate) fn to_str_internal( rep.push('0'); } - rep + (rep, additional) } pub(crate) fn fmt_scientific_notation( @@ -542,6 +549,7 @@ mod test { fn display_does_not_overflow_max_capacity() { let num = Decimal::from_str("1.2").unwrap(); let mut buffer = ArrayString::<[u8; 64]>::new(); - let _ = buffer.write_fmt(format_args!("{:.31}", num)); + let _ = buffer.write_fmt(format_args!("{:.31}", num)).unwrap(); + assert_eq!("1.2000000000000000000000000000000", buffer.as_str()); } } From 7e0fa3945e89b84d3312480149ebc132b4618fc1 Mon Sep 17 00:00:00 2001 From: Paul Mason Date: Fri, 15 Oct 2021 14:12:32 -0700 Subject: [PATCH 5/5] Bind deserialize to a maximum precision --- src/decimal.rs | 18 +++++++++++++++--- tests/decimal_tests.rs | 17 +++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/src/decimal.rs b/src/decimal.rs index 8661ff5f..ebc504a0 100644 --- a/src/decimal.rs +++ b/src/decimal.rs @@ -691,13 +691,25 @@ impl Decimal { /// * Bytes 9-12: mid portion of `m` /// * Bytes 13-16: high portion of `m` #[must_use] - pub const fn deserialize(bytes: [u8; 16]) -> Decimal { - Decimal { - flags: (bytes[0] as u32) | (bytes[1] as u32) << 8 | (bytes[2] as u32) << 16 | (bytes[3] as u32) << 24, + pub fn deserialize(bytes: [u8; 16]) -> Decimal { + // We can bound flags by a bitwise mask to correspond to: + // Bits 0-15: unused + // Bits 16-23: Contains "e", a value between 0-28 that indicates the scale + // Bits 24-30: unused + // Bit 31: the sign of the Decimal value, 0 meaning positive and 1 meaning negative. + let raw = Decimal { + flags: ((bytes[0] as u32) | (bytes[1] as u32) << 8 | (bytes[2] as u32) << 16 | (bytes[3] as u32) << 24) + & 0x801F_0000, lo: (bytes[4] as u32) | (bytes[5] as u32) << 8 | (bytes[6] as u32) << 16 | (bytes[7] as u32) << 24, mid: (bytes[8] as u32) | (bytes[9] as u32) << 8 | (bytes[10] as u32) << 16 | (bytes[11] as u32) << 24, hi: (bytes[12] as u32) | (bytes[13] as u32) << 8 | (bytes[14] as u32) << 16 | (bytes[15] as u32) << 24, + }; + // Scale must be bound to maximum precision. The panic currently prevents this function being + // marked as a const function. + if raw.scale() > MAX_PRECISION_U32 { + panic!("{}", Error::ScaleExceedsMaximumPrecision(raw.scale())) } + raw } /// Returns `true` if the decimal is negative. diff --git a/tests/decimal_tests.rs b/tests/decimal_tests.rs index 11477ca0..15bdc4f5 100644 --- a/tests/decimal_tests.rs +++ b/tests/decimal_tests.rs @@ -132,6 +132,23 @@ fn it_can_serialize_deserialize() { } } +#[test] +#[should_panic(expected = "Scale exceeds the maximum precision allowed: 30 > 28")] +fn it_panics_deserializing_unbounded_values() { + let _ = Decimal::deserialize([1u8, 0, 30, 206, 97, 81, 216, 182, 20, 30, 165, 78, 18, 155, 169, 62]); +} + +#[test] +fn it_can_deserialize_bounded_values() { + let tests = [[1u8, 0, 28, 206, 97, 81, 216, 182, 20, 30, 165, 78, 18, 155, 169, 62]]; + for &bytes in &tests { + let dec = Decimal::deserialize(bytes); + let string = format!("{:.9999}", dec); + let dec2 = Decimal::from_str(&string).unwrap(); + assert_eq!(dec, dec2); + } +} + // Formatting #[test]