From d309a61f5c45eb454dc99a1fe37301d9c21d75ce Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:59:21 +0200 Subject: [PATCH] fix(encode): reject a non-positive playback rate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `remaining` never reaches 0.5 when it starts at 0.0 (0.0/0.5 == 0.0) or at any negative value (it diverges toward -inf). The loop never terminates and pushes a fresh `String` on every iteration, so rustmotion hangs while consuming memory until the OOM killer fires. Reached from a plain scenario JSON: `{"type":"video","src":"clip.mp4","playback_rate":0}` — `volume` defaults to 1.0 (`crates/rustmotion-components/src/video.rs:14` `fn default_volume() -> f32 { 1.0 }`), so `collect_videos_in_child` collects it (`v.volume > 0.0`, line 128), `collect_video_audio_tracks` calls `extract_audio_to_wav(..., occ.playback_rate)` (line 421), which calls `build_atempo_filter(0.0)` at line 333. `playback_rate` is an unvalidated `Option` (`crates/rustmotion-components/src/video.rs:26`); a repo-wide grep shows no validator constrains it. Both the ffmpeg render path (`encode_with_ffmpeg_hw_impl` line 432) and the native path (`mux.rs:34`) reach it. Refs #220 --- crates/rustmotion/src/encode/video_audio.rs | 27 +++++++++++++- crates/rustmotion/tests/audit_ws_c.rs | 39 +++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/crates/rustmotion/src/encode/video_audio.rs b/crates/rustmotion/src/encode/video_audio.rs index 5ce3c1fb..caac9e1e 100644 --- a/crates/rustmotion/src/encode/video_audio.rs +++ b/crates/rustmotion/src/encode/video_audio.rs @@ -199,19 +199,41 @@ fn ffmpeg_available() -> bool { /// rate 4.0 → `atempo=2.0,atempo=2.0` /// rate 0.1 → `atempo=0.5,atempo=0.2` (0.5 * 0.2 = 0.1) /// -/// Returns `None` if rate == 1.0 (no filter needed). +/// Returns `None` if rate == 1.0 (no filter needed), or if `rate` cannot +/// possibly be reached by any chain of `atempo` stages (`<= 0.0` or +/// non-finite — see the guard below). pub fn build_atempo_filter(rate: f64) -> Option { const EPSILON: f64 = 1e-9; + // `remaining` only ever converges toward `[0.5, 2.0]` by repeatedly + // multiplying or dividing by 2.0 starting from a *positive, finite* + // `rate`. At `rate == 0.0`, `remaining /= 0.5` stays `0.0` forever; at a + // negative or non-finite rate it diverges away from the loop's own exit + // test. Either way the `while` below never terminates and pushes a new + // `String` on every turn — the guard has to reject these before that + // loop is ever reached, not inside it. + if !rate.is_finite() || rate <= 0.0 { + return None; + } if (rate - 1.0).abs() < EPSILON { return None; } + // A ceiling on the chain length, independent of the guard above: a + // legitimate rate as extreme as 1e9 only needs ~30 stages, so this never + // fires for real input. It exists so that a future mistake in this + // arithmetic degrades into "no atempo filter" instead of reopening the + // same unbounded loop the guard above closes. + const MAX_STAGES: usize = 64; + let mut parts: Vec = Vec::new(); let mut remaining = rate; if rate > 1.0 { // Each stage multiplies by at most 2.0 while remaining > 2.0 + EPSILON { + if parts.len() >= MAX_STAGES { + return None; + } parts.push("atempo=2.0".to_string()); remaining /= 2.0; } @@ -219,6 +241,9 @@ pub fn build_atempo_filter(rate: f64) -> Option { } else { // Each stage multiplies by at least 0.5 while remaining < 0.5 - EPSILON { + if parts.len() >= MAX_STAGES { + return None; + } parts.push("atempo=0.5".to_string()); remaining /= 0.5; } diff --git a/crates/rustmotion/tests/audit_ws_c.rs b/crates/rustmotion/tests/audit_ws_c.rs index e612794e..0b18c5b3 100644 --- a/crates/rustmotion/tests/audit_ws_c.rs +++ b/crates/rustmotion/tests/audit_ws_c.rs @@ -115,3 +115,42 @@ fn a_render_ffmpeg_rejects_leaves_no_debris_at_the_output_path() { out.path().display() ); } + +// ─── build_atempo_filter(rate) must not loop forever ─────────────────────── +// +// `remaining /= 0.5` never reaches the loop's `>= 0.5` exit test starting +// from `0.0`, and diverges away from it starting from any negative rate — +// either way the pre-fix loop pushed a fresh `String` forever. Calling the +// unguarded function directly on the test thread would hang the whole +// suite, so each candidate rate runs on its own thread with a bounded +// `recv_timeout`: a present guard returns well inside the timeout, a +// missing one times out and fails the assertion instead of the process. +#[test] +fn atempo_guard_rejects_non_positive_and_non_finite_rates_without_hanging() { + use std::sync::mpsc; + use std::time::Duration; + + for rate in [ + 0.0_f64, + -1.0, + -0.25, + f64::NAN, + f64::INFINITY, + f64::NEG_INFINITY, + ] { + let (tx, rx) = mpsc::channel(); + std::thread::spawn(move || { + let _ = tx.send(rustmotion::encode::video_audio::build_atempo_filter(rate)); + }); + match rx.recv_timeout(Duration::from_secs(2)) { + Ok(result) => assert_eq!( + result, None, + "rate={rate} must return None instead of building an atempo chain" + ), + Err(_) => panic!( + "rate={rate} did not return within 2s — the guard against the infinite loop \ + in build_atempo_filter is missing or broken" + ), + } + } +}