From 9f78875338ea2fda13983cd7d1f73d4b3f0b8c74 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 21:17:36 -0400 Subject: [PATCH] security: linear vector import, stroking and rounding, validation, and cyclic dev snapshots (#1306) * security: linear vector import, stroking and rounding, validation and a cycle check for dev snapshots Closing a subpath, rounding corners and chaining stroke runs each rescanned the whole network per step, and Document::validate searched a parent's children per child. They now update only the current subpath's last segment, rebuild each region loop in one pass, walk per-vertex adjacency lists, and check edges against a once-built index. A dev snapshot whose layers contain a cycle under the root is refused instead of reconstructed. Co-Authored-By: Claude Sonnet 5.5 * fix(doc): address review findings for snapshot children and 2-segment rounded loops --------- Co-authored-by: Claude Sonnet 5.5 --- crates/doc/src/handoff.rs | 30 +++++++++++ crates/doc/src/handoff_tests.rs | 51 ++++++++++++++++++ crates/doc/src/lib.rs | 8 ++- crates/doc/src/tests.rs | 57 ++++++++++++++++++++ crates/doc/src/vector.rs | 94 ++++++++++++++++++++++----------- 5 files changed, 208 insertions(+), 32 deletions(-) diff --git a/crates/doc/src/handoff.rs b/crates/doc/src/handoff.rs index 9700b56c..04bf5e95 100644 --- a/crates/doc/src/handoff.rs +++ b/crates/doc/src/handoff.rs @@ -71,8 +71,38 @@ impl Document { if let (Some(now), Some(r)) = (self.node(root), d.nodes.get_mut(&root)) { Arc::make_mut(r).parent = now.parent; } + // A damaged file can give a historical layer itself (or an ancestor) as a child; a copy + // with a cycle under the root would send every walk of it round forever. + if !d.is_tree_under(root) { + return None; + } d.dev_snapshots.clear(); d.dirty = crate::Dirty::default(); Some(d) } + + /// Whether every layer under `root` is reached once (no layer is its own descendant or listed + /// by two parents), walking iteratively and stopping at the first repeat. + fn is_tree_under(&self, root: NodeId) -> bool { + let mut seen = BTreeSet::new(); + let mut stack = vec![root]; + while let Some(id) = stack.pop() { + if !seen.insert(id) { + return false; + } + let Some(n) = self.nodes.get(&id) else { + return false; + }; + for &c in &n.children { + let Some(child) = self.nodes.get(&c) else { + return false; + }; + if child.parent != Some(id) { + return false; + } + stack.push(c); + } + } + true + } } diff --git a/crates/doc/src/handoff_tests.rs b/crates/doc/src/handoff_tests.rs index 7fc26898..79b97194 100644 --- a/crates/doc/src/handoff_tests.rs +++ b/crates/doc/src/handoff_tests.rs @@ -34,3 +34,54 @@ fn a_snapshot_shows_the_layer_as_it_was() { doc.dev_snapshots.clear(); assert!(serde_json::to_value(&doc).unwrap().get("dev_snapshots").is_none()); } + +#[test] +fn a_snapshot_with_a_layer_inside_itself_is_refused() { + let mut doc = Document::new("d"); + let page = doc.first_page().unwrap(); + let f = doc.create(page, NodeKind::Frame, "Screen", 0.0, 0.0, 100.0, 100.0).unwrap(); + let mut snap = doc.snapshot_subtree(f).unwrap(); + // A damaged file: the historical frame lists itself as a child. + let mut bad = (*snap.nodes[0]).clone(); + bad.children.push(f); + snap.nodes[0] = Arc::new(bad); + doc.dev_snapshots.insert(f, Arc::new(snap)); + assert!(doc.with_dev_snapshot(f).is_none()); +} + +#[test] +fn a_snapshot_with_missing_or_nonreciprocal_child_is_refused() { + use crate::NodeId; + let mut doc = Document::new("d"); + let page = doc.first_page().unwrap(); + let f = doc.create(page, NodeKind::Frame, "Screen", 0.0, 0.0, 100.0, 100.0).unwrap(); + let _c1 = doc.create(f, NodeKind::Rectangle, "R1", 0.0, 0.0, 10.0, 10.0).unwrap(); + let snap = doc.snapshot_subtree(f).unwrap(); + + let mut missing = snap.clone(); + let mut bad_root = (*missing.nodes[0]).clone(); + bad_root.children.push(NodeId(99999)); + missing.nodes[0] = Arc::new(bad_root); + doc.dev_snapshots.insert(f, Arc::new(missing)); + assert!(doc.with_dev_snapshot(f).is_none()); + + let mut nonrecip = snap.clone(); + let mut bad_child = (*nonrecip.nodes[1]).clone(); + bad_child.parent = Some(NodeId(88888)); + nonrecip.nodes[1] = Arc::new(bad_child); + doc.dev_snapshots.insert(f, Arc::new(nonrecip)); + assert!(doc.with_dev_snapshot(f).is_none()); +} + +#[test] +fn validate_is_linear_in_a_wide_parent() { + let mut doc = Document::new("d"); + let page = doc.first_page().unwrap(); + for i in 0..5_000 { + doc.create(page, NodeKind::Rectangle, "R", f64::from(i), 0.0, 1.0, 1.0).unwrap(); + } + assert!(doc.validate().is_empty()); + let kid = doc.node(page).unwrap().children[0]; + doc.get_mut(page).unwrap().children.retain(|c| *c != kid); + assert!(doc.validate().iter().any(|m| m.contains("not listed in parent"))); +} diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index 70797dd8..9ff1d968 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -1023,6 +1023,10 @@ impl Document { } } let mut issues = Vec::new(); + // Every (parent, child) edge and the page list, indexed once so each node's checks are + // lookups: a wide parent or a long page list doesn't make validation quadratic. + let edges: std::collections::BTreeSet<(NodeId, NodeId)> = self.nodes.iter().flat_map(|(p, n)| n.children.iter().map(|c| (*p, *c))).collect(); + let page_ids: std::collections::BTreeSet = self.pages.iter().copied().collect(); for (id, n) in self.nodes.iter() { if n.id != *id { note(&mut issues, || format!("node key {id} has id {}", n.id)); @@ -1039,12 +1043,12 @@ impl Document { } } if let Some(p) = n.parent - && !self.node(p).is_some_and(|pn| pn.children.contains(id)) + && !(self.node(p).is_some() && edges.contains(&(p, *id))) { note(&mut issues, || format!("{id} not listed in parent {p}")); } // The library store's root is a page that is never listed. - let listed = self.pages.contains(id) || (n.is_page() && self.libraries.store == Some(*id)); + let listed = page_ids.contains(id) || (n.is_page() && self.libraries.store == Some(*id)); if n.is_page() != listed { note(&mut issues, || format!("page list mismatch for {id}")); } diff --git a/crates/doc/src/tests.rs b/crates/doc/src/tests.rs index 1a006c26..928dca55 100644 --- a/crates/doc/src/tests.rs +++ b/crates/doc/src/tests.rs @@ -559,3 +559,60 @@ fn is_ancestor_holds_for_very_deep_chains() { assert!(d.is_ancestor(root, leaf)); assert!(matches!(d.reparent(root, leaf, None), Err(DocError::Cycle(..)))); } + +#[test] +fn many_small_vector_subpaths_build_and_stroke_without_rescans() { + // Many closed subpaths whose last point repeats the start (#F-332), a long open chain (#F-334) + // and a big rounded polygon (#F-333): each used to scan the whole network per step. + let mut p = figcraft_geom::kurbo::BezPath::new(); + for i in 0..3_000 { + let x = f64::from(i) * 10.0; + p.move_to((x, 0.0)); + p.line_to((x + 5.0, 0.0)); + p.line_to((x + 5.0, 5.0)); + p.line_to((x, 0.0)); + p.close_path(); + } + let net = VectorNetwork::from_path(&p); + assert_eq!(net.vertices.len(), 3 * 3_000); + assert_eq!(net.regions[0].loops.len(), 3_000); + assert!(net.segments.iter().all(|s| s.a < net.vertices.len() && s.b < net.vertices.len())); + assert_eq!(net.stroke_runs().len(), 3_000); + + let pts: Vec = (0..20_000).map(|i| Point::new(f64::from(i), f64::from(i % 7))).collect(); + let chain = VectorNetwork::from_points(&pts, false); + let runs = chain.stroke_runs(); + assert_eq!(runs.len(), 1); + assert!(!runs[0].closed); + + let ring: Vec = (0..4_000) + .map(|i| { + let t = f64::from(i) / 4_000.0 * std::f64::consts::TAU; + Point::new(1000.0 * t.cos(), 1000.0 * t.sin() * if i % 2 == 0 { 1.0 } else { 0.9 }) + }) + .collect(); + let mut poly = VectorNetwork::from_points(&ring, true); + for v in &mut poly.vertices { + v.radius = 0.5; + } + let rounded = poly.rounded().unwrap(); + assert!(rounded.regions[0].loops[0].len() > poly.segments.len(), "arcs are walked between the sides"); + assert_eq!(rounded.regions[0].loops[0].len(), rounded.segments.len()); +} + +#[test] +fn two_segment_closed_loop_rounded_corners() { + let mut v0 = Vertex::new(0.0, 0.0); + v0.radius = 2.0; + let mut v1 = Vertex::new(10.0, 0.0); + v1.radius = 2.0; + let net = VectorNetwork { + vertices: vec![v0, v1], + segments: vec![Segment { a: 0, b: 1, ta: (0.0, 0.0), tb: (0.0, 0.0) }, Segment { a: 1, b: 0, ta: (0.0, 5.0), tb: (0.0, -5.0) }], + regions: vec![Region { loops: vec![vec![0, 1]], winding: WindingRule::NonZero }], + }; + let r = net.rounded().unwrap(); + assert_eq!(r.regions[0].loops[0].len(), 4, "both arcs are in the loop"); + let path = net.fill_path(); + assert!(!path.elements().is_empty()); +} diff --git a/crates/doc/src/vector.rs b/crates/doc/src/vector.rs index 1165bd60..b3040b1e 100644 --- a/crates/doc/src/vector.rs +++ b/crates/doc/src/vector.rs @@ -134,6 +134,16 @@ pub struct VectorNetwork { pub regions: Vec, } +/// Point the subpath's last segment (the only one ending at the fresh vertex `from`) at `to`, +/// so merging a duplicate endpoint doesn't rescan the segments of earlier subpaths. +fn redirect_last_segment(net: &mut VectorNetwork, loop_segs: &[usize], from: usize, to: usize) { + if let Some(s) = loop_segs.last().and_then(|i| net.segments.get_mut(*i)) + && s.b == from + { + s.b = to; + } +} + impl VectorNetwork { /// A network from a polyline / polygon. pub fn from_points(points: &[Point], closed: bool) -> Self { @@ -169,11 +179,7 @@ impl VectorNetwork { if a == f || loop_segs.len() < 2 || (pa - pf).hypot() > 1e-6 || a + 1 != net.vertices.len() { return; } - for s in net.segments.iter_mut() { - if s.b == a { - s.b = f; - } - } + redirect_last_segment(net, loop_segs, a, f); net.vertices.pop(); all_loops.push(std::mem::take(loop_segs)); }; @@ -219,11 +225,7 @@ impl VectorNetwork { let same = matches!((close_pt, first_pt), (Some(c), Some(f)) if (c - f).hypot() <= 1e-6); if same && a != f { // Merge the duplicate end vertex into the first one. - for s in net.segments.iter_mut() { - if s.b == a { - s.b = f; - } - } + redirect_last_segment(&mut net, &loop_segs, a, f); if a + 1 == net.vertices.len() { net.vertices.pop(); } @@ -385,6 +387,7 @@ impl VectorNetwork { s.ta = (q.p1.x - q.p0.x, q.p1.y - q.p0.y); s.tb = (q.p2.x - q.p3.x, q.p2.y - q.p3.y); } + let mut by_vertex: std::collections::BTreeMap = std::collections::BTreeMap::new(); for c in corners { if let Some(v) = out.vertices.get_mut(c.v) { (v.x, v.y) = (c.a.x, c.a.y); @@ -400,15 +403,28 @@ impl VectorNetwork { } out.segments.push(Segment { a: c.v, b: nb, ta: c.ta, tb: c.tb }); let arc = out.segments.len() - 1; - // The arc goes between the two segments wherever a loop walks through the point. - for lp in out.regions.iter_mut().flat_map(|r| r.loops.iter_mut()) { - let n = lp.len(); - let between = - |k: usize| matches!((lp.get(k), lp.get((k + 1) % n)), (Some(&x), Some(&y)) if (x, y) == (c.s1, c.s2) || (x, y) == (c.s2, c.s1)); - if let Some(k) = (0..n).find(|k| between(*k)) { - lp.insert(k + 1, arc); + by_vertex.insert(c.v, arc); + } + // The arc goes between the two segments wherever a loop walks through the point. Each loop + // is rebuilt in one pass, so a polygon with many rounded corners isn't quadratic. + for lp in out.regions.iter_mut().flat_map(|r| r.loops.iter_mut()) { + let Some(&first_idx) = lp.first() else { continue }; + let start_v = match (self.segments.get(first_idx), lp.get(1).and_then(|&i| self.segments.get(i))) { + (Some(first), Some(second)) if first.a == second.a || first.a == second.b => first.b, + (Some(first), _) => first.a, + _ => 0, + }; + let mut rebuilt = Vec::with_capacity(lp.len() + by_vertex.len().min(lp.len())); + let mut cur = start_v; + for &x in lp.iter() { + rebuilt.push(x); + let Some(s) = self.segments.get(x) else { continue }; + cur = if s.a == cur { s.b } else { s.a }; + if let Some(&arc) = by_vertex.get(&cur) { + rebuilt.push(arc); } } + *lp = rebuilt; } Some(out) } @@ -547,25 +563,40 @@ impl VectorNetwork { } } let degree = |v: usize| degrees.get(v).copied().unwrap_or(0); - let next_from = |v: usize, used: &[bool]| -> Option<(usize, bool)> { - self.segments.iter().enumerate().find_map(|(i, s)| { - if used.get(i).copied().unwrap_or(true) { - return None; + // Each vertex's segments in segment order, and how far down that list the used ones reach, + // so finding the next unused segment never rescans the whole network. + let mut adjacent: Vec> = vec![Vec::new(); self.vertices.len()]; + for (i, s) in self.segments.iter().enumerate() { + if let Some(l) = adjacent.get_mut(s.a) { + l.push((i, false)); + } + if s.b != s.a + && let Some(l) = adjacent.get_mut(s.b) + { + l.push((i, true)); + } + } + let mut cursor = vec![0usize; self.vertices.len()]; + let mut remaining = self.segments.len(); + let mut next_from = |v: usize, used: &[bool]| -> Option<(usize, bool)> { + let list = adjacent.get(v)?; + let at = cursor.get_mut(v)?; + while let Some(&(i, rev)) = list.get(*at) { + if !used.get(i).copied().unwrap_or(true) { + return Some((i, rev)); } - if s.a == v { - Some((i, false)) - } else if s.b == v { - Some((i, true)) - } else { - None - } - }) + *at += 1; + } + None }; let stop = |v: usize| degree(v) != 2 || breaks(v); // Start runs at endpoints, junctions and breaks first, then whatever remains (pure loops). let mut starts: Vec = (0..self.vertices.len()).filter(|v| stop(*v)).collect(); starts.extend((0..self.vertices.len()).filter(|v| !stop(*v))); for v0 in starts { + if remaining == 0 { + break; + } while let Some((si, rev)) = next_from(v0, &used) { let Some(start) = self.vertices.get(v0) else { break }; let mut path = BezPath::new(); @@ -574,8 +605,11 @@ impl VectorNetwork { let mut closed = false; let mut seg = Some((si, rev)); while let Some((i, r)) = seg { - if let Some(u) = used.get_mut(i) { + if let Some(u) = used.get_mut(i) + && !*u + { *u = true; + remaining = remaining.saturating_sub(1); } let Some(s) = self.segments.get(i) else { break }; self.push_seg(&mut path, s, r);