From 13160b3ec3e76e6be565fd4987ffe1ea7263150f Mon Sep 17 00:00:00 2001 From: Devyn Cairns Date: Wed, 17 Apr 2024 05:25:16 -0700 Subject: [PATCH] Replace subtraction of Instants and Durations with saturating subtractions (#12549) # Description Duration can not be negative, and an underflow causes a panic. This should fix #12539 as from what I can tell that bug was caused in `nu-explore::pager::events` from subtracting durations, but I figured this might be more widespread, and saturating to zero generally makes sense. I also added the relevant clippy lint to try to prevent this from happening in the future. I can't think of a reason we would ever want to subtract durations without checking first. cc @fdncred # Tests + Formatting - :green_circle: `toolkit fmt` - :green_circle: `toolkit clippy` - :green_circle: `toolkit test` - :green_circle: `toolkit test stdlib` --- .github/workflows/ci.yml | 2 +- crates/nu-command/src/debug/timeit.rs | 5 ++++- crates/nu-explore/src/pager/events.rs | 2 +- crates/nu-system/src/linux.rs | 5 +++-- crates/nu-system/src/macos.rs | 4 ++-- crates/nu-system/src/windows.rs | 4 ++-- toolkit.nu | 2 ++ 7 files changed, 15 insertions(+), 9 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c51b1d923b..6c52886f51 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,7 +10,7 @@ env: NUSHELL_CARGO_PROFILE: ci NU_LOG_LEVEL: DEBUG # If changing these settings also change toolkit.nu - CLIPPY_OPTIONS: "-D warnings -D clippy::unwrap_used" + CLIPPY_OPTIONS: "-D warnings -D clippy::unwrap_used -D clippy::unchecked_duration_subtraction" concurrency: group: ${{ github.workflow }}-${{ github.head_ref && github.ref || github.run_id }} diff --git a/crates/nu-command/src/debug/timeit.rs b/crates/nu-command/src/debug/timeit.rs index 2747db02af..92f8fe18cd 100644 --- a/crates/nu-command/src/debug/timeit.rs +++ b/crates/nu-command/src/debug/timeit.rs @@ -63,7 +63,10 @@ impl Command for TimeIt { let end_time = Instant::now(); - let output = Value::duration((end_time - start_time).as_nanos() as i64, call.head); + let output = Value::duration( + end_time.saturating_duration_since(start_time).as_nanos() as i64, + call.head, + ); Ok(output.into_pipeline_data()) } diff --git a/crates/nu-explore/src/pager/events.rs b/crates/nu-explore/src/pager/events.rs index 7d155061d6..b408b10f1d 100644 --- a/crates/nu-explore/src/pager/events.rs +++ b/crates/nu-explore/src/pager/events.rs @@ -47,7 +47,7 @@ impl UIEvents { } let time_spent = now.elapsed(); - let rest = self.tick_rate - time_spent; + let rest = self.tick_rate.saturating_sub(time_spent); Self { tick_rate: rest }.next() } diff --git a/crates/nu-system/src/linux.rs b/crates/nu-system/src/linux.rs index 5f55f96b5c..238c386959 100644 --- a/crates/nu-system/src/linux.rs +++ b/crates/nu-system/src/linux.rs @@ -106,7 +106,7 @@ pub fn collect_proc(interval: Duration, _with_thread: bool) -> Vec let curr_stat = curr_proc.stat().ok(); let curr_status = curr_proc.status().ok(); let curr_time = Instant::now(); - let interval = curr_time - prev_time; + let interval = curr_time.saturating_duration_since(prev_time); let ppid = curr_proc.stat().map(|p| p.ppid).unwrap_or_default(); let curr_proc = ProcessTask::Process(curr_proc); @@ -203,7 +203,8 @@ impl ProcessInfo { let curr_time = cs.utime + cs.stime; let prev_time = ps.utime + ps.stime; - let usage_ms = (curr_time - prev_time) * 1000 / procfs::ticks_per_second(); + let usage_ms = + curr_time.saturating_sub(prev_time) * 1000 / procfs::ticks_per_second(); let interval_ms = self.interval.as_secs() * 1000 + u64::from(self.interval.subsec_millis()); usage_ms as f64 * 100.0 / interval_ms as f64 diff --git a/crates/nu-system/src/macos.rs b/crates/nu-system/src/macos.rs index bafea12da7..06683da6fd 100644 --- a/crates/nu-system/src/macos.rs +++ b/crates/nu-system/src/macos.rs @@ -93,7 +93,7 @@ pub fn collect_proc(interval: Duration, _with_thread: bool) -> Vec let curr_res = pidrusage::(pid).ok(); let curr_time = Instant::now(); - let interval = curr_time - prev_time; + let interval = curr_time.saturating_duration_since(prev_time); let ppid = curr_task.pbsd.pbi_ppid as i32; let proc = ProcessInfo { @@ -383,7 +383,7 @@ impl ProcessInfo { self.curr_task.ptinfo.pti_total_user + self.curr_task.ptinfo.pti_total_system; let prev_time = self.prev_task.ptinfo.pti_total_user + self.prev_task.ptinfo.pti_total_system; - let usage_ticks = curr_time - prev_time; + let usage_ticks = curr_time.saturating_sub(prev_time); let interval_us = self.interval.as_micros(); let ticktime_us = mach_ticktime() / 1000.0; usage_ticks as f64 * 100.0 * ticktime_us / interval_us as f64 diff --git a/crates/nu-system/src/windows.rs b/crates/nu-system/src/windows.rs index f7492e8811..d79ce5b0ea 100644 --- a/crates/nu-system/src/windows.rs +++ b/crates/nu-system/src/windows.rs @@ -198,7 +198,7 @@ pub fn collect_proc(interval: Duration, _with_thread: bool) -> Vec let priority = get_priority(handle); let curr_time = Instant::now(); - let interval = curr_time - prev_time; + let interval = curr_time.saturating_duration_since(prev_time); let mut all_ok = true; all_ok &= command.is_some(); @@ -1059,7 +1059,7 @@ impl ProcessInfo { let curr_time = self.cpu_info.curr_sys + self.cpu_info.curr_user; let prev_time = self.cpu_info.prev_sys + self.cpu_info.prev_user; - let usage_ms = (curr_time - prev_time) / 10000u64; + let usage_ms = curr_time.saturating_sub(prev_time) / 10000u64; let interval_ms = self.interval.as_secs() * 1000 + u64::from(self.interval.subsec_millis()); usage_ms as f64 * 100.0 / interval_ms as f64 } diff --git a/toolkit.nu b/toolkit.nu index a8007b4237..07b0363440 100644 --- a/toolkit.nu +++ b/toolkit.nu @@ -48,6 +48,7 @@ export def clippy [ -- -D warnings -D clippy::unwrap_used + -D clippy::unchecked_duration_subtraction ) if $verbose { @@ -73,6 +74,7 @@ export def clippy [ -- -D warnings -D clippy::unwrap_used + -D clippy::unchecked_duration_subtraction ) } catch {