From 07d85b0f0245fcf22d2c8abea4159478cf780366 Mon Sep 17 00:00:00 2001 From: cirocueto2013-wq Date: Wed, 7 Oct 2026 20:50:33 -0300 Subject: [PATCH] fix: validate hostile project, export and MXF inputs Validate staged speed edits before adopting their timeline state. Co-authored-by: Codex --- crates/edit/src/lib.rs | 1 + crates/engine/src/commands.rs | 48 +++++++------ crates/engine/src/export_tests.rs | 27 ++++++++ crates/engine/src/export_tools.rs | 34 +++++----- crates/engine/src/lib.rs | 20 +++++- crates/engine/src/tests.rs | 60 +++++++++++++++++ crates/export/src/audio_out.rs | 11 ++- crates/export/src/lib.rs | 45 ++++++++++++- crates/export/src/pipeline.rs | 12 ++++ crates/export/src/settings.rs | 2 +- crates/export/src/settings_tests.rs | 52 +++++++++++++++ crates/format/src/lib.rs | 24 ++++++- crates/h264enc/src/lib.rs | 6 +- crates/mxf/src/essence.rs | 11 ++- crates/mxf/src/file.rs | 100 ++++++++++++++++++++-------- crates/mxf/src/tests.rs | 26 ++++++++ crates/project/src/caption.rs | 22 ++++++ crates/project/src/lib.rs | 84 +++++++++++++++++++++++ 18 files changed, 512 insertions(+), 73 deletions(-) diff --git a/crates/edit/src/lib.rs b/crates/edit/src/lib.rs index 2699c259..67937f85 100644 --- a/crates/edit/src/lib.rs +++ b/crates/edit/src/lib.rs @@ -920,6 +920,7 @@ pub fn set_speed_group(seq: &mut Sequence, clips: &[ClipId], speed: f64, reverse if ripple { transitions_follow_cuts(seq, &mut work); } + work.check().map_err(EditError::Other)?; *seq = work; Ok(()) } diff --git a/crates/engine/src/commands.rs b/crates/engine/src/commands.rs index 50ecc070..1d5f9b3d 100644 --- a/crates/engine/src/commands.rs +++ b/crates/engine/src/commands.rs @@ -164,6 +164,12 @@ pub(crate) fn u64_p(p: &Value, k: &str) -> Option { p.get(k).and_then(|v| v.as_u64().or_else(|| v.as_f64().map(|f| f as u64))) } +/// Typed dimensions/counts must not truncate, wrap or silently accept negative/fractional values. +pub(crate) fn checked_u32_p(p: &Value, key: &str, cmd: &str) -> Result> { + let Some(value) = p.get(key).filter(|v| !v.is_null()) else { return Ok(None) }; + value.as_u64().and_then(|v| u32::try_from(v).ok()).map(Some).ok_or_else(|| bad(cmd, format!("`{key}` must be an unsigned 32-bit integer"))) +} + /// Parse a time from params: `time` (ticks), `frame`, `seconds` or `timecode`, with `prefix`. pub(crate) fn time_p(s: &Session, p: &Value, prefix: &str) -> Option { let rate = s.sequence_rate(); @@ -661,17 +667,17 @@ fn build() -> Vec { if let Some(from) = item_p(p, "fromItem").and_then(|i| s.project.item(i)).and_then(|i| i.as_media()) { st = default_seq_settings_for(&from.info); } - if let Some(w) = u64_p(p, "width") { - st.width = w as u32; + if let Some(w) = checked_u32_p(p, "width", "file.newSequence")? { + st.width = w; } - if let Some(h) = u64_p(p, "height") { - st.height = h as u32; + if let Some(h) = checked_u32_p(p, "height", "file.newSequence")? { + st.height = h; } if let Some(fps) = fps_p(p, "file.newSequence")? { st.frame_rate = fps; } - if let Some(sr) = u64_p(p, "sampleRate") { - st.sample_rate = sr as u32; + if let Some(sr) = checked_u32_p(p, "sampleRate", "file.newSequence")? { + st.sample_rate = sr; } if let Some(m) = str_p(p, "mix") { st.audio_master = @@ -685,13 +691,17 @@ fn build() -> Vec { None => None, }; st.preset = format!("{}x{} {}", st.width, st.height, st.frame_rate.label()); - let nv = u64_p(p, "video").unwrap_or(3) as usize; - let na = u64_p(p, "audio").unwrap_or(3) as usize; + st.validate().map_err(|e| bad("file.newSequence", e))?; + let nv = checked_u32_p(p, "video", "file.newSequence")?.unwrap_or(3); + let na = checked_u32_p(p, "audio", "file.newSequence")?.unwrap_or(3); + if nv > 256 || na > 256 { + return Err(bad("file.newSequence", "at most 256 video and 256 audio tracks are supported")); + } let n = s.project.sequences().count() + 1; let name = str_p(p, "name").map(str::to_string).unwrap_or_else(|| format!("Sequence {n:02}")); let seq_label = s.prefs.labels.defaults.sequence; let id = s.edit("New Sequence", |pr, st2| { - let id = pr.new_sequence(&name, st, nv, na, None); + let id = pr.new_sequence(&name, st, nv as usize, na as usize, None); if let Some(it) = pr.item_mut(id) { it.label = seq_label; } @@ -1515,22 +1525,22 @@ fn build() -> Vec { pr.item_mut(id).ok_or(EngineError::NoSequence)?.name = n.to_string(); } let q = pr.sequence_mut(id).ok_or(EngineError::NoSequence)?; - if let Some(w) = u64_p(&p, "width") { - q.settings.width = w as u32; + if let Some(w) = checked_u32_p(&p, "width", "sequence.settings")? { + q.settings.width = w; } - if let Some(h) = u64_p(&p, "height") { - q.settings.height = h as u32; + if let Some(h) = checked_u32_p(&p, "height", "sequence.settings")? { + q.settings.height = h; } if let Some(f) = fps { q.settings.frame_rate = f; } - if let Some(sr) = u64_p(&p, "sampleRate") { - q.settings.sample_rate = sr as u32; + if let Some(sr) = checked_u32_p(&p, "sampleRate", "sequence.settings")? { + q.settings.sample_rate = sr; } if let Some(m) = mix { q.settings.audio_master = m; } - Ok(()) + q.settings.validate().map_err(|e| bad("sequence.settings", e)) })?; Ok(Value::Null) } @@ -2221,7 +2231,7 @@ fn build() -> Vec { let pq = p.clone(); s.edit_sequence("Change Effect Parameter", |q, _, _| { let (_, it) = q.find_item_mut(c).ok_or(filmcraft_edit::EditError::NoItem(c))?; - let mt = it.source_time_at(tl.clamp(it.start, it.end() - Tick(1))); + let mt = it.source_time_at(tl.clamp(it.start, (it.end() - Tick(1)).max(it.start))); let e = match &eff { Value::Number(n) => it.effects.get_mut(n.as_u64().unwrap_or(0) as usize), Value::String(sid) => it.effects.iter_mut().find(|e| &e.effect == sid), @@ -2252,7 +2262,7 @@ fn build() -> Vec { let ph = s.playhead(); s.edit_sequence("Toggle Animation", |q, _, _| { let (_, it) = q.find_item_mut(c).ok_or(filmcraft_edit::EditError::NoItem(c))?; - let mt = it.source_time_at(ph.clamp(it.start, it.end() - Tick(1))); + let mt = it.source_time_at(ph.clamp(it.start, (it.end() - Tick(1)).max(it.start))); let e = match &eff { Value::Number(n) => it.effects.get_mut(n.as_u64().unwrap_or(0) as usize), Value::String(sid) => it.effects.iter_mut().find(|e| &e.effect == sid), @@ -2519,7 +2529,7 @@ fn keyframe_op(s: &mut Session, p: &Value, op: &str) -> Result { }; s.edit_sequence(label, |q, _, _| { let (_, it) = q.find_item_mut(c).ok_or(filmcraft_edit::EditError::NoItem(c))?; - let mt_now = it.source_time_at(ph.clamp(it.start, it.end() - Tick(1))); + let mt_now = it.source_time_at(ph.clamp(it.start, (it.end() - Tick(1)).max(it.start))); let e = match &eff { Value::Number(n) => it.effects.get_mut(n.as_u64().unwrap_or(0) as usize), Value::String(sid) => it.effects.iter_mut().find(|e| &e.effect == sid), diff --git a/crates/engine/src/export_tests.rs b/crates/engine/src/export_tests.rs index 08bf90fc..64c83216 100644 --- a/crates/engine/src/export_tests.rs +++ b/crates/engine/src/export_tests.rs @@ -9,6 +9,20 @@ use serde_json::{Value, json}; use crate::Session; +#[test] +fn hostile_export_ranges_are_rejected_without_panicking() { + let mut session = demo(); + for params in [ + json!({"range":"custom", "startTime":i64::MIN, "endTime":i64::MAX}), + json!({"range":"custom", "startTime":-1, "endTime":100}), + json!({"range":"custom", "startTime":100, "endTime":100}), + ] { + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| session.execute("export.resolve", params))); + assert!(result.is_ok(), "a hostile export range must not panic"); + assert!(result.unwrap().is_err()); + } +} + /// A scratch directory under the workspace `target/`, removed when dropped. struct Scratch(PathBuf); @@ -36,6 +50,19 @@ fn demo() -> Session { s } +#[test] +fn export_integer_parameters_cannot_wrap_or_overflow() { + let mut s = demo(); + for name in ["bitrateKbps", "maxBitrateKbps", "keyframeDistance"] { + for value in [json!(-1), json!(1.5), json!(u64::from(u32::MAX) + 1)] { + let mut params = json!({}); + params[name] = value; + assert!(s.execute("export.resolve", params).is_err(), "{name}"); + } + } + assert!(s.execute("export.resolve", json!({"bitrateKbps":u32::MAX})).is_ok()); +} + fn probe(path: &str) -> Option { let ffprobe = filmcraft_testkit::ffprobe_or_skip("export presets")?; let out = std::process::Command::new(ffprobe).args(["-v", "error", "-of", "json", "-show_format", "-show_streams", path]).output().unwrap(); diff --git a/crates/engine/src/export_tools.rs b/crates/engine/src/export_tools.rs index 26d03d54..4d81fdab 100644 --- a/crates/engine/src/export_tools.rs +++ b/crates/engine/src/export_tools.rs @@ -187,18 +187,18 @@ pub fn settings_from_params(s: &Session, p: &Value, cmd: &str) -> Result<(Option if let Some(v) = u64_p(p, "quality") { settings.quality = v.min(100) as u8; } - if let Some(v) = u64_p(p, "bitrateKbps") { - settings.bitrate_kbps = v as u32; + if let Some(v) = crate::commands::checked_u32_p(p, "bitrateKbps", cmd)? { + settings.bitrate_kbps = v; settings.adaptive_bitrate = None; } - if let Some(v) = u64_p(p, "maxBitrateKbps") { - settings.max_bitrate_kbps = Some(v as u32); + if let Some(v) = crate::commands::checked_u32_p(p, "maxBitrateKbps", cmd)? { + settings.max_bitrate_kbps = Some(v); } if let Some(v) = str_p(p, "bitrateMode") { settings.bitrate_mode = serde_json::from_value(json!(v)).map_err(|_| bad(cmd, "bitrateMode: cbr | vbr1Pass | vbr2Pass"))?; } - if let Some(v) = u64_p(p, "keyframeDistance") { - settings.keyframe_distance = Some(v as u32); + if let Some(v) = crate::commands::checked_u32_p(p, "keyframeDistance", cmd)? { + settings.keyframe_distance = Some(v); } if let Some(v) = p.get("hardwareEncoding") { settings.hardware_encoding = match v { @@ -230,8 +230,10 @@ pub fn settings_from_params(s: &Session, p: &Value, cmd: &str) -> Result<(Option if let Some(v) = bool_p(p, "sdr") { settings.sdr = v; } - if let (Some(w), Some(h)) = (u64_p(p, "width"), u64_p(p, "height")) { - settings.frame_size = Some((w as u32, h as u32)); + match (crate::commands::checked_u32_p(p, "width", cmd)?, crate::commands::checked_u32_p(p, "height", cmd)?) { + (Some(w), Some(h)) => settings.frame_size = Some((w, h)), + (None, None) => {} + _ => return Err(bad(cmd, "provide both width and height")), } if let Some(v) = p.get("fps") { settings.frame_rate = Some(parse_rate(v).ok_or_else(|| bad(cmd, "fps: a number (29.97) or \"num/den\""))?); @@ -263,6 +265,7 @@ pub fn sequence_param(s: &Session, p: &Value, cmd: &str) -> Result { /// without a `range` mean `custom`. pub fn range_param(s: &Session, project: &Project, seq: ItemId, v: Option<&Value>, p: &Value, cmd: &str) -> Result> { let q = project.sequence(seq).ok_or(EngineError::NoSequence)?; + q.check().map_err(|e| bad(cmd, e))?; let fd = q.settings.frame_rate.frame_duration(); let whole = TimeRange::from_bounds(Tick::ZERO, q.duration().max(fd)); let mode = match v { @@ -272,13 +275,9 @@ pub fn range_param(s: &Session, project: &Project, seq: ItemId, v: Option<&Value Some(o @ Value::Object(_)) => return range_param(s, project, seq, o.get("mode").or(Some(&json!("custom"))), o, cmd), Some(other) => return Err(bad(cmd, format!("range: unexpected {other}"))), }; - Ok(Some(match mode.as_str() { + let range = match mode.as_str() { "entire" | "entiresequence" | "sequence" | "all" => whole, - "inout" | "sequenceinout" => { - let a = q.mark_in.unwrap_or(Tick::ZERO); - let b = q.mark_out.map(|o| o + fd).unwrap_or(q.duration()); - TimeRange::from_bounds(a, b.max(a + fd)) - } + "inout" | "sequenceinout" => filmcraft_export::export_range(project, seq, &ExportSettings::default()).map_err(|e| bad(cmd, e.to_string()))?, "workarea" => q.work_area.ok_or_else(|| bad(cmd, "the sequence has no work area"))?, "custom" => { let a = time_p(s, p, "start").ok_or_else(|| bad(cmd, "custom range: need start (startTime / startSeconds / startFrame / startTimecode)"))?; @@ -286,10 +285,13 @@ pub fn range_param(s: &Session, project: &Project, seq: ItemId, v: Option<&Value if b <= a { return Err(bad(cmd, "custom range: end must be after start")); } - TimeRange::from_bounds(a, b) + let duration = b.0.checked_sub(a.0).ok_or_else(|| bad(cmd, "custom range: duration overflows"))?; + TimeRange::new(a, Tick(duration)) } other => return Err(bad(cmd, format!("range `{other}`: entire | inOut | workArea | custom"))), - })) + }; + filmcraft_export::validate_range(range).map_err(|e| bad(cmd, e.to_string()))?; + Ok(Some(range)) } /// Where exports go when no path is given: next to the saved project, else ~/Movies, else the diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index b0780319..71363c11 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -923,7 +923,7 @@ impl Session { /// Render the active sequence at the playhead in its working colour space (HDR values kept; /// for scopes and analysis). pub fn render_program_working(&self, scale: f32) -> Option { - let seq = self.state.active_sequence?; + let seq = self.renderable_sequence(scale)?; let provider = self.media.provider(self.project.clone(), self.services.clone()); let opts = filmcraft_render::RenderOptions { scale, working_output: true, ..Default::default() }; Some(filmcraft_render::render_sequence(&self.project, seq, self.playhead(), opts, &provider)) @@ -937,12 +937,28 @@ impl Session { /// Render the active sequence at `t` (snapped to its frame, as the playhead would be) without /// moving the playhead (CPU reference path). pub fn render_program_at(&self, scale: f32, t: Tick) -> Option { - let seq = self.state.active_sequence?; + let seq = self.renderable_sequence(scale)?; let t = self.sequence_rate().snap(t.max(Tick::ZERO)); let provider = self.media.provider(self.project.clone(), self.services.clone()); let opts = filmcraft_render::RenderOptions { scale, captions: true, ..Default::default() }; Some(filmcraft_render::render_sequence(&self.project, seq, t, opts, &provider)) } + + fn renderable_sequence(&self, scale: f32) -> Option { + let id = self.state.active_sequence?; + let seq = self.project.sequence(id)?; + if !scale.is_finite() || scale <= 0.0 { + eprintln!("filmcraft: render scale must be finite and positive"); + return None; + } + let (w, h) = filmcraft_render::output_size(seq, scale); + let valid = u32::try_from(w).ok().zip(u32::try_from(h).ok()).is_some_and(|(w, h)| filmcraft_project::validate_frame_size(w, h).is_ok()); + if !valid { + eprintln!("filmcraft: requested render frame exceeds the image size limits"); + return None; + } + Some(id) + } } /// The earliest media time clips of an item may show: a subclip that restricts trims starts at its diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 8446d938..e816d859 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -1,4 +1,15 @@ use super::*; + +#[test] +fn invalid_preview_scales_are_rejected_without_allocating() { + let mut s = Session::default(); + s.execute("file.newSequence", serde_json::json!({"width":160,"height":90})).unwrap(); + for scale in [0.0, -1.0, f32::NAN, f32::INFINITY, f32::MAX] { + assert!(s.render_program(scale).is_none()); + assert!(s.render_program_working(scale).is_none()); + } + assert_eq!(s.render_program(0.5).unwrap().w, 80); +} use serde_json::json; fn demo() -> Session { @@ -95,6 +106,38 @@ fn slide_moves_linked_audio_with_the_video() { assert_eq!((a.start, a.source_in), (nv.start, nv.source_in), "linked audio slides too and stays in sync"); } +#[test] +fn sequence_parameters_are_bounded_and_failed_changes_are_atomic() { + let mut s = demo(); + let before = (*s.project).clone(); + let history = s.history.undo.len(); + for command in ["file.newSequence", "sequence.settings"] { + for params in [ + json!({"width":0}), + json!({"height":0}), + json!({"sampleRate":0}), + json!({"width":4_294_967_360_u64}), + json!({"height":u64::MAX}), + json!({"sampleRate":u64::MAX}), + json!({"width":-1}), + json!({"width":1.5}), + json!({"width":16384,"height":16384}), + json!({"fps":1001}), + json!({"sampleRate":384001}), + ] { + assert!(s.execute(command, params.clone()).is_err(), "{command} {params}"); + assert_eq!(*s.project, before); + assert_eq!(s.history.undo.len(), history); + } + } + for params in [json!({"video":u64::MAX}), json!({"audio":u64::MAX}), json!({"video":257}), json!({"audio":-1})] { + assert!(s.execute("file.newSequence", params.clone()).is_err(), "{params}"); + assert_eq!(*s.project, before); + } + s.execute("file.newSequence", json!({"width":7680,"height":4320,"video":0,"audio":0})).unwrap(); + assert_eq!(s.active_sequence().unwrap().settings.width, 7680, "standard 8K remains supported"); +} + #[test] fn zero_fps_is_refused() { let mut s = demo(); @@ -435,3 +478,20 @@ fn ripple_trim_to_playhead_follows_the_selection() { let linked = s.active_sequence().unwrap().audio_tracks[0].items.iter().find(|i| i.start == start).map(|i| i.end()); assert_eq!(linked, Some(ph), "its linked sound on A1 is trimmed with it"); } + +#[test] +fn empty_items_do_not_panic_keyframe_commands() { + let mut s = demo(); + let seq = s.state.active_sequence.unwrap(); + let clip = { + let item = &mut std::sync::Arc::make_mut(&mut s.project).sequence_mut(seq).unwrap().video_tracks[0].items[0]; + item.duration = Tick::ZERO; + item.id + }; + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + s.execute("effects.toggleAnimation", json!({"clip":clip.0,"effect":"motion","param":"position"})) + })); + assert!(result.is_ok(), "an empty clip must not supply reversed clamp bounds"); + let error = result.unwrap().unwrap_err(); + assert!(error.to_string().contains("non-positive duration"), "expected the ordinary invariant error, got: {error}"); +} diff --git a/crates/export/src/audio_out.rs b/crates/export/src/audio_out.rs index a2889710..b92b6054 100644 --- a/crates/export/src/audio_out.rs +++ b/crates/export/src/audio_out.rs @@ -157,8 +157,17 @@ pub(crate) fn interleave(planar: &[Vec]) -> Vec { let mut out = Vec::with_capacity(n * planar.len()); for i in 0..n { for c in planar { - out.push(c[i]); + out.push(c.get(i).copied().unwrap_or(0.0)); } } out } + +#[cfg(test)] +mod bounds_tests { + #[test] + fn ragged_audio_planes_keep_channel_positions_without_panicking() { + assert_eq!(super::interleave(&[vec![1.0, 2.0], vec![3.0]]), vec![1.0, 3.0, 2.0, 0.0]); + assert!(super::interleave(&[]).is_empty()); + } +} diff --git a/crates/export/src/lib.rs b/crates/export/src/lib.rs index bcec623e..6b8fddf9 100644 --- a/crates/export/src/lib.rs +++ b/crates/export/src/lib.rs @@ -474,6 +474,28 @@ impl Default for ExportSettings { impl ExportSettings { /// Reject settings the encoders cannot honour. pub fn validate(&self) -> Result<()> { + if let Some(range) = self.range { + validate_range(range)?; + } + if !self.scale.is_finite() || self.scale <= 0.0 { + return Err(ExportError::Unsupported("output scale must be finite and positive".into())); + } + if let Some((width, height)) = self.frame_size { + filmcraft_project::validate_frame_size(width, height).map_err(ExportError::Unsupported)?; + } + if let Some(rate) = self.frame_rate + && (rate.num <= 0 + || rate.den <= 0 + || rate.as_f64() > 1000.0 + || rate.num > i64::from(u32::MAX) + || rate.den > i64::from(u32::MAX) + || (i128::from(filmcraft_time::TICKS_PER_SECOND) * i128::from(rate.den) / i128::from(rate.num.max(1))) > i128::from(Tick::MAX.0)) + { + return Err(ExportError::Unsupported("output frame rate must be positive and at most 1000 fps".into())); + } + if self.audio.sample_rate.is_some_and(|rate| rate == 0 || rate > 384_000) { + return Err(ExportError::Unsupported("output sample rate must be between 1 and 384000 Hz".into())); + } if self.field_order != FieldOrder::Progressive && self.has_video() { return Err(ExportError::Unsupported(format!("{} field order: FilmCraft's encoders write progressive frames", self.field_order.label()))); } @@ -1156,7 +1178,7 @@ fn h264_factory(format: Format, w: u32, h: u32, rate: FrameRate, s: &ExportSetti cfg.keyint = s.keyframe_distance.filter(|k| *k > 0).unwrap_or_else(|| (rate.num as f64 / rate.den as f64 * 2.0).round().max(1.0) as u32); cfg.slices = h264_slices(h); let kbps = s.bitrate_kbps.max(100); - let max = s.max_bitrate_kbps.filter(|m| *m >= kbps).unwrap_or(kbps * 3 / 2); + let max = s.max_bitrate_kbps.filter(|m| *m >= kbps).unwrap_or_else(|| (u64::from(kbps) * 3 / 2).min(u64::from(u32::MAX)) as u32); cfg.rate = match s.bitrate_mode { BitrateMode::Cbr => filmcraft_h264enc::RateControl::Cbr { kbps }, _ => filmcraft_h264enc::RateControl::Vbr { target_kbps: kbps, max_kbps: max }, @@ -1195,13 +1217,30 @@ fn h264_factory(format: Format, w: u32, h: u32, rate: FrameRate, s: &ExportSetti /// The range to export (settings → In/Out → whole sequence). pub fn export_range(project: &Project, seq: ItemId, settings: &ExportSettings) -> Result { let q = project.sequence(seq).ok_or(ExportError::NoSequence)?; + q.check().map_err(ExportError::Unsupported)?; if let Some(r) = settings.range { + validate_range(r)?; return Ok(r); } let fd = q.settings.frame_rate.frame_duration(); let a = q.mark_in.unwrap_or(Tick::ZERO); - let b = q.mark_out.map(|o| o + fd).unwrap_or(q.duration()); - Ok(TimeRange::from_bounds(a, b.max(a + fd))) + let minimum_end = a.0.checked_add(fd.0).ok_or_else(|| ExportError::Unsupported("export In point overflows the time range".into()))?; + let b = match q.mark_out { + Some(out) => out.0.checked_add(fd.0).ok_or_else(|| ExportError::Unsupported("export Out point overflows the time range".into()))?, + None => q.duration().0, + }; + let duration = b.max(minimum_end).checked_sub(a.0).ok_or_else(|| ExportError::Unsupported("export range duration overflows".into()))?; + let range = TimeRange::new(a, Tick(duration)); + validate_range(range)?; + Ok(range) +} + +/// Validate input time before frame/sample conversion or any `TimeRange::end` arithmetic. +pub fn validate_range(range: TimeRange) -> Result<()> { + if range.start.0 < 0 || range.duration.0 <= 0 || range.start.0.checked_add(range.duration.0).is_none_or(|end| end > Tick::MAX.0) { + return Err(ExportError::Unsupported("export range must start at or after zero, have positive duration and end within supported time bounds".into())); + } + Ok(()) } /// Output frames `[f0, f1)` of `range` at `rate` (frame `f` is at `rate.tick_of(f)`). diff --git a/crates/export/src/pipeline.rs b/crates/export/src/pipeline.rs index 1132dfe5..fde70f83 100644 --- a/crates/export/src/pipeline.rs +++ b/crates/export/src/pipeline.rs @@ -71,8 +71,20 @@ pub(crate) struct Pipeline { impl Pipeline { pub fn new(project: Arc, seq: ItemId, settings: &ExportSettings, hdr_out: bool) -> Result { let q = project.sequence(seq).ok_or(ExportError::NoSequence)?; + q.settings.validate().map_err(ExportError::Unsupported)?; + settings.validate()?; let r = settings.resolve(q.settings.width, q.settings.height, q.settings.frame_rate, q.settings.sample_rate); + filmcraft_project::validate_frame_size(r.width, r.height).map_err(ExportError::Unsupported)?; let geom = Geometry::new(settings, q.settings.width, q.settings.height, r.width, r.height); + if let Some((w, h)) = geom.content { + let w = u32::try_from(w).map_err(|_| ExportError::Unsupported("scaled picture width is too large".into()))?; + let h = u32::try_from(h).map_err(|_| ExportError::Unsupported("scaled picture height is too large".into()))?; + filmcraft_project::validate_frame_size(w, h).map_err(ExportError::Unsupported)?; + } + let (w, h) = filmcraft_render::output_size(q, geom.render_scale); + let w = u32::try_from(w).map_err(|_| ExportError::Unsupported("rendered picture width is too large".into()))?; + let h = u32::try_from(h).map_err(|_| ExportError::Unsupported("rendered picture height is too large".into()))?; + filmcraft_project::validate_frame_size(w, h).map_err(ExportError::Unsupported)?; let pipe = q.settings.color; let out_tf = hdr_out.then(|| filmcraft_color::OutputTransform::new(&pipe, pipe.working.output_space())); let opts = RenderOptions { scale: geom.render_scale, captions: settings.burn_captions, working_output: hdr_out, ..Default::default() }; diff --git a/crates/export/src/settings.rs b/crates/export/src/settings.rs index 34cf205a..91d89a16 100644 --- a/crates/export/src/settings.rs +++ b/crates/export/src/settings.rs @@ -382,7 +382,7 @@ impl ExportSettings { .max(100); let max = match self.bitrate_mode { BitrateMode::Cbr => target, - _ => self.max_bitrate_kbps.filter(|m| *m >= target).unwrap_or(target * 3 / 2), + _ => self.max_bitrate_kbps.filter(|m| *m >= target).unwrap_or_else(|| (u64::from(target) * 3 / 2).min(u64::from(u32::MAX)) as u32), }; let keyint = self.keyframe_distance.filter(|k| *k > 0).unwrap_or_else(|| (fps * 2.0).round().max(1.0) as u32); let channels = match self.audio.channels { diff --git a/crates/export/src/settings_tests.rs b/crates/export/src/settings_tests.rs index 2fb9988c..63e83967 100644 --- a/crates/export/src/settings_tests.rs +++ b/crates/export/src/settings_tests.rs @@ -145,6 +145,40 @@ fn wav_and_aiff_audio_only() { } } +#[test] +fn invalid_export_allocations_are_rejected_before_rendering() { + let (project, sequence, _) = matte([1.0, 0.0, 0.0, 1.0], 64, 36, None); + for size in [(0, 36), (64, 0), (u32::MAX, u32::MAX), (16384, 16384)] { + let settings = ExportSettings { frame_size: Some(size), ..Default::default() }; + assert!(settings.validate().is_err(), "{size:?}"); + assert!(pipeline::Pipeline::new(project.clone(), sequence, &settings, false).is_err()); + } + for scale in [0.0, -1.0, f32::NAN, f32::INFINITY, 1e30] { + let settings = ExportSettings { scale, ..Default::default() }; + assert!(pipeline::Pipeline::new(project.clone(), sequence, &settings, false).is_err(), "scale {scale}"); + } + for rate in [0, 384_001, u32::MAX] { + let settings = ExportSettings { audio: AudioSettings { sample_rate: Some(rate), ..Default::default() }, ..Default::default() }; + assert!(settings.validate().is_err()); + } +} + +#[test] +fn hostile_export_ranges_are_rejected_before_time_arithmetic() { + let (project, seq, sources) = matte([1.0, 0.0, 0.0, 1.0], 64, 36, None); + for range in [ + TimeRange::new(Tick(i64::MIN), Tick(i64::MAX)), + TimeRange::new(Tick(i64::MAX), Tick(1)), + TimeRange::new(Tick::ZERO, Tick(-1)), + TimeRange::new(Tick::ZERO, Tick::ZERO), + ] { + let settings = ExportSettings { range: Some(range), ..Default::default() }; + assert!(settings.validate().is_err()); + assert!(export_range(&project, seq, &settings).is_err()); + assert!(export(&project, seq, &settings, &sources, &Progress::default()).is_err()); + } +} + #[test] fn frame_size_rate_and_scaling() { let (p, seq, m) = matte([1.0, 0.0, 0.0, 1.0], 320, 180, Some(-12.0)); @@ -479,3 +513,21 @@ fn h265_is_a_format_that_needs_a_registered_encoder() { HERE.store(false, Ordering::SeqCst); assert!(!available(Format::Hevc)); } + +#[test] +fn extreme_bitrates_do_not_overflow_resolution() { + let s = ExportSettings { bitrate_kbps: u32::MAX, adaptive_bitrate: None, ..Default::default() }; + let resolved = s.resolve(1920, 1080, FrameRate::FPS_30, 48_000); + assert_eq!(resolved.target_kbps, u32::MAX); + assert_eq!(resolved.max_kbps, u32::MAX); +} + +#[test] +fn extreme_bitrates_do_not_overflow_encoder_setup() { + for max_bitrate_kbps in [None, Some(u32::MAX)] { + let settings = ExportSettings { bitrate_kbps: u32::MAX, max_bitrate_kbps, ..Default::default() }; + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| super::h264_factory(Format::H264, 64, 36, FrameRate::FPS_24, &settings))); + assert!(result.is_ok(), "an extreme bitrate must return an encoder result without overflowing its fallback"); + assert!(result.unwrap().unwrap().is_err(), "an unrepresentable encoder buffer rate must be rejected"); + } +} diff --git a/crates/format/src/lib.rs b/crates/format/src/lib.rs index 2cb7f1cd..6dc6b5b3 100644 --- a/crates/format/src/lib.rs +++ b/crates/format/src/lib.rs @@ -198,12 +198,21 @@ pub fn decode(bytes: &[u8]) -> Result { if found == SCHEMA_VERSION { // Fast path: deserialize straight into the model. let env: EnvelopeIn = serde_json::from_slice(bytes).map_err(|e| FormatError::Corrupt(e.to_string()))?; - return Ok(env.loaded(found)); + return validate_loaded(env.loaded(found)); } let doc: Value = serde_json::from_slice(bytes).map_err(|e| FormatError::Corrupt(e.to_string()))?; let doc = migrate_with(MIGRATIONS, doc, found)?; let env: EnvelopeIn = serde_json::from_value(doc).map_err(|e| FormatError::Corrupt(format!("after upgrading from schema v{found}: {e}")))?; - Ok(env.loaded(found)) + validate_loaded(env.loaded(found)) +} + +fn validate_loaded(loaded: Loaded) -> Result { + for item in loaded.project.sequences() { + if let filmcraft_project::ItemKind::Sequence(sequence) = &item.kind { + sequence.check().map_err(|e| FormatError::Corrupt(format!("sequence `{}`: {e}", item.name)))?; + } + } + Ok(loaded) } fn check_supported(found: u32) -> Result<(), FormatError> { @@ -313,6 +322,17 @@ mod tests { assert_eq!(SCHEMA_VERSION, 12); } + #[test] + fn damaged_sequence_settings_are_rejected_before_rendering() { + let mut p = Project::new("Invalid sequence"); + let id = p.new_sequence("Bad", filmcraft_project::SequenceSettings::default(), 1, 1, None); + for (width, height, sample_rate) in [(0, 90, 48000), (160, 0, 48000), (160, 90, 0), (16384, 16384, 48000)] { + let settings = &mut p.sequence_mut(id).unwrap().settings; + (settings.width, settings.height, settings.sample_rate) = (width, height, sample_rate); + assert!(matches!(decode(&encode(&p, false)), Err(FormatError::Corrupt(_)))); + } + } + #[test] fn roundtrip_current() { let p = Project::new("Round Trip"); diff --git a/crates/h264enc/src/lib.rs b/crates/h264enc/src/lib.rs index bbf866be..c09c8cc0 100644 --- a/crates/h264enc/src/lib.rs +++ b/crates/h264enc/src/lib.rs @@ -381,11 +381,13 @@ impl Encoder { (RateControl::Cbr { kbps }, Pass::First) => (Some(*kbps), RcKind::Abr { kbps: *kbps }, (None, None)), (RateControl::Cbr { kbps }, Pass::Single) => (Some(*kbps), RcKind::Abr { kbps: *kbps }, (Some(*kbps), Some(*kbps))), (RateControl::Vbr { target_kbps, max_kbps }, Pass::Second(st)) => { - (Some(*max_kbps), ratecontrol::RateControl::plan_two_pass(st, *target_kbps), (Some(*max_kbps), Some(max_kbps * 2))) + let buffer = max_kbps.checked_mul(2).ok_or_else(|| Error::InvalidConfig("VBR buffer rate exceeds the supported integer range".into()))?; + (Some(*max_kbps), ratecontrol::RateControl::plan_two_pass(st, *target_kbps), (Some(*max_kbps), Some(buffer))) } (RateControl::Vbr { target_kbps, max_kbps }, Pass::First) => (Some(*max_kbps), RcKind::Abr { kbps: *target_kbps }, (None, None)), (RateControl::Vbr { target_kbps, max_kbps }, Pass::Single) => { - (Some(*max_kbps), RcKind::Abr { kbps: *target_kbps }, (Some(*max_kbps), Some(max_kbps * 2))) + let buffer = max_kbps.checked_mul(2).ok_or_else(|| Error::InvalidConfig("VBR buffer rate exceeds the supported integer range".into()))?; + (Some(*max_kbps), RcKind::Abr { kbps: *target_kbps }, (Some(*max_kbps), Some(buffer))) } }; if matches!(cfg.rate, RateControl::Cbr { kbps: 0 } | RateControl::Vbr { target_kbps: 0, .. }) { diff --git a/crates/mxf/src/essence.rs b/crates/mxf/src/essence.rs index f1524a43..c74a17ea 100644 --- a/crates/mxf/src/essence.rs +++ b/crates/mxf/src/essence.rs @@ -188,7 +188,7 @@ pub struct SoundInfo { impl SoundInfo { /// Bytes per sample frame of plain PCM. pub fn frame_bytes(&self) -> usize { - if self.block_align > 0 { self.block_align as usize } else { self.channels.max(1) as usize * (self.bits as usize).div_ceil(8).max(1) } + if self.block_align > 0 { self.block_align as usize } else { (self.channels.max(1) as usize).saturating_mul((self.bits as usize).div_ceil(8).max(1)) } } } @@ -208,6 +208,9 @@ pub fn aes3_element_samples(v: &[u8]) -> usize { /// (bits 4-27 of the little-endian 32-bit word), first `channels` of the 8 channels. pub fn decode_pcm(v: &[u8], format: SoundFormat, channels: usize, bits: u32, frame_bytes: usize) -> Result>> { let ch = channels.max(1); + if ch > 256 { + return Err(crate::Error::Invalid("PCM channel count exceeds 256".into())); + } match format { SoundFormat::Pcm => { let bps = (bits as usize).div_ceil(8); @@ -215,6 +218,9 @@ pub fn decode_pcm(v: &[u8], format: SoundFormat, channels: usize, bits: u32, fra return Err(crate::Error::Unsupported(format!("{bits}-bit PCM with {frame_bytes}-byte frames"))); } let n = v.len() / frame_bytes; + if n.checked_mul(ch).is_none_or(|n| n > 16_777_216) { + return Err(crate::Error::Invalid("PCM decode exceeds the sample buffer limit".into())); + } let mut out = vec![Vec::with_capacity(n); ch]; let scale = 1.0 / (1u64 << (8 * bps - 1)) as f32; for i in 0..n { @@ -233,6 +239,9 @@ pub fn decode_pcm(v: &[u8], format: SoundFormat, channels: usize, bits: u32, fra Ok(out) } SoundFormat::Aes3Element => { + if ch > 8 { + return Err(crate::Error::Invalid("an AES3 element has at most eight channels".into())); + } let n = aes3_element_samples(v); let mut out = vec![Vec::with_capacity(n); ch]; for i in 0..n { diff --git a/crates/mxf/src/file.rs b/crates/mxf/src/file.rs index 34ae62e0..4e3ed365 100644 --- a/crates/mxf/src/file.rs +++ b/crates/mxf/src/file.rs @@ -223,11 +223,11 @@ pub struct EssenceTrack { impl EssenceTrack { /// First presented edit unit (stored position of the material package's zero point). pub fn first_edit_unit(&self) -> i64 { - (self.origin + self.start_position).max(0) + self.origin.saturating_add(self.start_position).max(0) } /// Total sound sample frames stored. pub fn stored_sample_frames(&self) -> u64 { - self.chunks.last().map_or(0, |c| c.first_sample + c.samples) + self.chunks.last().map_or(0, |c| c.first_sample.saturating_add(c.samples)) } /// Sound sample frames per edit unit (rational), e.g. 48000/25. pub fn samples_per_edit_unit(&self) -> (i64, i64) { @@ -749,27 +749,28 @@ pub fn open(src: &(impl ByteSource + ?Sized)) -> Result { } /// Index entries per stored edit unit for an index SID (all segments when `sid` is 0 or unmatched). -fn index_entries(index: &[IndexSegment], sid: u32) -> (Vec>, u32) { +fn index_entries(index: &[IndexSegment], sid: u32) -> Result<(std::collections::BTreeMap, u32)> { let any = index.iter().any(|s| s.index_sid == sid); let segs: Vec<&IndexSegment> = index.iter().filter(|s| !any || s.index_sid == sid).collect(); - let mut out: Vec> = Vec::new(); + // ST 377-1 segments use absolute edit-unit positions, which need not start at zero. + // Keep positions sparse so even hostile gaps allocate only for real entries. + segs.iter() + .try_fold(0usize, |n, s| n.checked_add(s.entries.len())) + .filter(|&n| n <= 1_048_576) + .ok_or_else(|| Error::Invalid("index exceeds the stored-entry limit".into()))?; + let mut out = std::collections::BTreeMap::new(); let mut eubc = 0; for s in segs { if s.edit_unit_byte_count > 0 { eubc = s.edit_unit_byte_count; } for (k, e) in s.entries.iter().enumerate() { - let Ok(p) = usize::try_from(s.start_position + k as i64) else { continue }; - if p > 50_000_000 { - break; - } - if out.len() <= p { - out.resize(p + 1, None); - } - out[p] = Some(*e); + let Some(position) = s.start_position.checked_add(k as i64) else { break }; + let Ok(p) = usize::try_from(position) else { continue }; + out.insert(p, *e); } } - (out, eubc) + Ok((out, eubc)) } fn build_track( @@ -871,8 +872,8 @@ fn build_pictures( index: &[IndexSegment], warnings: &mut Vec, ) -> Result<()> { - let (entries, eubc) = index_entries(index, t.index_sid); - let expected = t.duration.map(|d| (d + t.first_edit_unit()).max(0) as usize); + let (entries, eubc) = index_entries(index, t.index_sid)?; + let expected = t.duration.map(|d| d.saturating_add(t.first_edit_unit()).max(0) as usize); let clip = elements.len() == 1 && expected.is_none_or(|n| n > 1) && (eubc > 0 || entries.len() > 1); let mut samples: Vec = Vec::new(); if clip { @@ -884,7 +885,8 @@ fn build_pictures( samples.push(Sample { offset: e.value_offset + i * eubc as u64, size: eubc, pts: i as i64, key: true, b_picture: false }); } } else { - let offs: Vec = entries.iter().map_while(|x| x.map(|x| x.stream_offset)).collect(); + let offs: Vec = + entries.iter().enumerate().map_while(|(position, (&indexed, entry))| (position == indexed).then_some(entry.stream_offset)).collect(); let base = offs.first().copied().unwrap_or(0); for (i, o) in offs.iter().enumerate() { let start = o - base.min(*o); @@ -902,12 +904,12 @@ fn build_pictures( } let n = samples.len(); // Key frames and B pictures from the index. - let flagged = entries.iter().flatten().any(|e| e.random_access()); + let flagged = entries.values().any(|e| e.random_access()); if !t.codec.intra_only() { if flagged { t.indexed = true; for (i, s) in samples.iter_mut().enumerate() { - let e = entries.get(i).copied().flatten(); + let e = entries.get(&i).copied(); s.key = e.is_some_and(|e| e.random_access()) || i == 0; s.b_picture = e.is_some_and(|e| e.is_b()); } @@ -924,12 +926,12 @@ fn build_pictures( } // Presentation order from the temporal offsets: display d is stored at d + TO[d]. let mut display: Vec = (0..n).collect(); - let has_to = entries.iter().take(n).flatten().any(|e| e.temporal_offset != 0); + let has_to = entries.range(..n).any(|(_, e)| e.temporal_offset != 0); if has_to { let mut pts = vec![i64::MIN; n]; let mut ok = true; for d in 0..n { - let to = entries.get(d).copied().flatten().map_or(0, |e| e.temporal_offset as i64); + let to = entries.get(&d).copied().map_or(0, |e| e.temporal_offset as i64); let s = d as i64 + to; if s < 0 || s >= n as i64 || pts[s as usize] != i64::MIN { ok = false; @@ -992,6 +994,9 @@ impl MxfFile { pub fn read_sample(&self, src: &(impl ByteSource + ?Sized), track: usize, i: usize) -> Result> { let t = self.tracks.get(track).ok_or_else(|| Error::Invalid(format!("no track {track}")))?; let s = t.samples.get(i).ok_or_else(|| Error::Invalid(format!("no sample {i}")))?; + if u64::from(s.size) > src.len().saturating_sub(s.offset) || s.size > 268_435_456 { + return Err(Error::Invalid("picture sample exceeds the source or frame buffer limit".into())); + } let mut v = vec![0u8; s.size as usize]; src.read_at(s.offset, &mut v)?; Ok(v) @@ -1003,25 +1008,44 @@ impl MxfFile { let t = self.tracks.get(track).ok_or_else(|| Error::Invalid(format!("no track {track}")))?; let si = t.sound.as_ref().ok_or_else(|| Error::Invalid("not a sound track".into()))?; let ch = si.channels.max(1) as usize; + if ch > 256 || frames.checked_mul(ch).is_none_or(|n| n > 16_777_216) { + return Err(Error::Invalid("sound request exceeds the channel or sample buffer limit".into())); + } + if t.sound_format == SoundFormat::Pcm && (si.bits == 0 || si.bits > 32) { + return Err(Error::Unsupported("invalid PCM bit depth or block alignment".into())); + } + let fb = if t.sound_format == SoundFormat::Pcm { si.frame_bytes() } else { 0 }; + if t.sound_format == SoundFormat::Pcm && (fb == 0 || fb > 4096) { + return Err(Error::Unsupported("invalid PCM block alignment".into())); + } let mut out = vec![vec![0f32; frames]; ch]; - let end = start + frames as u64; - let fb = si.frame_bytes(); - let mut ci = t.chunks.partition_point(|c| c.first_sample + c.samples <= start); + let end = start.saturating_add(frames as u64); + let mut ci = t.chunks.partition_point(|c| c.first_sample.saturating_add(c.samples) <= start); while let Some(c) = t.chunks.get(ci) { if c.first_sample >= end { break; } let a = start.max(c.first_sample); - let b = end.min(c.first_sample + c.samples); + let b = end.min(c.first_sample.saturating_add(c.samples)); if b > a { let decoded = match t.sound_format { SoundFormat::Pcm => { - let off = c.offset + (a - c.first_sample) * fb as u64; - let mut v = vec![0u8; ((b - a) as usize) * fb]; + let off = (a - c.first_sample) + .checked_mul(fb as u64) + .and_then(|n| c.offset.checked_add(n)) + .ok_or_else(|| Error::Invalid("PCM chunk offset overflows".into()))?; + let bytes = ((b - a) as usize) + .checked_mul(fb) + .filter(|&n| n <= 268_435_456) + .ok_or_else(|| Error::Invalid("PCM chunk exceeds the buffer limit".into()))?; + let mut v = vec![0u8; bytes]; src.read_at(off, &mut v)?; decode_pcm(&v, SoundFormat::Pcm, ch, si.bits, fb)? } SoundFormat::Aes3Element => { + if c.size > 268_435_456 { + return Err(Error::Invalid("AES3 chunk exceeds the buffer limit".into())); + } let mut v = vec![0u8; c.size as usize]; src.read_at(c.offset, &mut v)?; let all = decode_pcm(&v, SoundFormat::Aes3Element, ch, si.bits, 0)?; @@ -1042,3 +1066,27 @@ impl MxfFile { Ok(out) } } + +#[cfg(test)] +mod index_bounds_tests { + use super::*; + + #[test] + fn nonzero_segment_positions_and_sparse_gaps_preserve_entries() { + let entry = IndexEntry { flags: 0x80, stream_offset: 1234, ..Default::default() }; + for start_position in [5, 49_999_999, i64::MAX] { + let segment = IndexSegment { start_position, entries: vec![entry, entry], ..Default::default() }; + let (entries, _) = index_entries(&[segment], 0).unwrap(); + let start = usize::try_from(start_position).unwrap(); + assert_eq!(entries.get(&start), Some(&entry)); + assert_eq!(entries.len(), if start_position == i64::MAX { 1 } else { 2 }); + } + } + + #[test] + fn negative_preroll_positions_do_not_drop_later_valid_entries() { + let segment = IndexSegment { start_position: -1, entries: vec![IndexEntry::default(); 3], ..Default::default() }; + let (entries, _) = index_entries(&[segment], 0).unwrap(); + assert_eq!(entries.keys().copied().collect::>(), vec![0, 1]); + } +} diff --git a/crates/mxf/src/tests.rs b/crates/mxf/src/tests.rs index b9ed40f1..86ca0ed9 100644 --- a/crates/mxf/src/tests.rs +++ b/crates/mxf/src/tests.rs @@ -361,3 +361,29 @@ fn drop_frame_timecode_format() { let tc = Timecode { start: 90_000 + 25 * 61 + 3, rounded_base: 25, drop_frame: false }; assert_eq!(tc.format(), "01:01:01:03"); } + +#[test] +fn hostile_pcm_requests_fail_before_allocation() { + let (to, flags) = ipbb(); + let bytes = build(5, prores_like, &to, &flags); + let mut file = open(&bytes).unwrap(); + let track = file.track_of_kind(TrackKind::Sound).unwrap(); + file.tracks[track].sound.as_mut().unwrap().channels = u32::MAX; + assert!(file.read_pcm(&bytes, track, 0, 100).is_err()); + file.tracks[track].sound.as_mut().unwrap().channels = 2; + file.tracks[track].sound.as_mut().unwrap().block_align = u32::MAX; + assert!(file.read_pcm(&bytes, track, 0, 100).is_err()); + assert!(file.read_pcm(&bytes, track, 0, usize::MAX).is_err()); + assert!(crate::decode_pcm(&[], SoundFormat::Pcm, usize::MAX, 16, 4).is_err()); +} + +#[test] +fn a_corrupt_index_position_cannot_allocate_a_giant_sparse_table() { + let (to, flags) = ipbb(); + let mut bytes = build(5, prores_like, &to, &flags); + let prefix = [0x3f, 0x0c, 0, 8, 0, 0, 0, 0, 0, 0, 0, 0]; + let at = bytes.windows(prefix.len()).position(|w| w == prefix).unwrap(); + bytes[at + 4..at + 12].copy_from_slice(&49_999_999i64.to_be_bytes()); + let file = open(&bytes).unwrap(); + assert_eq!(file.tracks[file.track_of_kind(TrackKind::Picture).unwrap()].samples.len(), 5); +} diff --git a/crates/project/src/caption.rs b/crates/project/src/caption.rs index 0583ade9..77b67efb 100644 --- a/crates/project/src/caption.rs +++ b/crates/project/src/caption.rs @@ -209,6 +209,9 @@ impl CaptionTrack { /// Invariant check: captions sorted, positive duration, no overlaps. pub fn check(&self) -> Result<(), String> { for c in &self.captions { + if !crate::bounded_time_range(c.start, c.duration) { + return Err(format!("{}: caption {:?} is outside supported time bounds", self.name, c.id)); + } if c.duration.0 <= 0 { return Err(format!("{}: caption {:?} has non-positive duration", self.name, c.id)); } @@ -310,4 +313,23 @@ mod tests { assert_eq!(CaptionFormat::from_name("cea608"), Some(CaptionFormat::Cea608)); assert_eq!(CaptionFormat::from_name("CEA-708"), Some(CaptionFormat::Cea708)); } + + #[test] + fn corrupt_caption_times_do_not_overflow_the_overlap_check() { + let mut track = CaptionTrack::new(TrackId(1), "Subtitles".into(), CaptionFormat::Subtitle); + for (id, start) in [(1, i64::MAX), (2, 10)] { + track.captions.push(Caption { + id: ClipId(id), + start: Tick(start), + duration: Tick(50), + text: String::new(), + speaker: None, + cue_id: None, + settings: String::new(), + }); + } + let result = std::panic::catch_unwind(|| track.check()); + assert!(result.is_ok()); + assert!(result.unwrap().is_err()); + } } diff --git a/crates/project/src/lib.rs b/crates/project/src/lib.rs index 7ed521a8..4e1dba64 100644 --- a/crates/project/src/lib.rs +++ b/crates/project/src/lib.rs @@ -786,6 +786,17 @@ impl Track { } /// Invariant check: items do not overlap. pub fn check(&self) -> Result<(), String> { + // Validate before calling end(), including on a single-item track. + for item in &self.items { + if !bounded_time_range(item.start, item.duration) { + return Err(format!("{}: item {:?} is outside supported time bounds", self.name, item.id)); + } + } + for transition in &self.transitions { + if !bounded_time_range(transition.start, transition.duration) { + return Err(format!("{}: transition {:?} is outside supported time bounds", self.name, transition.id)); + } + } for w in self.items.windows(2) { if w[0].end() > w[1].start { return Err(format!("{}: items {:?} and {:?} overlap", self.name, w[0].id, w[1].id)); @@ -847,6 +858,36 @@ impl Default for SequenceSettings { } } +/// Bound working-frame allocations while retaining standard 8K and wide panoramic frames. +pub fn validate_frame_size(width: u32, height: u32) -> Result<(), String> { + if width == 0 || height == 0 || width > 16_384 || height > 16_384 || u64::from(width) * u64::from(height) > 67_108_864 { + return Err("frame size must be positive, at most 16384 pixels per side and 67108864 pixels total".into()); + } + Ok(()) +} + +impl SequenceSettings { + pub fn validate(&self) -> Result<(), String> { + validate_frame_size(self.width, self.height)?; + if self.frame_rate.num <= 0 + || self.frame_rate.den <= 0 + || self.frame_rate.as_f64() > 1000.0 + || self.frame_rate.num > i64::from(u32::MAX) + || self.frame_rate.den > i64::from(u32::MAX) + || (i128::from(TICKS_PER_SECOND) * i128::from(self.frame_rate.den) / i128::from(self.frame_rate.num.max(1))) > i128::from(Tick::MAX.0) + { + return Err("frame rate must be positive, at most 1000 fps, with unsigned 32-bit rational components".into()); + } + if self.sample_rate == 0 || self.sample_rate > 384_000 { + return Err("sample rate must be between 1 and 384000 Hz".into()); + } + if self.par.0 == 0 || self.par.1 == 0 { + return Err("pixel aspect ratio must be positive".into()); + } + Ok(()) + } +} + #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct Sequence { pub settings: SequenceSettings, @@ -976,6 +1017,12 @@ impl Sequence { v } pub fn check(&self) -> Result<(), String> { + self.settings.validate()?; + if self.mark_in.into_iter().chain(self.mark_out).any(|t| t < Tick::MIN || t > Tick::MAX) + || self.work_area.is_some_and(|r| !bounded_time_range(r.start, r.duration)) + { + return Err("sequence marks or work area are outside supported time bounds".into()); + } for t in self.all_tracks() { t.check()?; } @@ -986,6 +1033,13 @@ impl Sequence { } } +/// Bound persisted timeline arithmetic before any start + duration operation. Negative timeline +/// positions remain supported; empty ranges are valid metadata (track items/captions also require +/// a positive duration in their own invariant checks). +pub(crate) fn bounded_time_range(start: Tick, duration: Tick) -> bool { + start >= Tick::MIN && start <= Tick::MAX && duration.0 >= 0 && start.0.checked_add(duration.0).is_some_and(|end| end <= Tick::MAX.0) +} + /// Project-level settings. #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct ProjectSettings { @@ -1625,6 +1679,36 @@ mod tests { assert!(q.sequence(seq).unwrap().check().is_ok()); } + #[test] + fn hostile_frame_rates_are_rejected_before_frame_arithmetic() { + for frame_rate in [FrameRate { num: i64::MAX, den: i64::MAX }, FrameRate { num: 1, den: i64::from(u32::MAX) }] { + let settings = SequenceSettings { frame_rate, ..Default::default() }; + assert!(settings.validate().is_err()); + } + assert!(SequenceSettings { width: 7680, height: 4320, ..Default::default() }.validate().is_ok()); + } + + #[test] + fn corrupt_timeline_times_are_rejected_before_end_arithmetic() { + let (mut project, media, seq) = demo_project(); + let rate = project.sequence(seq).unwrap().settings.frame_rate; + let range = TimeRange::new(Tick::ZERO, rate.tick_of(24)); + let first = project.make_track_item(media, TrackKind::Video, Tick::ZERO, range, rate).unwrap(); + let second = project.make_track_item(media, TrackKind::Video, rate.tick_of(24), range, rate).unwrap(); + project.sequence_mut(seq).unwrap().video_tracks[0].items = vec![first, second]; + for start in [Tick(i64::MAX), Tick(i64::MIN), Tick::MAX] { + project.sequence_mut(seq).unwrap().video_tracks[0].items[0].start = start; + let result = std::panic::catch_unwind(|| project.sequence(seq).unwrap().check()); + assert!(result.is_ok()); + assert!(result.unwrap().is_err()); + } + project.sequence_mut(seq).unwrap().video_tracks[0].items[0].start = Tick::ZERO; + for mark in [Tick(i64::MAX), Tick(i64::MIN)] { + project.sequence_mut(seq).unwrap().mark_out = Some(mark); + assert!(project.sequence(seq).unwrap().check().is_err()); + } + } + #[test] fn point_params_keep_auto_through_json_and_no_other_point_reads_null() { // serde_json writes NaN as `null`; a point parameter reads it back as NaN ("auto")