From 575e690cc51b2c132dfbfb5bc59ad090a146a01c Mon Sep 17 00:00:00 2001 From: Valentin Klein Date: Sat, 10 Oct 2026 21:33:37 +0200 Subject: [PATCH] Bound block expansion in drawing and extents: nested, self-referencing or arrayed blocks no longer hang (fixes #255) (#267) * Bound block expansion in drawing and extents (fixes #255) MAX_BLOCK_DEPTH bounds only the depth of nested block references, so a small DXF with a self-referencing block, blocks that each insert the next several times, or one large MINSERT expanded to billions of copies and hung the app (drawing, PNG/SVG/PDF export, extents, saving). - render: a per-display-list budget (MAX_BLOCK_EXPANSION, 2 million block copies and entities drawn inside blocks); past it the remaining block contents are not drawn. - doc: entity_bounds computes each block's content bounds once per call and reuses them for every further reference. Co-Authored-By: Claude Opus 5.5 * Move Builder::expand below the drawing primitives Pure code motion so this branch merges cleanly with fix/272-aci7-only-flips (#272), which changes the Builder::polyline signature next to it. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- crates/doc/src/extents.rs | 27 ++++++++++--- crates/io/tests/block_expansion.rs | 65 ++++++++++++++++++++++++++++++ crates/render/src/lib.rs | 28 ++++++++++++- 3 files changed, 114 insertions(+), 6 deletions(-) create mode 100644 crates/io/tests/block_expansion.rs diff --git a/crates/doc/src/extents.rs b/crates/doc/src/extents.rs index 41f25d0b4..dc89e6185 100644 --- a/crates/doc/src/extents.rs +++ b/crates/doc/src/extents.rs @@ -1,5 +1,7 @@ //! Approximate entity bounds (used for extents, zoom and selection pre-filtering). +use std::collections::HashMap; + use cadcraft_geom::{Arc, Bounds2, EPS, Polyline, Vec2, bulge_to_arc}; use crate::{Drawing, Entity, EntityKind, LwPolyline, Prim}; @@ -33,6 +35,14 @@ fn text_box(t: &crate::Text, len: usize) -> Bounds2 { } pub fn entity_bounds(d: &Drawing, e: &Entity, depth: usize) -> Bounds2 { + bounds_in(d, e, depth, &mut HashMap::new()) +} + +/// [`entity_bounds`] with the bounds of block contents (in block coordinates) already computed, +/// by block name: every further reference to a block reuses them instead of expanding the block +/// again, so nested or self-referencing blocks cost one pass over each definition instead of one +/// per reference (which grows exponentially with the nesting depth). +fn bounds_in(d: &Drawing, e: &Entity, depth: usize, blocks: &mut HashMap) -> Bounds2 { match &e.kind { EntityKind::Text(t) => text_box(t, t.value.chars().count()), EntityKind::AttDef(a) => text_box(&a.text, a.tag.chars().count()), @@ -62,10 +72,17 @@ pub fn entity_bounds(d: &Drawing, e: &Entity, depth: usize) -> Bounds2 { } let Some(blk) = d.block(&ins.block) else { return Bounds2::from_points([ins.insert.xy()]) }; let m = ins.transform(blk.base.xy()); - let mut inner = Bounds2::EMPTY; - for be in blk.entities.iter() { - inner = inner.union(&entity_bounds(d, be, depth + 1)); - } + let inner = match blocks.get(&blk.name) { + Some(b) => *b, + None => { + let mut inner = Bounds2::EMPTY; + for be in blk.entities.iter() { + inner = inner.union(&bounds_in(d, be, depth + 1, blocks)); + } + blocks.insert(blk.name.clone(), inner); + inner + } + }; let mut b = if inner.is_empty() { Bounds2::from_points([ins.insert.xy()]) } else { Bounds2::from_points(inner.corners().map(|c| m.apply(c))) }; if ins.cols > 1 || ins.rows > 1 { @@ -106,7 +123,7 @@ pub fn entity_bounds(d: &Drawing, e: &Entity, depth: usize) -> Bounds2 { && depth < MAX_BLOCK_DEPTH { for be in blk.entities.iter() { - b = b.union(&entity_bounds(d, be, depth + 1)); + b = b.union(&bounds_in(d, be, depth + 1, blocks)); } } b diff --git a/crates/io/tests/block_expansion.rs b/crates/io/tests/block_expansion.rs new file mode 100644 index 000000000..b190156f8 --- /dev/null +++ b/crates/io/tests/block_expansion.rs @@ -0,0 +1,65 @@ +//! Nested, self-referencing and arrayed block references in a hostile DXF must not hang drawing +//! or extents (issue #255): the expansion is bounded, not only its depth. + +use cadcraft_doc::{Drawing, EntityKind, Space}; +use cadcraft_render::{MAX_BLOCK_EXPANSION, Options, build}; + +fn read(text: &str) -> Drawing { + cadcraft_io::read(text.as_bytes(), "x.dxf").unwrap() +} + +/// Block `A` holds a line and `refs` references to itself; model space holds one `A`. +fn self_referencing(refs: usize) -> String { + let mut s = String::from("0\nSECTION\n2\nBLOCKS\n0\nBLOCK\n2\nA\n0\nLINE\n10\n0\n20\n0\n11\n1\n21\n1\n"); + for k in 1..=refs { + s += &format!("0\nINSERT\n2\nA\n10\n{k}\n20\n0\n"); + } + s + "0\nENDBLK\n0\nENDSEC\n0\nSECTION\n2\nENTITIES\n0\nINSERT\n2\nA\n10\n0\n20\n0\n0\nENDSEC\n0\nEOF\n" +} + +/// Blocks `F0`..`F14`, each holding four references to the next; `F15` holds a line (drawn +/// 4^15 times: its references sit exactly at `MAX_BLOCK_DEPTH`). +fn fan_out() -> String { + let mut s = String::from("0\nSECTION\n2\nBLOCKS\n"); + for k in 0..15 { + s += &format!("0\nBLOCK\n2\nF{k}\n"); + for x in 0..4 { + s += &format!("0\nINSERT\n2\nF{}\n10\n{x}\n20\n0\n", k + 1); + } + s += "0\nENDBLK\n"; + } + s + "0\nBLOCK\n2\nF15\n0\nLINE\n10\n0\n20\n0\n11\n1\n21\n1\n0\nENDBLK\n0\nENDSEC\n0\nSECTION\n2\nENTITIES\n0\nINSERT\n2\nF0\n10\n0\n20\n0\n0\nENDSEC\n0\nEOF\n" +} + +/// One MINSERT of a one-line block with `cols` x `rows` copies 2 units apart. +fn minsert(cols: u32, rows: u32) -> String { + format!( + "0\nSECTION\n2\nBLOCKS\n0\nBLOCK\n2\nA\n0\nLINE\n10\n0\n20\n0\n11\n1\n21\n1\n0\nENDBLK\n0\nENDSEC\n0\nSECTION\n2\nENTITIES\n0\nINSERT\n2\nA\n10\n0\n20\n0\n70\n{cols}\n71\n{rows}\n44\n2\n45\n2\n0\nENDSEC\n0\nEOF\n" + ) +} + +#[test] +fn block_expansion_bombs_are_bounded() { + for text in [self_referencing(4), self_referencing(12), fan_out(), minsert(10_000, 10_000)] { + let d = read(&text); + let ext = d.extents(&Space::Model); + assert!(!ext.is_empty() && ext.min.is_finite() && ext.max.is_finite(), "{ext:?}"); + let list = build(&d, &Space::Model, &Options::default()); + assert!(!list.prims.is_empty() && list.prims.len() <= MAX_BLOCK_EXPANSION, "{}", list.prims.len()); + // Saving computes the extents again ($EXTMIN/$EXTMAX). + assert!(cadcraft_io::write_dxf(&d).contains("$EXTMAX")); + } +} + +#[test] +fn ordinary_nested_and_arrayed_blocks_are_drawn_whole() { + // 3 x 2 copies of a one-line block: six lines, extents spanning all copies. + let d = read(&minsert(3, 2)); + assert_eq!(build(&d, &Space::Model, &Options::default()).prims.len(), 6); + let ext = d.extents(&Space::Model); + assert_eq!((ext.min.x, ext.min.y, ext.max.x, ext.max.y), (0.0, 0.0, 5.0, 3.0)); + // A self-referencing block draws its line at every level down to MAX_BLOCK_DEPTH. + let d = read(&self_referencing(1)); + assert!(matches!(d.model.iter().next().map(|e| &e.kind), Some(EntityKind::Insert(_)))); + assert_eq!(build(&d, &Space::Model, &Options::default()).prims.len(), cadcraft_doc::MAX_BLOCK_DEPTH); +} diff --git a/crates/render/src/lib.rs b/crates/render/src/lib.rs index 4f8f73ee5..1b4db7b1b 100644 --- a/crates/render/src/lib.rs +++ b/crates/render/src/lib.rs @@ -61,6 +61,8 @@ pub struct DisplayList { /// The paper sheet when the list was built for a layout (paper space); the UI draws the /// white sheet, its shadow and the printable-area outline from it. pub sheet: Option, + /// Block contents expanded while building (see [`MAX_BLOCK_EXPANSION`]). + pub expanded: usize, } impl DisplayList { @@ -130,6 +132,12 @@ struct Builder<'a> { plotting: bool, } +/// Upper bound on the block contents drawn per display list (each block reference, MINSERT copy +/// and entity drawn inside a block counts one). Nested, self-referencing or arrayed block +/// references in a hostile file would otherwise expand exponentially (`MAX_BLOCK_DEPTH` only +/// bounds the depth); past the limit the remaining block contents are not drawn. +pub const MAX_BLOCK_EXPANSION: usize = 2_000_000; + impl Builder<'_> { fn polyline(&mut self, ctx: &Ctx, color: Rgb, lw: f32, pts: &[Vec2]) { if pts.len() < 2 { @@ -188,6 +196,11 @@ impl Builder<'_> { self.list.verts.push(ctx.xf.apply_vec(dir).normalized()); self.list.prims.push(DPrim { handle: ctx.top, color, lw: 0.0, kind: Kind::Infinite { ray }, start, len: 2 }); } + /// Take one unit of the block-expansion budget; false once it is spent. + fn expand(&mut self) -> bool { + self.list.expanded = self.list.expanded.saturating_add(1); + self.list.expanded <= MAX_BLOCK_EXPANSION + } } /// Build the display list for a space. For a layout (paper space) this includes the model-space @@ -249,6 +262,9 @@ fn viewport(b: &mut Builder, d: &Drawing, e: &Entity, vp: &cadcraft_doc::Viewpor } // Border (on the viewport's layer; layer off hides only the border). entity(b, &top_ctx(d, Mat3::IDENTITY, e.handle, &[], &[]), e); + if b.list.expanded >= MAX_BLOCK_EXPANSION { + return; + } let center = vp.center.xy(); let ok = |v: f64| v.is_finite() && v > 0.0; if !(ok(vp.width) && ok(vp.height) && ok(vp.view_height) && center.is_finite() && vp.view_center.is_finite()) { @@ -277,6 +293,7 @@ fn viewport(b: &mut Builder, d: &Drawing, e: &Entity, vp: &cadcraft_doc::Viewpor } entity(&mut sub, &top_ctx(d, xf, VIEWPORT_CONTENT, &vp.frozen_layers, &vp.layer_colors), me); } + b.list.expanded = b.list.expanded.saturating_add(sub.list.expanded); append_clipped(&mut b.list, &sub.list, &rect); } @@ -420,6 +437,9 @@ fn entity(b: &mut Builder, ctx: &Ctx, e: &Entity) { { let sub = sub_ctx(ctx, e, Mat3::IDENTITY); for be in blk.entities.iter() { + if !b.expand() { + break; + } entity(b, &sub, be); } } else { @@ -800,8 +820,11 @@ fn insert(b: &mut Builder, ctx: &Ctx, e: &Entity, ins: &cadcraft_doc::Insert, rg if let Some(blk) = ctx.d.block(&ins.block) { let cols = ins.cols.clamp(1, 10_000); let rows = ins.rows.clamp(1, 10_000); - for r in 0..rows { + 'copies: for r in 0..rows { for c in 0..cols { + if !b.expand() { + break 'copies; + } let off = Vec2::new(ins.col_spacing * f64::from(c), ins.row_spacing * f64::from(r)).rotate(ins.rotation); let m = Mat3::translate(off).then_before(ins.transform(blk.base.xy())); let sub = sub_ctx(ctx, e, m); @@ -810,6 +833,9 @@ fn insert(b: &mut Builder, ctx: &Ctx, e: &Entity, ins: &cadcraft_doc::Insert, rg if matches!(be.kind, EntityKind::AttDef(_)) { continue; } + if !b.expand() { + break 'copies; + } entity(b, &sub, be); } }