From ce235c6f80388523341f51d437804504b6005c67 Mon Sep 17 00:00:00 2001 From: Royce Date: Fri, 23 Oct 2026 20:09:26 +0800 Subject: [PATCH 1/2] fix(model): saturate the arithmetic in trim_start_to / trim_end_to (#907) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Samples` is `i64`, and these two public APIs did plain subtraction: ```rust let new_start = new_start.min(self.end() - 1); // end() - 1 let delta = new_start - self.start; // extreme new_start vs extreme start self.length -= delta; // -= let new_end = new_end.max(self.start + 1); // start + 1 self.length = new_end - self.start; // extreme new_end vs extreme start ``` Command callers clamp to `MAX_POSITION` first, so nothing in the app trips this today — but the methods are `pub`, and a clip read from a hostile file can already hold any `i64` before `sanitize` runs. An extreme `new_start` against an extreme `start` wraps in release and panics in debug. Each site becomes a saturating operation followed by the same clamp `sanitize` already applies, so the result is identical for every input a command can produce. Test: `trims_saturate_on_extreme_arguments` sweeps `i64::MIN`/`MAX`, `±2 * MAX_POSITION` and the edges, in both directions, asserting positions stay within `±MAX_POSITION` and lengths within `1..=MAX_POSITION`. Verified against the unfixed arithmetic: the test fails with `attempt to subtract with overflow`. cargo test -p soundcraft-model: 41 passed, 0 failed. cargo clippy -p soundcraft-model --lib --no-deps --tests clean. --- crates/model/src/clip.rs | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/crates/model/src/clip.rs b/crates/model/src/clip.rs index 850cf044..11e80b0f 100644 --- a/crates/model/src/clip.rs +++ b/crates/model/src/clip.rs @@ -244,13 +244,16 @@ impl Clip { /// Move the start edge to `new_start` keeping material anchored (trim start). pub fn trim_start_to(&mut self, new_start: Samples) { - let new_start = new_start.min(self.end() - 1); - let delta = new_start - self.start; + // #907: these are public APIs, so they have to survive arguments a command would never + // pass (commands clamp to `MAX_POSITION` first). `Samples` is `i64`, so an extreme + // `new_start` against an extreme `start` wrapped in release and panicked in debug. + let new_start = new_start.clamp(-MAX_POSITION, MAX_POSITION).min(self.end().saturating_sub(1)); + let delta = new_start.saturating_sub(self.start); if let ClipContent::Audio { offset, .. } | ClipContent::Video { offset, .. } = &mut self.content { *offset = offset.saturating_add(delta); } // MIDI note ticks are re-anchored by the engine, which knows the tempo map. - self.length -= delta; + self.length = self.length.saturating_sub(delta).clamp(1, MAX_POSITION); self.start = new_start; if self.sync_point != 0 { self.sync_point = self.sync_point.saturating_sub(delta); @@ -267,8 +270,9 @@ impl Clip { /// Move the end edge to `new_end`. pub fn trim_end_to(&mut self, new_end: Samples) { - let new_end = new_end.max(self.start + 1); - self.length = new_end - self.start; + // #907: saturating, like `trim_start_to`. + let new_end = new_end.clamp(-MAX_POSITION, MAX_POSITION).max(self.start.saturating_add(1)); + self.length = new_end.saturating_sub(self.start).clamp(1, MAX_POSITION); self.drop_stray_sync(); self.clamp_fades(); } @@ -479,4 +483,24 @@ mod tests { c.trim_start_to(900); assert!(c.fade_out.len <= c.length); } + + /// #907: `trim_start_to` / `trim_end_to` are public APIs, so they have to survive arguments a + /// command would never pass. `Samples` is `i64`, so an extreme `new_start` against an extreme + /// `start` wrapped in release and panicked in debug. Every position stays within + /// `±MAX_POSITION` and every length within `1..=MAX_POSITION`. + #[test] + fn trims_saturate_on_extreme_arguments() { + for &new_start in &[Samples::MIN, -(MAX_POSITION * 2), -1, 0, MAX_POSITION, MAX_POSITION * 2, Samples::MAX] { + let mut c = Clip::audio(ClipId(1), "a", SourceId(1), 1000, MAX_POSITION / 2, 1000); + c.trim_start_to(new_start); + assert!((-(MAX_POSITION as i128)..=(MAX_POSITION as i128)).contains(&(c.start as i128)), "start {new_start}: {}", c.start); + assert!((1..=MAX_POSITION).contains(&c.length), "start {new_start}: length {}", c.length); + } + for &new_end in &[Samples::MIN, -(MAX_POSITION * 2), -1, 0, MAX_POSITION, MAX_POSITION * 2, Samples::MAX] { + let mut c = Clip::audio(ClipId(2), "b", SourceId(1), 1000, -(MAX_POSITION / 2), 1000); + c.trim_end_to(new_end); + assert!((1..=MAX_POSITION).contains(&c.length), "end {new_end}: length {}", c.length); + assert!(c.end() - c.start == c.length, "end {new_end}: start {} end {}", c.start, c.end()); + } + } } From b0f07331a2103fc081dfaeeabef7b140b9863020 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sat, 24 Oct 2026 00:53:41 -0400 Subject: [PATCH 2/2] Clip::trim_start_to keeps the end when an extreme start would overflow the length Co-Authored-By: Claude Opus 5.5 --- crates/model/src/clip.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/model/src/clip.rs b/crates/model/src/clip.rs index 11e80b0f..f43c10d8 100644 --- a/crates/model/src/clip.rs +++ b/crates/model/src/clip.rs @@ -247,7 +247,9 @@ impl Clip { // #907: these are public APIs, so they have to survive arguments a command would never // pass (commands clamp to `MAX_POSITION` first). `Samples` is `i64`, so an extreme // `new_start` against an extreme `start` wrapped in release and panicked in debug. - let new_start = new_start.clamp(-MAX_POSITION, MAX_POSITION).min(self.end().saturating_sub(1)); + // Never so far back that the length would pass `MAX_POSITION`: trimming the start keeps the end. + let end = self.end(); + let new_start = new_start.max(end.saturating_sub(MAX_POSITION)).clamp(-MAX_POSITION, MAX_POSITION).min(end.saturating_sub(1)); let delta = new_start.saturating_sub(self.start); if let ClipContent::Audio { offset, .. } | ClipContent::Video { offset, .. } = &mut self.content { *offset = offset.saturating_add(delta); @@ -495,6 +497,7 @@ mod tests { c.trim_start_to(new_start); assert!((-(MAX_POSITION as i128)..=(MAX_POSITION as i128)).contains(&(c.start as i128)), "start {new_start}: {}", c.start); assert!((1..=MAX_POSITION).contains(&c.length), "start {new_start}: length {}", c.length); + assert_eq!(c.end(), MAX_POSITION / 2 + 1000, "start {new_start} moved the end"); } for &new_end in &[Samples::MIN, -(MAX_POSITION * 2), -1, 0, MAX_POSITION, MAX_POSITION * 2, Samples::MAX] { let mut c = Clip::audio(ClipId(2), "b", SourceId(1), 1000, -(MAX_POSITION / 2), 1000);