From 487fad9b28692456a856d4dc6752f5b48237f66f Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 8 Aug 2026 13:53:18 +0200 Subject: [PATCH] fix(components): stop five painters aborting on schema-valid input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each of these renders a scenario that `rustmotion validate` accepts, and each one killed the encode mid-frame. A painter that panics takes the whole render with it, so the guard belongs in the painter rather than upstream. - shape: skia asserts `pos.len() == colors.len()` inside the gradient shader, so a `stops` list of a different length than `colors` aborted the process. Drop stops we cannot honour and let skia space the colours. - table: `"row_colors": []` deserializes to Some(vec![]), not None, so the default palette was never substituted and the modulo guard still indexed an empty slice. - tag_cloud: same shape — `palette()` handed back the caller's empty vec and the painter took `index % len()` on it. - dot_map: `dot_spacing: 0` makes the division +inf, and `inf as u32` saturates to u32::MAX, scheduling ~1.8e19 iterations. The geometry pass rejects 0.01 but not 0, so the floor has to live in the painter. Also caps the grid per axis so a large box cannot schedule unbounded work. - codeblock diff: `col`, `delete` and `insert` all counted bytes while the reveal interpolates a fraction of that total, so mid-animation offsets landed inside a multi-byte glyph and `replace_range` aborted. Switch the accounting to characters and convert to byte offsets only when slicing. This also fixes the animation itself: a CJK glyph used to take three reveal steps instead of one. The new integration test drives all five through the real pipeline — serde, box_builder, run_layout, paint_tree — so a fix that guarded the painter but left the component undeserialisable would still fail. dot_map paints on a worker with a deadline, because a runaway loop would otherwise wedge CI instead of reporting. --- .../src/codeblock/diff.rs | 50 +++-- crates/rustmotion-components/src/dot_map.rs | 17 +- crates/rustmotion-components/src/shape.rs | 10 +- crates/rustmotion-components/src/table.rs | 9 +- crates/rustmotion-components/src/tag_cloud.rs | 9 +- .../tests/degenerate_inputs.rs | 185 ++++++++++++++++++ 6 files changed, 255 insertions(+), 25 deletions(-) create mode 100644 crates/rustmotion-components/tests/degenerate_inputs.rs diff --git a/crates/rustmotion-components/src/codeblock/diff.rs b/crates/rustmotion-components/src/codeblock/diff.rs index 5726c80..864d614 100644 --- a/crates/rustmotion-components/src/codeblock/diff.rs +++ b/crates/rustmotion-components/src/codeblock/diff.rs @@ -444,7 +444,7 @@ pub(super) fn compute_word_diff(old_line: &str, new_line: &str) -> Vec { pending_delete.push_str(change.value()); @@ -456,7 +456,7 @@ pub(super) fn compute_word_diff(old_line: &str, new_line: &str) -> Vec Vec usize { + s.char_indices() + .nth(char_idx) + .map(|(i, _)| i) + .unwrap_or(s.len()) +} + pub(super) fn draw_cursor_edited_line( canvas: &Canvas, font: &Font, @@ -498,7 +511,10 @@ pub(super) fn draw_cursor_edited_line( return; } - let total_work: usize = edits.iter().map(|e| e.delete.len() + e.insert.len()).sum(); + let total_work: usize = edits + .iter() + .map(|e| e.delete.chars().count() + e.insert.chars().count()) + .sum(); if total_work == 0 { let highlighted = highlight_code(new_line, language, theme); if let Some(line) = highlighted.first() { @@ -515,32 +531,33 @@ pub(super) fn draw_cursor_edited_line( for edit in edits { let adjusted_col = (edit.col as i64 + offset_adjust).max(0) as usize; - let delete_len = edit.delete.len(); - let insert_len = edit.insert.len(); + let delete_len = edit.delete.chars().count(); + let insert_len = edit.insert.chars().count(); let edit_work = delete_len + insert_len; if work_done + edit_work <= chars_progress { - let end = (adjusted_col + delete_len).min(current_line.len()); - let start = adjusted_col.min(current_line.len()); - current_line.replace_range(start..end, &edit.insert); + let start = byte_at(¤t_line, adjusted_col); + let end = byte_at(¤t_line, adjusted_col + delete_len); + current_line.replace_range(start..end.max(start), &edit.insert); offset_adjust += insert_len as i64 - delete_len as i64; work_done += edit_work; } else { let remaining_progress = chars_progress - work_done; if remaining_progress < delete_len { let chars_deleted = remaining_progress; - let del_start = (adjusted_col + delete_len - chars_deleted).min(current_line.len()); - let del_end = (adjusted_col + delete_len).min(current_line.len()); + let first_deleted = adjusted_col + delete_len - chars_deleted; + let del_start = byte_at(¤t_line, first_deleted); + let del_end = byte_at(¤t_line, adjusted_col + delete_len); if del_start < del_end { current_line.replace_range(del_start..del_end, ""); } - cursor_col = Some(del_start.min(current_line.len())); + cursor_col = Some(first_deleted); } else { let chars_inserted = remaining_progress - delete_len; - let start = adjusted_col.min(current_line.len()); - let end = (adjusted_col + delete_len).min(current_line.len()); - let partial_insert = &edit.insert[..chars_inserted.min(edit.insert.len())]; - current_line.replace_range(start..end, partial_insert); + let start = byte_at(¤t_line, adjusted_col); + let end = byte_at(¤t_line, adjusted_col + delete_len); + let partial_insert = &edit.insert[..byte_at(&edit.insert, chars_inserted)]; + current_line.replace_range(start..end.max(start), partial_insert); cursor_col = Some(adjusted_col + chars_inserted); } break; @@ -562,7 +579,8 @@ pub(super) fn draw_cursor_edited_line( }; if should_show { - let prefix = ¤t_line[..col.min(current_line.len())]; + // `col` counts characters, like every other column here. + let prefix = ¤t_line[..byte_at(¤t_line, col)]; let (prefix_width, _) = font.measure_str(prefix, None); let cursor_x = x + prefix_width; let mut cursor_paint = paint_from_hex(cursor_color); diff --git a/crates/rustmotion-components/src/dot_map.rs b/crates/rustmotion-components/src/dot_map.rs index ae69d1f..8fa9438 100644 --- a/crates/rustmotion-components/src/dot_map.rs +++ b/crates/rustmotion-components/src/dot_map.rs @@ -154,12 +154,23 @@ impl DotMap { world_paint.set_style(PaintStyle::Fill); world_paint.set_anti_alias(true); - let spacing = self.dot_spacing; + // A spacing at or below zero makes the division +inf, and `inf as u32` + // saturates to u32::MAX in Rust — the nested loop below would then be + // scheduled for ~1.8e19 iterations and never return. The geometry pass + // rejects 0.01 but not 0, so the floor has to live here. + let spacing = if self.dot_spacing.is_finite() && self.dot_spacing >= 1.0 { + self.dot_spacing + } else { + 1.0 + }; let radius = self.dot_radius; let margin = spacing; - let cols = ((w - margin * 2.0) / spacing) as u32; - let rows = ((h - margin * 2.0) / spacing) as u32; + // Belt and braces: even a legal spacing on a very large box should not + // be able to schedule an unbounded amount of work. + const MAX_DOTS_PER_AXIS: u32 = 4096; + let cols = (((w - margin * 2.0) / spacing) as u32).min(MAX_DOTS_PER_AXIS); + let rows = (((h - margin * 2.0) / spacing) as u32).min(MAX_DOTS_PER_AXIS); for row in 0..rows { for col in 0..cols { diff --git a/crates/rustmotion-components/src/shape.rs b/crates/rustmotion-components/src/shape.rs index 5e58a8d..df6ffc1 100644 --- a/crates/rustmotion-components/src/shape.rs +++ b/crates/rustmotion-components/src/shape.rs @@ -61,7 +61,15 @@ impl Painter for Shape { .iter() .map(|c| color4f_from_hex(c)) .collect(); - let stops: Option> = gradient.stops.clone(); + // skia asserts `pos.len() == colors.len()` inside the gradient + // shader — a mismatch aborts the process instead of erroring. + // Nothing upstream enforces the pairing, so drop stops we + // cannot honour and let skia distribute the colours evenly. + let stops: Option> = gradient + .stops + .as_ref() + .filter(|s| s.len() == colors.len()) + .cloned(); let mut paint = Paint::default(); paint.set_anti_alias(true); diff --git a/crates/rustmotion-components/src/table.rs b/crates/rustmotion-components/src/table.rs index 806eb99..0c0bfab 100644 --- a/crates/rustmotion-components/src/table.rs +++ b/crates/rustmotion-components/src/table.rs @@ -146,7 +146,14 @@ impl Table { let text_color = self.style.color_str_or("#FFFFFF"); let header_text_color = self.header_text_color.as_deref().unwrap_or("#FFFFFF"); let default_row_colors = vec!["#1F2937".to_string(), "#111827".to_string()]; - let row_colors = self.row_colors.as_ref().unwrap_or(&default_row_colors); + // `"row_colors": []` deserializes to Some(vec![]), not None — a generator + // writes it to mean "no striping". Row painting indexes this slice, so an + // empty one has to fall back rather than reach the painter. + let row_colors = self + .row_colors + .as_ref() + .filter(|c| !c.is_empty()) + .unwrap_or(&default_row_colors); // Resolve fonts before the optional clip below so an early return on // font failure keeps canvas save/restore balanced. diff --git a/crates/rustmotion-components/src/tag_cloud.rs b/crates/rustmotion-components/src/tag_cloud.rs index eedad5a..93fdf21 100644 --- a/crates/rustmotion-components/src/tag_cloud.rs +++ b/crates/rustmotion-components/src/tag_cloud.rs @@ -79,11 +79,12 @@ impl TagCloud { 1.0 - (1.0 - p).powi(3) } + /// Never empty: callers index into it with `%`, and `"colors": []` — which a + /// generator emits to mean "no custom palette" — otherwise divides by zero. fn palette(&self) -> Vec<&str> { - if let Some(colors) = &self.colors { - colors.iter().map(|s| s.as_str()).collect() - } else { - DEFAULT_PALETTE.to_vec() + match &self.colors { + Some(colors) if !colors.is_empty() => colors.iter().map(|s| s.as_str()).collect(), + _ => DEFAULT_PALETTE.to_vec(), } } diff --git a/crates/rustmotion-components/tests/degenerate_inputs.rs b/crates/rustmotion-components/tests/degenerate_inputs.rs new file mode 100644 index 0000000..8b609f3 --- /dev/null +++ b/crates/rustmotion-components/tests/degenerate_inputs.rs @@ -0,0 +1,185 @@ +//! A painter must never panic — nor hang — on input the schema accepts. +//! +//! `rustmotion validate` is the documented gate before delivery, and it answers +//! "Valid scenario" for every component below. Each one was observed aborting or +//! wedging the renderer mid-frame, which kills the whole encode: the scenarios +//! here are the reproductions, verbatim, kept as a regression floor. +//! +//! Every case routes through the real pipeline — serde, box_builder, run_layout, +//! paint_tree — so a fix that only guards the painter while leaving the component +//! undeserialisable would still fail. + +use std::sync::mpsc; +use std::time::Duration; + +use rustmotion_components::box_builder::{build_scene_with_anim, BuildAnimationCtx}; +use rustmotion_components::legacy_dispatch::LegacyPaintDispatcher; +use rustmotion_components::{ChildComponent, Component, PositionMode}; +use rustmotion_core::css::taffy_bridge::ConversionContext; +use rustmotion_core::engine::layout_pass::run_layout; +use rustmotion_core::engine::paint_pass::{paint_tree, PaintFrame}; + +const W: u32 = 400; +const H: u32 = 300; + +/// Deserialize one component and paint it at `time`. Panics propagate — that is +/// the point of the test. +fn paint(json: serde_json::Value, time: f64) { + let component: Component = serde_json::from_value(json).expect("component is schema-valid"); + let children = vec![ChildComponent { + component, + position: Some(PositionMode::Absolute { x: 0.0, y: 0.0 }), + x: None, + y: None, + z_index: None, + bleed: false, + }]; + + let mut surface = + skia_safe::surfaces::raster_n32_premul((W as i32, H as i32)).expect("raster surface"); + let canvas = surface.canvas(); + canvas.clear(skia_safe::Color4f::new(0.0, 0.0, 0.0, 0.0)); + + let built = build_scene_with_anim( + &children, + (W as f32, H as f32), + BuildAnimationCtx { + time, + scene_duration: 2.0, + fps: 30, + }, + ); + let layout = run_layout( + &built.root, + (W as f32, H as f32), + &ConversionContext::default(), + ); + let dispatcher = LegacyPaintDispatcher::for_scene(&built); + let frame = PaintFrame { + time, + frame_index: (time * 30.0) as u32, + fps: 30, + video_width: W, + video_height: H, + scene_duration: 2.0, + camera: None, + }; + paint_tree(canvas, &built.root, &layout, &frame, &dispatcher); +} + +/// Paint on a worker so a runaway loop fails the test instead of wedging the +/// suite. `dot_spacing: 0` used to spin for billions of iterations; a plain +/// `paint()` call would hang CI rather than report. +fn paint_within(json: serde_json::Value, time: f64, budget: Duration, what: &str) { + let (tx, rx) = mpsc::channel(); + let worker = std::thread::spawn(move || { + paint(json, time); + let _ = tx.send(()); + }); + match rx.recv_timeout(budget) { + Ok(()) => worker.join().expect(what), + Err(_) => panic!("{what}: still painting after {budget:?} — runaway loop"), + } +} + +#[test] +fn shape_survives_a_gradient_whose_stops_do_not_match_its_colors() { + // skia asserts pos.len() == colors.len() inside the gradient shader, so a + // mismatched `stops` aborted the process rather than returning an error. + for gradient_type in ["linear", "radial"] { + paint( + serde_json::json!({ + "type": "shape", + "shape": "rect", + "style": { "width": 200, "height": 100 }, + "fill": { + "type": gradient_type, + "colors": ["#FF0000", "#0000FF"], + "stops": [0.0, 0.5, 1.0] + } + }), + 0.5, + ); + } +} + +#[test] +fn table_survives_an_empty_row_colors_list() { + // `row_colors: []` deserializes to Some(vec![]), so the default palette was + // never substituted and the modulo guard still indexed an empty slice. + paint( + serde_json::json!({ + "type": "table", + "headers": ["A", "B"], + "rows": [["1", "2"], ["3", "4"]], + "row_colors": [], + "style": { "width": 300, "height": 200 } + }), + 0.5, + ); +} + +#[test] +fn tag_cloud_survives_an_empty_colors_list() { + // palette() returned the caller's empty vec, and the painter took + // `index % palette.len()` on it. + paint( + serde_json::json!({ + "type": "tag_cloud", + "tags": [{ "text": "rust", "weight": 3 }, { "text": "skia", "weight": 1 }], + "colors": [], + "style": { "width": 300, "height": 200 } + }), + 0.5, + ); +} + +#[test] +fn dot_map_terminates_on_a_zero_dot_spacing() { + // (w - 0) / 0 is +inf, and `inf as u32` saturates to u32::MAX in Rust, so the + // nested loop was scheduled for ~1.8e19 iterations. The geometry pass catches + // dot_spacing: 0.01 but not 0. + paint_within( + serde_json::json!({ + "type": "dot_map", + "points": [{ "lat": 48.8, "lng": 2.3 }], + "dot_spacing": 0, + "style": { "width": 300, "height": 150 } + }), + 0.5, + Duration::from_secs(10), + "dot_map with dot_spacing: 0", + ); +} + +#[test] +fn codeblock_diff_survives_multibyte_text() { + // The edit script counts bytes while the reveal interpolates a fraction of + // that count, so mid-animation offsets landed inside a multi-byte character + // and `replace_range` aborted with "not a char boundary". + for (from, to) in [ + ("let a = 1;", "let café = «héllo→»;"), + ("let x = 1;", "let y = \"éàü\";"), + ("a", "日本語のテキスト"), + ("ok", "🎬 clap"), + ] { + // Sweep the reveal: the panic only fires on the frames where progress + // lands part-way through a glyph. + for step in 0..=20 { + paint( + serde_json::json!({ + "type": "codeblock", + "code": from, + "language": "rust", + "diff": true, + "states": [ + { "code": from, "at": 0.0 }, + { "code": to, "at": 0.4, "cursor": { "enabled": true } } + ], + "style": { "width": 380, "height": 120 } + }), + f64::from(step) * 0.1, + ); + } + } +}