From 1e104b7ef99e4f971a6d70e8afec96223311eb16 Mon Sep 17 00:00:00 2001 From: Nicolas Boichat Date: Fri, 21 Mar 2025 11:50:58 +0100 Subject: [PATCH] uucore: format: num_parser: Add value to Overflow error When parsing integers, we should still return the min/max value of the type (depending on the type), and wrap that in the error. We need to refactor the map function to handle this case better, and add an extract function to get the value out of an error (if any). This fixes the integer part of #7508. --- .../src/lib/features/format/argument.rs | 4 +- .../src/lib/features/format/num_parser.rs | 132 ++++++++++++------ tests/by-util/test_printf.rs | 13 ++ 3 files changed, 107 insertions(+), 42 deletions(-) diff --git a/src/uucore/src/lib/features/format/argument.rs b/src/uucore/src/lib/features/format/argument.rs index e1877de3d..0be53f457 100644 --- a/src/uucore/src/lib/features/format/argument.rs +++ b/src/uucore/src/lib/features/format/argument.rs @@ -105,9 +105,9 @@ fn extract_value(p: Result>, input: &s }, ); match e { - ExtendedParserError::Overflow => { + ExtendedParserError::Overflow(v) => { show_error!("{}: Numerical result out of range", input.quote()); - Default::default() + v } ExtendedParserError::NotNumeric => { show_error!("{}: expected a numeric value", input.quote()); diff --git a/src/uucore/src/lib/features/format/num_parser.rs b/src/uucore/src/lib/features/format/num_parser.rs index 98cbd105a..648506c3e 100644 --- a/src/uucore/src/lib/features/format/num_parser.rs +++ b/src/uucore/src/lib/features/format/num_parser.rs @@ -59,21 +59,48 @@ pub enum ExtendedParserError<'a, T> { /// The beginning of the input made sense and has been parsed, /// while the remaining doesn't. PartialMatch(T, &'a str), - /// The integral part has overflowed the requested type, or - /// has overflowed the `u64` internal storage when parsing the - /// integral part of a floating point number. - Overflow, + /// The value has overflowed the type storage. The returned value + /// is saturated (e.g. positive or negative infinity, or min/max + /// value for the integer type). + Overflow(T), } -impl<'a, T> ExtendedParserError<'a, T> { +impl<'a, T> ExtendedParserError<'a, T> +where + T: Zero, +{ + // Extract the value out of an error, if possible. + fn extract(self) -> T { + match self { + Self::NotNumeric => T::zero(), + Self::Overflow(v) => v, + Self::PartialMatch(v, _) => v, + } + } + + // Map an error to another, using the provided conversion function. + // The error (self) takes precedence over errors happening during the + // conversion. fn map( self, - f: impl FnOnce(T, &'a str) -> ExtendedParserError<'a, U>, - ) -> ExtendedParserError<'a, U> { + f: impl FnOnce(T) -> Result>, + ) -> ExtendedParserError<'a, U> + where + U: Zero, + { + fn extract(v: Result>) -> U + where + U: Zero, + { + v.unwrap_or_else(|e| e.extract()) + } + match self { - Self::NotNumeric => ExtendedParserError::NotNumeric, - Self::Overflow => ExtendedParserError::Overflow, - Self::PartialMatch(v, s) => f(v, s), + ExtendedParserError::NotNumeric => ExtendedParserError::NotNumeric, + ExtendedParserError::PartialMatch(v, rest) => { + ExtendedParserError::PartialMatch(extract(f(v)), rest) + } + ExtendedParserError::Overflow(v) => ExtendedParserError::Overflow(extract(f(v))), } } } @@ -92,28 +119,34 @@ pub trait ExtendedParser { impl ExtendedParser for i64 { /// Parse a number as i64. No fractional part is allowed. fn extended_parse(input: &str) -> Result> { - fn into_i64(ebd: ExtendedBigDecimal) -> Option { + fn into_i64<'a>(ebd: ExtendedBigDecimal) -> Result> { match ebd { ExtendedBigDecimal::BigDecimal(bd) => { let (digits, scale) = bd.into_bigint_and_scale(); if scale == 0 { - i64::try_from(digits).ok() + let negative = digits.sign() == Sign::Minus; + match i64::try_from(digits) { + Ok(i) => Ok(i), + _ => Err(ExtendedParserError::Overflow(if negative { + i64::MIN + } else { + i64::MAX + })), + } } else { - None + // Should not happen. + Err(ExtendedParserError::NotNumeric) } } - ExtendedBigDecimal::MinusZero => Some(0), - _ => None, + ExtendedBigDecimal::MinusZero => Ok(0), + // No other case should not happen. + _ => Err(ExtendedParserError::NotNumeric), } } match parse(input, true) { - Ok(v) => into_i64(v).ok_or(ExtendedParserError::Overflow), - Err(e) => Err(e.map(|v, rest| { - into_i64(v) - .map(|v| ExtendedParserError::PartialMatch(v, rest)) - .unwrap_or(ExtendedParserError::Overflow) - })), + Ok(v) => into_i64(v), + Err(e) => Err(e.map(into_i64)), } } } @@ -121,27 +154,35 @@ impl ExtendedParser for i64 { impl ExtendedParser for u64 { /// Parse a number as u64. No fractional part is allowed. fn extended_parse(input: &str) -> Result> { - fn into_u64(ebd: ExtendedBigDecimal) -> Option { + fn into_u64<'a>(ebd: ExtendedBigDecimal) -> Result> { match ebd { ExtendedBigDecimal::BigDecimal(bd) => { let (digits, scale) = bd.into_bigint_and_scale(); if scale == 0 { - u64::try_from(digits).ok() + let negative = digits.sign() == Sign::Minus; + match u64::try_from(digits) { + Ok(i) => Ok(i), + _ => Err(ExtendedParserError::Overflow(if negative { + // TODO: We should wrap around here #7488 + 0 + } else { + u64::MAX + })), + } } else { - None + // Should not happen. + Err(ExtendedParserError::NotNumeric) } } - _ => None, + // TODO: Handle -0 too #7488 + // No other case should not happen. + _ => Err(ExtendedParserError::NotNumeric), } } match parse(input, true) { - Ok(v) => into_u64(v).ok_or(ExtendedParserError::Overflow), - Err(e) => Err(e.map(|v, rest| { - into_u64(v) - .map(|v| ExtendedParserError::PartialMatch(v, rest)) - .unwrap_or(ExtendedParserError::Overflow) - })), + Ok(v) => into_u64(v), + Err(e) => Err(e.map(into_u64)), } } } @@ -149,21 +190,23 @@ impl ExtendedParser for u64 { impl ExtendedParser for f64 { /// Parse a number as f64 fn extended_parse(input: &str) -> Result> { - // TODO: This is generic, so this should probably be implemented as an ExtendedBigDecimal trait (ToPrimitive). - fn into_f64(ebd: ExtendedBigDecimal) -> f64 { - match ebd { + fn into_f64<'a>(ebd: ExtendedBigDecimal) -> Result> { + // TODO: This is generic, so this should probably be implemented as an ExtendedBigDecimal trait (ToPrimitive). + let v = match ebd { + // TODO: bd -> f64 conversion can fail, handle that. ExtendedBigDecimal::BigDecimal(bd) => bd.to_f64().unwrap(), ExtendedBigDecimal::MinusZero => -0.0, ExtendedBigDecimal::Nan => f64::NAN, ExtendedBigDecimal::MinusNan => -f64::NAN, ExtendedBigDecimal::Infinity => f64::INFINITY, ExtendedBigDecimal::MinusInfinity => -f64::INFINITY, - } + }; + Ok(v) } match parse(input, false) { - Ok(v) => Ok(into_f64(v)), - Err(e) => Err(e.map(|v, rest| ExtendedParserError::PartialMatch(into_f64(v), rest))), + Ok(v) => into_f64(v), + Err(e) => Err(e.map(into_f64)), } } } @@ -390,9 +433,10 @@ mod tests { fn test_decimal_u64() { assert_eq!(Ok(123), u64::extended_parse("123")); assert_eq!(Ok(u64::MAX), u64::extended_parse(&format!("{}", u64::MAX))); + // TODO: We should wrap around here #7488 assert!(matches!( u64::extended_parse("-123"), - Err(ExtendedParserError::Overflow) + Err(ExtendedParserError::Overflow(0)) )); assert!(matches!( u64::extended_parse(""), @@ -421,16 +465,24 @@ mod tests { assert_eq!(Ok(i64::MIN), i64::extended_parse(&format!("{}", i64::MIN))); assert!(matches!( i64::extended_parse(&format!("{}", u64::MAX)), - Err(ExtendedParserError::Overflow) + Err(ExtendedParserError::Overflow(i64::MAX)) )); assert!(matches!( i64::extended_parse(&format!("{}", i64::MAX as u64 + 1)), - Err(ExtendedParserError::Overflow) + Err(ExtendedParserError::Overflow(i64::MAX)) )); assert!(matches!( i64::extended_parse("-123e10"), Err(ExtendedParserError::PartialMatch(-123, "e10")) )); + assert!(matches!( + i64::extended_parse(&format!("{}", -(u64::MAX as i128))), + Err(ExtendedParserError::Overflow(i64::MIN)) + )); + assert!(matches!( + i64::extended_parse(&format!("{}", i64::MIN as i128 - 1)), + Err(ExtendedParserError::Overflow(i64::MIN)) + )); } #[test] diff --git a/tests/by-util/test_printf.rs b/tests/by-util/test_printf.rs index ee2f399d5..60297e2e3 100644 --- a/tests/by-util/test_printf.rs +++ b/tests/by-util/test_printf.rs @@ -675,6 +675,19 @@ fn test_overflow() { new_ucmd!() .args(&["%d", "36893488147419103232"]) .fails_with_code(1) + .stdout_is("9223372036854775807") + .stderr_is("printf: '36893488147419103232': Numerical result out of range\n"); + + new_ucmd!() + .args(&["%d", "-36893488147419103232"]) + .fails_with_code(1) + .stdout_is("-9223372036854775808") + .stderr_is("printf: '-36893488147419103232': Numerical result out of range\n"); + + new_ucmd!() + .args(&["%u", "36893488147419103232"]) + .fails_with_code(1) + .stdout_is("18446744073709551615") .stderr_is("printf: '36893488147419103232': Numerical result out of range\n"); }