From 388cb6c83af0b09b57dbd80301012b17447c1cc4 Mon Sep 17 00:00:00 2001
From: Jeffrey Finkelstein <jeffrey.finkelstein@protonmail.com>
Date: Thu, 17 Mar 2022 19:03:26 -0400
Subject: [PATCH] uucore: use Duration::saturating_mul in parse_time

Use `Duration::saturating_mul()` to avoid a panic due to overflow in
`uucore::parse_time::from_str()`. This change prevents panic on very
large arguments to timeout and sleep.
---
 src/uucore/src/lib/parser/parse_time.rs | 67 ++++++++++++++++++++++++-
 tests/by-util/test_sleep.rs             | 16 ++++++
 tests/by-util/test_timeout.rs           | 17 +++++++
 3 files changed, 99 insertions(+), 1 deletion(-)

diff --git a/src/uucore/src/lib/parser/parse_time.rs b/src/uucore/src/lib/parser/parse_time.rs
index 68f0ca8d0..366eebdea 100644
--- a/src/uucore/src/lib/parser/parse_time.rs
+++ b/src/uucore/src/lib/parser/parse_time.rs
@@ -6,11 +6,39 @@
 // file that was distributed with this source code.
 
 // spell-checker:ignore (vars) NANOS numstr
+//! Parsing a duration from a string.
+//!
+//! Use the [`from_str`] function to parse a [`Duration`] from a string.
 
 use std::time::Duration;
 
 use crate::display::Quotable;
 
+/// Parse a duration from a string.
+///
+/// The string may contain only a number, like "123" or "4.5", or it
+/// may contain a number with a unit specifier, like "123s" meaning
+/// one hundred twenty three seconds or "4.5d" meaning four and a half
+/// days. If no unit is specified, the unit is assumed to be seconds.
+///
+/// This function uses [`Duration::saturating_mul`] to compute the
+/// number of seconds, so it does not overflow. If overflow would have
+/// occurred, [`Duration::MAX`] is returned instead.
+///
+/// # Errors
+///
+/// This function returns an error if the input string is empty, the
+/// input is not a valid number, or the unit specifier is invalid or
+/// unknown.
+///
+/// # Examples
+///
+/// ```rust
+/// use std::time::Duration;
+/// use uucore::parse_time::from_str;
+/// assert_eq!(from_str("123"), Ok(Duration::from_secs(123)));
+/// assert_eq!(from_str("2d"), Ok(Duration::from_secs(60 * 60 * 24 * 2)));
+/// ```
 pub fn from_str(string: &str) -> Result<Duration, String> {
     let len = string.len();
     if len == 0 {
@@ -39,5 +67,42 @@ pub fn from_str(string: &str) -> Result<Duration, String> {
     let whole_secs = num.trunc();
     let nanos = (num.fract() * (NANOS_PER_SEC as f64)).trunc();
     let duration = Duration::new(whole_secs as u64, nanos as u32);
-    Ok(duration * times)
+    Ok(duration.saturating_mul(times))
+}
+
+#[cfg(test)]
+mod tests {
+
+    use crate::parse_time::from_str;
+    use std::time::Duration;
+
+    #[test]
+    fn test_no_units() {
+        assert_eq!(from_str("123"), Ok(Duration::from_secs(123)));
+    }
+
+    #[test]
+    fn test_units() {
+        assert_eq!(from_str("2d"), Ok(Duration::from_secs(60 * 60 * 24 * 2)));
+    }
+
+    #[test]
+    fn test_saturating_mul() {
+        assert_eq!(from_str("9223372036854775808d"), Ok(Duration::MAX));
+    }
+
+    #[test]
+    fn test_error_empty() {
+        assert!(from_str("").is_err());
+    }
+
+    #[test]
+    fn test_error_invalid_unit() {
+        assert!(from_str("123X").is_err());
+    }
+
+    #[test]
+    fn test_error_invalid_magnitude() {
+        assert!(from_str("12abc3s").is_err());
+    }
 }
diff --git a/tests/by-util/test_sleep.rs b/tests/by-util/test_sleep.rs
index 94a0a6896..91b0cd02b 100644
--- a/tests/by-util/test_sleep.rs
+++ b/tests/by-util/test_sleep.rs
@@ -1,3 +1,4 @@
+// spell-checker:ignore dont
 use crate::common::util::*;
 
 use std::time::{Duration, Instant};
@@ -115,3 +116,18 @@ fn test_sleep_sum_duration_many() {
 fn test_sleep_wrong_time() {
     new_ucmd!().args(&["0.1s", "abc"]).fails();
 }
+
+// TODO These tests would obviously block for a very long time. We
+// only want to verify that there is no error here, so we could just
+// figure out a way to terminate the child process after a short
+// period of time.
+
+// #[test]
+#[allow(dead_code)]
+fn test_dont_overflow() {
+    new_ucmd!()
+        .arg("9223372036854775808d")
+        .succeeds()
+        .no_stderr()
+        .no_stdout();
+}
diff --git a/tests/by-util/test_timeout.rs b/tests/by-util/test_timeout.rs
index b9c8a59ed..96a5b6a05 100644
--- a/tests/by-util/test_timeout.rs
+++ b/tests/by-util/test_timeout.rs
@@ -1,3 +1,4 @@
+// spell-checker:ignore dont
 use crate::common::util::*;
 
 // FIXME: this depends on the system having true and false in PATH
@@ -64,3 +65,19 @@ fn test_preserve_status() {
         .no_stderr()
         .no_stdout();
 }
+
+#[test]
+fn test_dont_overflow() {
+    new_ucmd!()
+        .args(&["9223372036854775808d", "sleep", "0"])
+        .succeeds()
+        .code_is(0)
+        .no_stderr()
+        .no_stdout();
+    new_ucmd!()
+        .args(&["-k", "9223372036854775808d", "10", "sleep", "0"])
+        .succeeds()
+        .code_is(0)
+        .no_stderr()
+        .no_stdout();
+}