Merge pull request #14 from storytold/pr/never-crash-interchange

Never crash: interchange import/export (bad precomp refs, deep XML) without panics
This commit is contained in:
Brandon Thomas 2026-10-04 15:29:41 -04:00 committed by GitHub
commit 43c485e31e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 163 additions and 26 deletions

View File

@ -254,7 +254,7 @@ pub fn export(project: &Project, comp: ItemId, opts: &ExportOptions, prerendered
bins: HashMap::new(),
stack: vec![],
};
let seq = ex.sequence(comp);
let seq = ex.sequence(comp).ok_or(Error::NoComp(comp.0))?;
let fc_opts = filmcraft_interchange::ExportOptions { relative_to: opts.relative_to.clone(), name: Some(name), ..Default::default() };
let (bytes, report) = filmcraft_interchange::export(&ex.fp, seq, opts.format.fc(), &fc_opts)?;
for e in report.entries {
@ -390,12 +390,12 @@ impl Exporter<'_> {
id
}
/// The sequence for a comp (created once).
fn sequence(&mut self, cid: ItemId) -> fp::ItemId {
/// The sequence for a comp (created once); None if `cid` is not a composition.
fn sequence(&mut self, cid: ItemId) -> Option<fp::ItemId> {
if let Some(s) = self.seqs.get(&cid) {
return *s;
return Some(*s);
}
let comp = self.p.comp_arc(cid).expect("comp");
let comp = self.p.comp_arc(cid)?;
let it = self.p.item(cid);
let name = it.map(|i| i.name.clone()).unwrap_or_default();
let settings = fp::SequenceSettings {
@ -430,12 +430,13 @@ impl Exporter<'_> {
Plan::Render(why) => match self.pre.get(&(cid, l.id)).cloned() {
Some(r) => {
let item = self.prerendered_item(&format!("{} (pre-rendered)", l.name), &r);
let mut ti = self.track_item(item, &l.name, fp::TrackKind::Video, &tm, rate);
ti.source_in = filmcraft_time::Tick::ZERO;
ti.speed = 1.0;
ti.reverse = false;
self.out.prerendered += 1;
Some(ti)
self.track_item(item, &l.name, fp::TrackKind::Video, &tm, rate).map(|mut ti| {
ti.source_in = filmcraft_time::Tick::ZERO;
ti.speed = 1.0;
ti.reverse = false;
self.out.prerendered += 1;
ti
})
}
None => {
self.warn(format!("layer \"{}\" was left out ({why}; pre-rendering is off or unavailable)", l.name));
@ -457,16 +458,16 @@ impl Exporter<'_> {
Some(ItemKind::Solid(s)) => Some(self.solid_item(*item, s.color, &comp)),
_ => None,
},
LayerSource::Comp { item } if !self.stack.contains(item) => Some(self.sequence(*item)),
LayerSource::Comp { item } if !self.stack.contains(item) => self.sequence(*item),
_ => None,
};
item.map(|item| {
let mut ti = self.track_item(item, &l.name, fp::TrackKind::Video, &tm, rate);
item.and_then(|item| {
let mut ti = self.track_item(item, &l.name, fp::TrackKind::Video, &tm, rate)?;
self.motion(&mut ti, l, &comp, &tm);
if let Some(h) = l.props.get("timeRemap").filter(|p| !p.is_animated()) {
ti.frame_hold = Some(tick_out(Tick::from_seconds_f64(h.value.as_f64())));
}
ti
Some(ti)
})
}
};
@ -486,14 +487,14 @@ impl Exporter<'_> {
_ => None,
},
LayerSource::Comp { item } if self.p.comp(*item).is_some_and(|c| comp_has_audio(self.p, c)) && !self.stack.contains(item) => {
Some(self.sequence(*item))
self.sequence(*item)
}
_ => None,
};
if let Some(item) = sound
&& !matches!(pl, Plan::Skip(_))
&& let Some(mut ti) = self.track_item(item, &l.name, fp::TrackKind::Audio, &tm, rate)
{
let mut ti = self.track_item(item, &l.name, fp::TrackKind::Audio, &tm, rate);
ti.enabled = l.switches.audio;
ti.link = has_video_clip.then_some(link);
self.levels(&mut ti, l, &tm);
@ -529,18 +530,18 @@ impl Exporter<'_> {
seq.work_area = None;
}
self.stack.pop();
sid
Some(sid)
}
fn track_item(&mut self, item: fp::ItemId, name: &str, kind: fp::TrackKind, tm: &Timing, rate: FrameRate) -> fp::TrackItem {
fn track_item(&mut self, item: fp::ItemId, name: &str, kind: fp::TrackKind, tm: &Timing, rate: FrameRate) -> Option<fp::TrackItem> {
let range = filmcraft_time::TimeRange::new(tick_out(tm.source_in), tick_out(Tick((tm.duration.0 as f64 * tm.speed).round() as i64)));
let mut ti = self.fp.make_track_item(item, kind, tick_out(tm.start), range, rate_out(rate)).expect("item exists");
let mut ti = self.fp.make_track_item(item, kind, tick_out(tm.start), range, rate_out(rate))?;
ti.name = name.to_string();
ti.duration = tick_out(tm.duration);
ti.source_in = tick_out(tm.source_in);
ti.speed = tm.speed;
ti.reverse = tm.reverse;
ti
Some(ti)
}
/// Transform → Motion + Opacity (clip-time keyframes).
@ -554,7 +555,7 @@ impl Exporter<'_> {
_ => (comp.width, comp.height),
};
let center = |x: f64, y: f64| -> fp::ParamValue { fp::ParamValue::Vec2(Vec2 { x, y }) };
let mut m = ti.effect("motion").cloned().unwrap_or_else(|| fp::find_effect("motion").expect("motion").instance());
let Some(mut m) = effect_or_new(ti, "motion") else { return };
if let Some(p) = tr.get("position") {
let def = [comp.width as f64 / 2.0, comp.height as f64 / 2.0];
if p.is_animated() || differs(&p.value, def) {
@ -594,8 +595,9 @@ impl Exporter<'_> {
m.params.insert("rotation".into(), param_out(p, &to_clip, |v| fp::ParamValue::Float(v.as_f64())));
}
set_effect(ti, m);
if let Some(p) = tr.get("opacity") {
let mut o = ti.effect("opacity").cloned().unwrap_or_else(|| fp::find_effect("opacity").expect("opacity").instance());
if let Some(p) = tr.get("opacity")
&& let Some(mut o) = effect_or_new(ti, "opacity")
{
o.params.insert("opacity".into(), param_out(p, &to_clip, |v| fp::ParamValue::Float(v.as_f64())));
if l.blend_mode != effectcraft_color::BlendMode::Normal
&& let Some(i) = fp::effect::BLEND_MODES.iter().position(|m| m.eq_ignore_ascii_case(l.blend_mode.label()))
@ -615,7 +617,7 @@ impl Exporter<'_> {
if (p.value.as_vec2()[0] - p.value.as_vec2()[1]).abs() > 1e-9 {
self.warn("separate left/right audio levels were exported as the left level");
}
let mut v = ti.effect("volume").cloned().unwrap_or_else(|| fp::find_effect("volume").expect("volume").instance());
let Some(mut v) = effect_or_new(ti, "volume") else { return };
v.params.insert("level".into(), param_out(p, &to_clip, |v| fp::ParamValue::Float(v.as_vec2()[0])));
set_effect(ti, v);
}
@ -634,6 +636,11 @@ fn differs(v: &Value, def: [f64; 2]) -> bool {
(a[0] - def[0]).abs() > 1e-6 || (a[1] - def[1]).abs() > 1e-6
}
/// The clip's `id` effect, or a new instance of it (None if the registry has no such effect).
fn effect_or_new(ti: &fp::TrackItem, id: &str) -> Option<fp::EffectInstance> {
ti.effect(id).cloned().or_else(|| fp::find_effect(id).map(|e| e.instance()))
}
fn set_effect(ti: &mut fp::TrackItem, e: fp::EffectInstance) {
match ti.effects.iter_mut().find(|x| x.effect == e.effect) {
Some(x) => *x = e,

View File

@ -45,6 +45,83 @@ pub struct ImportResult {
/// Import a timeline document into `project`. `probe` reads a media file's footage metadata
/// (None = missing or unreadable: the item becomes a placeholder with the document's metadata).
/// The deepest element / array nesting accepted in XML and OTIO documents. Real timelines nest
/// a few levels per nested sequence; parsing several thousand levels overflowed the stack.
pub const MAX_DEPTH: usize = 256;
/// The deepest element nesting of an XML document (a quick scan that skips comments, CDATA,
/// processing instructions, declarations and quoted attribute values).
pub(crate) fn xml_depth(b: &[u8]) -> usize {
let (mut i, mut depth, mut max) = (0usize, 0usize, 0usize);
let skip_to = |from: usize, end: &[u8]| b.get(from..).and_then(|r| r.windows(end.len()).position(|w| w == end)).map_or(b.len(), |p| from + p + end.len());
while let Some(&c) = b.get(i) {
if c != b'<' {
i += 1;
continue;
}
let rest = b.get(i..).unwrap_or_default();
if rest.starts_with(b"<!--") {
i = skip_to(i + 4, b"-->");
} else if rest.starts_with(b"<![CDATA[") {
i = skip_to(i + 9, b"]]>");
} else if rest.starts_with(b"<?") {
i = skip_to(i + 2, b"?>");
} else if rest.starts_with(b"<!") {
i = skip_to(i + 2, b">");
} else if rest.starts_with(b"</") {
depth = depth.saturating_sub(1);
i = skip_to(i + 2, b">");
} else {
// A start tag: find its end outside quotes; `/>` closes it at once.
let mut j = i + 1;
let mut quote = None;
while let Some(&c) = b.get(j) {
match quote {
Some(q) if c == q => quote = None,
Some(_) => {}
None if c == b'"' || c == b'\'' => quote = Some(c),
None if c == b'>' => break,
None => {}
}
j += 1;
}
if b.get(j.wrapping_sub(1)) != Some(&b'/') {
depth += 1;
max = max.max(depth);
}
i = j + 1;
}
}
max
}
/// The deepest array / object nesting of a JSON document (strings skipped).
pub(crate) fn json_depth(b: &[u8]) -> usize {
let (mut depth, mut max, mut in_str, mut esc) = (0usize, 0usize, false, false);
for &c in b {
if in_str {
if esc {
esc = false;
} else if c == b'\\' {
esc = true;
} else if c == b'"' {
in_str = false;
}
continue;
}
match c {
b'"' => in_str = true,
b'[' | b'{' => {
depth += 1;
max = max.max(depth);
}
b']' | b'}' => depth = depth.saturating_sub(1),
_ => {}
}
}
max
}
pub fn import(
project: &mut Project,
bytes: &[u8],
@ -56,6 +133,16 @@ pub fn import(
Some(f) => f,
None => TimelineFormat::detect(bytes, Some(&opts.name)).ok_or(Error::UnknownFormat)?,
};
// The XML and JSON parsers recurse per nesting level: a small file nested thousands of levels
// deep overflowed the stack and aborted the app.
let depth = match format {
TimelineFormat::Fcp7Xml | TimelineFormat::Fcpxml => xml_depth(bytes),
TimelineFormat::Otio => json_depth(bytes),
_ => 0,
};
if depth > MAX_DEPTH {
return Err(Error::TooDeep(MAX_DEPTH));
}
let fc_opts = filmcraft_interchange::ImportOptions {
base_dir: opts.base_dir.clone(),
edl_frame_rate: opts.edl_frame_rate.map(|f| {
@ -434,7 +521,7 @@ impl Importer<'_> {
}
let fit = if ti.scale_to_frame && size.0 > 0 && size.1 > 0 { (comp.width as f64 / size.0 as f64).min(comp.height as f64 / size.1 as f64) } else { 1.0 };
let lc = l.clone();
let tr = l.transform_mut().expect("transform");
let Some(tr) = l.transform_mut() else { return };
// Position / anchor (NaN = centred).
for (src, dst) in [("position", "position"), ("anchor", "anchor")] {
let Some(p) = m.param(src) else { continue };

View File

@ -29,6 +29,8 @@
//! This crate is L3 and does no file I/O: media files are probed through a callback, pre-rendered
//! media come in as paths, and documents are bytes.
#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)]
mod export;
mod import;
#[cfg(test)]
@ -156,6 +158,8 @@ pub enum Error {
UnknownFormat,
#[error("no composition {0}")]
NoComp(u64),
#[error("the timeline document nests more than {0} levels deep")]
TooDeep(usize),
}
pub type Result<T> = std::result::Result<T, Error>;

View File

@ -498,3 +498,42 @@ fn formats_parse() {
assert_eq!(par_fraction(0.9), (9, 10));
assert_eq!(par_fraction(1.0), (1, 1));
}
#[test]
fn export_survives_a_precomp_layer_pointing_at_a_non_comp() {
// A damaged project: the precomp layer's source is a footage item (or a deleted item).
let (mut p, cid) = ae_project();
let footage = p.items.iter().find(|(_, i)| matches!(i.kind, ItemKind::Footage(_))).map(|(id, _)| *id).unwrap();
for bad in [footage, ItemId(9_999)] {
let mut c = (*p.comp_arc(cid).unwrap()).clone();
for l in &mut c.layers {
if matches!(l.source, LayerSource::Comp { .. }) {
l.source = LayerSource::Comp { item: bad };
}
}
p.item_mut(cid).unwrap().kind = ItemKind::Comp(Arc::new(c));
for format in [TimelineFormat::Fcp7Xml, TimelineFormat::Fcpxml, TimelineFormat::Otio] {
let out = export(&p, cid, &ExportOptions { format, prerender: PrerenderMode::None, ..Default::default() }, &HashMap::new()).unwrap();
assert_eq!(out.sequences, 1);
}
}
assert!(matches!(export(&p, footage, &ExportOptions::default(), &HashMap::new()), Err(Error::NoComp(_))));
}
#[test]
fn deeply_nested_documents_error_instead_of_overflowing_the_stack() {
// A few hundred kilobytes nested 100 000 levels deep overflowed the parsers' stack.
let n = 100_000;
let docs = [
(TimelineFormat::Fcp7Xml, format!("<?xml version=\"1.0\"?><xmeml version=\"5\">{}{}</xmeml>", "<a>".repeat(n), "</a>".repeat(n))),
(TimelineFormat::Fcpxml, format!("<?xml version=\"1.0\"?><fcpxml version=\"1.9\">{}{}</fcpxml>", "<a>".repeat(n), "</a>".repeat(n))),
(TimelineFormat::Otio, format!("{{\"OTIO_SCHEMA\":\"Timeline.1\",\"tracks\":{}{}}}", "[".repeat(n), "]".repeat(n))),
];
for (format, doc) in docs {
let mut p = Project::default();
let r = import(&mut p, doc.as_bytes(), Some(format), &ImportOptions::default(), &mut |_| None);
assert!(matches!(r, Err(Error::TooDeep(_))), "{format:?}");
}
assert_eq!(super::import::xml_depth(b"<a><!-- <b><b> --><c x='>'/><![CDATA[<d>]]><e></e></a>"), 2);
assert_eq!(super::import::json_depth(br#"{"a": "[[[{{", "b": [[1], {"c": "\\\"["}]}"#), 3);
}