diff --git a/src/uu/split/src/filenames.rs b/src/uu/split/src/filenames.rs index c8a575432a..da0c612508 100644 --- a/src/uu/split/src/filenames.rs +++ b/src/uu/split/src/filenames.rs @@ -46,6 +46,12 @@ use uucore::display::Quotable; use uucore::error::{UResult, USimpleError}; use uucore::translate; +// This is an allocation guard, not a semantic suffix limit. 4096 is around the +// common full-path limit and is already far above typical filename-component +// limits, so realistic suffix lengths stay accepted while pathological values +// cannot request attacker-sized filename buffers. +const MAX_SUFFIX_LENGTH: usize = 4096; + /// The format to use for suffixes in the filename for each output chunk. #[derive(Clone, Copy)] pub enum SuffixType { @@ -175,11 +181,13 @@ impl Suffix { let (mut length, is_length_cmd_opt) = if let Some(v) = matches.get_one::(OPT_SUFFIX_LENGTH) { // suffix length was specified in command line - ( - v.parse::() - .map_err(|_| SuffixError::NotParsable(v.to_owned()))?, - true, - ) + let parsed_length = v + .parse::() + .map_err(|_| SuffixError::NotParsable(v.to_owned()))?; + if parsed_length > MAX_SUFFIX_LENGTH { + return Err(SuffixError::NotParsable(v.to_owned())); + } + (parsed_length, true) } else { // no suffix length option was specified in command line // set to default value diff --git a/src/uu/split/src/number.rs b/src/uu/split/src/number.rs index 58c08d12ba..818a5b8889 100644 --- a/src/uu/split/src/number.rs +++ b/src/uu/split/src/number.rs @@ -103,7 +103,7 @@ impl Number { #[allow(dead_code)] fn digits(&self) -> Vec { match self { - Self::FixedWidth(number) => number.digits.clone(), + Self::FixedWidth(number) => number.digits(), Self::DynamicWidth(number) => number.digits(), } } @@ -188,69 +188,94 @@ impl Display for Number { #[derive(Clone)] pub struct FixedWidthNumber { radix: u8, - digits: Vec, + width: usize, + current: usize, } impl FixedWidthNumber { /// Instantiate a number of the given radix and width. - pub fn new(radix: u8, width: usize, mut suffix_start: usize) -> Result { - let mut digits = vec![0_u8; width]; - - for i in (0..digits.len()).rev() { - let remainder = (suffix_start % (radix as usize)) as u8; - suffix_start /= radix as usize; - digits[i] = remainder; - if suffix_start == 0 { - break; - } - } - if suffix_start == 0 { - Ok(Self { radix, digits }) + pub fn new(radix: u8, width: usize, suffix_start: usize) -> Result { + if fits_in_width(radix, width, suffix_start) { + Ok(Self { + radix, + width, + current: suffix_start, + }) } else { Err(Overflow) } } + fn digits(&self) -> Vec { + fixed_width_digits(self.radix, self.width, self.current) + } + /// Increment this number. /// /// This method adds one to this number. If incrementing this /// number would require more digits than are available with the /// specified width, then this method returns `Err(Overflow)`. fn increment(&mut self) -> Result<(), Overflow> { - for i in (0..self.digits.len()).rev() { - // Increment the current digit. - self.digits[i] += 1; - - // If the digit overflows, then set it to 0 and continue - // to the next iteration to increment the next most - // significant digit. Otherwise, terminate the loop, since - // there will be no further changes to any higher order - // digits. - if self.digits[i] == self.radix { - self.digits[i] = 0; - } else { - break; - } - } - - // Return an error on overflow, which is signified by all zeros. - if self.digits == vec![0; self.digits.len()] { - Err(Overflow) - } else { + let next = self.current.checked_add(1).ok_or(Overflow)?; + if fits_in_width(self.radix, self.width, next) { + self.current = next; Ok(()) + } else { + Err(Overflow) } } } impl Display for FixedWidthNumber { fn fmt(&self, f: &mut Formatter) -> fmt::Result { - for d in &self.digits { - f.write_char(map_digit(self.radix, *d))?; + let radix = self.radix as usize; + let mut remaining = self.current; + let mut significant_digits = Vec::new(); + + while remaining > 0 { + significant_digits.push((remaining % radix) as u8); + remaining /= radix; + } + + for _ in significant_digits.len()..self.width { + f.write_char(map_digit(self.radix, 0))?; + } + + for d in significant_digits.into_iter().rev() { + f.write_char(map_digit(self.radix, d))?; } Ok(()) } } +fn fits_in_width(radix: u8, width: usize, value: usize) -> bool { + let radix = radix as usize; + let mut remaining = value; + + for _ in 0..width { + remaining /= radix; + if remaining == 0 { + return true; + } + } + + remaining == 0 +} + +fn fixed_width_digits(radix: u8, width: usize, value: usize) -> Vec { + let radix = radix as usize; + let mut remaining = value; + let mut digits = Vec::with_capacity(width); + + for _ in 0..width { + digits.push((remaining % radix) as u8); + remaining /= radix; + } + + digits.reverse(); + digits +} + /// A positional notation representation of a number of dynamically growing width. /// /// The digits are represented as a [`Vec`] with the most @@ -571,6 +596,11 @@ mod tests { assert_eq!(format!("{}", num(0xf).unwrap()), "23"); } + #[test] + fn test_fixed_width_number_large_width_does_not_allocate() { + assert!(FixedWidthNumber::new(26, usize::MAX, 0).is_ok()); + } + #[test] fn test_dynamic_width_number_start_suffix() { fn num(n: usize) -> Result { diff --git a/tests/by-util/test_split.rs b/tests/by-util/test_split.rs index a5d6740239..70454eb645 100644 --- a/tests/by-util/test_split.rs +++ b/tests/by-util/test_split.rs @@ -914,6 +914,14 @@ fn test_suffix_length_req() { .stderr_only("split: the suffix length needs to be at least 2\n"); } +#[test] +fn test_large_suffix_length_is_rejected() { + new_ucmd!() + .args(&["-a", "66542562175252"]) + .fails_with_code(1) + .stderr_only("split: invalid suffix length: '66542562175252'\n"); +} + #[test] fn test_verbose() { new_ucmd!()