From 38aa2ee34d28d6e946a7203bb5813f18443412ea Mon Sep 17 00:00:00 2001 From: Christopher Overbeck Date: Thu, 17 Sep 2026 09:57:36 -0500 Subject: [PATCH 1/4] Avoid slicing text inside multi-byte characters Right-to-left line reordering walked each atom's byte range and sliced the line at every byte, panicking when a neutral multi-byte character such as a guillemet preceded the first strong character. Walk characters instead. FIND/FINDALL located the REPLACE keyword in a Unicode-uppercased copy and sliced the original with that offset; uppercasing can change byte length (dotless i, ligatures). Uppercase ASCII only, which keeps offsets aligned. FILLET and CHAMFER inline R/D shorthands now test the original text before slicing past the first character. Co-Authored-By: Claude Opus 5 (1M context) --- src/app/commands/inquiry.rs | 22 ++++++++++++++++++++-- src/entities/text_support.rs | 12 ++++++++++-- src/modules/draw/modify/fillet.rs | 4 ++-- 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/app/commands/inquiry.rs b/src/app/commands/inquiry.rs index 161a6c28c..eb33b4ca0 100644 --- a/src/app/commands/inquiry.rs +++ b/src/app/commands/inquiry.rs @@ -1302,8 +1302,9 @@ impl OpenCADStudio { let all_mode = cmd.starts_with("FINDALL"); let rest = cmd.split_once(' ').map(|(_, r)| r.trim()).unwrap_or(""); - // Split at " REPLACE " keyword (case-insensitive) - let (search, replacement) = if let Some(pos) = rest.to_uppercase().find(" REPLACE ") + // Split at " REPLACE " keyword (case-insensitive). ASCII case + // mapping keeps byte offsets valid for slicing `rest`. + let (search, replacement) = if let Some(pos) = rest.to_ascii_uppercase().find(" REPLACE ") { (&rest[..pos], Some(rest[pos + 9..].trim())) } else { @@ -2386,3 +2387,20 @@ mod align_selected_bounds_tests { assert_eq!(line_start_x(&app, b), 2.0, "b must keep its offset from a, not collapse onto it"); } } + +#[cfg(test)] +mod find_replace_command_tests { + use super::*; + + /// Unicode case mapping can change a string's byte length (`ı` uppercases + /// to `I`), so the REPLACE keyword split must not slice with offsets + /// taken from a case-mapped copy. + #[test] + fn find_replace_accepts_search_text_whose_case_mapping_changes_length() { + for cmd in ["FIND ı REPLACE x", "FINDALL fi REPLACE fi", "FIND ʼn replace n"] { + let mut app = OpenCADStudio::new_for_test(); + let i = app.active_tab; + let _ = app.dispatch_inquiry(cmd, i); + } + } +} diff --git a/src/entities/text_support.rs b/src/entities/text_support.rs index f26f7d52a..c115c4e18 100644 --- a/src/entities/text_support.rs +++ b/src/entities/text_support.rs @@ -798,9 +798,11 @@ pub fn reorder_line_atoms(atoms: Vec, is_rtl: bool) -> Vec= 0.0 { @@ -2109,7 +2109,7 @@ impl CadCommand for ChamferCommand { return Some(CmdResult::NeedPoint); } // "D 5.0" or "D 5.0 3.0" inline shorthand - if upper.starts_with('D') { + if t.starts_with(['d', 'D']) { let body = t[1..].trim(); let parts: Vec = body .split_whitespace() From a3d65e2fc8509096866425d6c55f7e5a831c9e09 Mon Sep 17 00:00:00 2001 From: Christopher Overbeck Date: Thu, 17 Sep 2026 10:02:30 -0500 Subject: [PATCH 2/4] Bound typed counts and values that could crash the editor - ARRAYRECT, ARRAYPOLAR, ARRAYPATH, ARRAY3D and COPY Array reject item counts above 100,000 (checked as a product of every dimension) and keep the prompt open. Unbounded counts allocated one transform per item and aborted on allocation failure. Stored defaults are clamped as well. - EXPLODE leaves a MINSERT above the same limit intact instead of materializing every cell. - Typed lengths and angles reject non-finite values and magnitudes above 1e15; SCALE rejects a non-finite factor. - Zero-radius FILLET compares candidate intersections with total_cmp and rejects non-finite results; partial_cmp().unwrap() panicked on NaN. - SOLID falls back to a minimal kernel tolerance when its corners' extent overflows, since Tolerance::new panics on infinity. Co-Authored-By: Claude Opus 5 (1M context) --- src/entities/common.rs | 41 +++++++++++++ src/entities/solid.rs | 29 +++++++++- src/modules/draw/modify/array.rs | 93 +++++++++++++++++++++++++++--- src/modules/draw/modify/copy.rs | 15 +++++ src/modules/draw/modify/explode.rs | 22 +++++++ src/modules/draw/modify/fillet.rs | 31 +++++++++- src/modules/draw/modify/scale.rs | 7 ++- 7 files changed, 223 insertions(+), 15 deletions(-) diff --git a/src/entities/common.rs b/src/entities/common.rs index 6ab4aa857..2c012d226 100644 --- a/src/entities/common.rs +++ b/src/entities/common.rs @@ -283,6 +283,20 @@ fn surveyor(value_rad: f64, prec: usize) -> String { /// A leading `-` is a sign, but the `-` inside `5'-9"` is a separator — after /// feet there is nothing left to subtract from. pub fn parse_length(text: &str) -> Option { + parse_length_unbounded(text).filter(|value| typed_value_in_range(*value)) +} + +/// Largest magnitude a typed length or angle may have. Rust's float parser +/// accepts `inf`, `nan` and overflowing exponents; geometry built from those, +/// or from values whose differences overflow, breaks downstream tolerance and +/// intersection math. +pub const MAX_TYPED_MAGNITUDE: f64 = 1e15; + +pub fn typed_value_in_range(value: f64) -> bool { + value.is_finite() && value.abs() <= MAX_TYPED_MAGNITUDE +} + +fn parse_length_unbounded(text: &str) -> Option { let text = text.trim(); let (sign, rest) = match text.strip_prefix('-') { Some(rest) => (-1.0, rest.trim_start()), @@ -339,6 +353,10 @@ fn parse_inches(text: &str) -> Option { /// number is read in whatever convention the drawing is set to, so what the /// readout shows can be typed straight back. pub fn parse_angle(text: &str) -> Option { + parse_angle_unbounded(text).filter(|value| typed_value_in_range(*value)) +} + +fn parse_angle_unbounded(text: &str) -> Option { let text = text.trim(); if text.is_empty() { return None; @@ -679,11 +697,16 @@ pub fn parse_f64(value: &str) -> Option { .ok() .or_else(|| parse_length(t)) .or_else(|| parse_angle_deg(t)) + .filter(|value| typed_value_in_range(*value)) } /// Parse an angle string the panel displayed via AUNITS back to DEGREES: /// "30", "30°"/"30d", DMS "30°15'20.5\"", grads "33.33g", radians "0.52r". pub fn parse_angle_deg(value: &str) -> Option { + parse_angle_deg_unbounded(value).filter(|value| typed_value_in_range(*value)) +} + +fn parse_angle_deg_unbounded(value: &str) -> Option { let s = value.trim(); if s.is_empty() { return None; @@ -1161,6 +1184,24 @@ pub(crate) fn polyline_segment_fill( } } +#[cfg(test)] +mod typed_range_tests { + use super::*; + + #[test] + fn non_finite_and_huge_typed_values_are_rejected() { + for text in ["inf", "-inf", "nan", "1e400", "1e308"] { + assert_eq!(parse_length(text), None, "length {text}"); + assert_eq!(parse_angle(text), None, "angle {text}"); + assert_eq!(parse_f64(text), None, "property {text}"); + assert_eq!(parse_angle_deg(text), None, "angle degrees {text}"); + } + assert_eq!(parse_length("1e16"), None); + assert_eq!(parse_length("1e15"), Some(1e15)); + assert_eq!(parse_length("-5'-6\""), Some(-66.0)); + } +} + #[cfg(test)] mod length_format_tests { use super::*; diff --git a/src/entities/solid.rs b/src/entities/solid.rs index bb4972f51..c41bc381a 100644 --- a/src/entities/solid.rs +++ b/src/entities/solid.rs @@ -80,7 +80,14 @@ fn boundary_edges(points: &[[f64; 3]]) -> Vec { } fn kernel_tolerance(points: &[[f64; 3]]) -> f64 { - cadkernel::space::coplanarity_tolerance(points).max(1.0e-12) + let tolerance = cadkernel::space::coplanarity_tolerance(points); + // Corners whose extent overflows f64 give an infinite tolerance, which + // `Tolerance::new` rejects by panicking. + if tolerance.is_finite() { + tolerance.max(1.0e-12) + } else { + 1.0e-12 + } } fn extrusion_body( @@ -366,3 +373,23 @@ impl Transformable for Solid { ); } } + +#[cfg(test)] +mod overflow_tests { + use super::*; + use acadrust::types::Vector3; + + /// Corners 1e308 apart overflow the coplanarity tolerance to infinity, + /// which the kernel's `Tolerance::new` rejects by panicking. + #[test] + fn corners_with_overflowing_extent_do_not_panic() { + let solid = Solid::new( + Vector3::new(0.0, 0.0, 0.0), + Vector3::new(1e308, 0.0, 0.0), + Vector3::new(0.0, 1e308, 0.0), + Vector3::new(1e308, 1e308, 0.0), + ); + let _ = wcs_bounds(&solid); + let _ = solid_geometry(&solid); + } +} diff --git a/src/modules/draw/modify/array.rs b/src/modules/draw/modify/array.rs index d0d7a83b6..eb4ba3b1b 100644 --- a/src/modules/draw/modify/array.rs +++ b/src/modules/draw/modify/array.rs @@ -45,6 +45,27 @@ pub const DROPDOWN_ITEMS: &[(&str, &str, IconKind)] = &[ ), ]; +// ── Item limit ───────────────────────────────────────────────────────────── + +/// Most items one array may create, including the source. Every item becomes +/// a transform and then a copy of each selected object, so an unbounded typed +/// count exhausts memory before anything is drawn. +pub(crate) const MAX_ARRAY_ITEMS: u64 = 100_000; + +/// Whether an array of `counts` (rows, columns, levels, ...) stays in bounds. +pub(crate) fn array_items_within_limit(counts: &[u64]) -> bool { + counts + .iter() + .try_fold(1u64, |total, &count| total.checked_mul(count)) + .is_some_and(|total| total <= MAX_ARRAY_ITEMS) +} + +pub(crate) fn array_limit_error() -> CmdResult { + CmdResult::ReportError( + crate::tf!("Array size exceeds the limit of {} items.", MAX_ARRAY_ITEMS).into_owned(), + ) +} + // ── Rectangular Array ────────────────────────────────────────────────────── #[derive(Debug, Clone, Copy)] @@ -72,8 +93,8 @@ impl ArrayRectCommand { handles, wire_models, step: RectStep::Rows, - default_rows: defaults::get_array_rows() as u32, - default_cols: defaults::get_array_cols() as u32, + default_rows: (defaults::get_array_rows() as u32).clamp(1, MAX_ARRAY_ITEMS as u32), + default_cols: (defaults::get_array_cols() as u32).clamp(1, MAX_ARRAY_ITEMS as u32), default_row_sp: defaults::get_array_row_sp(), default_col_sp: defaults::get_array_col_sp(), plane: WorkingPlane::default(), @@ -154,6 +175,9 @@ impl CadCommand for ArrayRectCommand { self.default_rows } else { let v = t.parse::().unwrap_or(self.default_rows).max(1); + if !array_items_within_limit(&[v as u64]) { + return Some(array_limit_error()); + } defaults::set_array_rows(v as f64); self.default_rows = v; v @@ -165,11 +189,15 @@ impl CadCommand for ArrayRectCommand { let cols = if t.is_empty() { self.default_cols } else { - let v = t.parse::().unwrap_or(self.default_cols).max(1); - defaults::set_array_cols(v as f64); - self.default_cols = v; - v + t.parse::().unwrap_or(self.default_cols).max(1) }; + if !array_items_within_limit(&[rows as u64, cols as u64]) { + return Some(array_limit_error()); + } + if !t.is_empty() { + defaults::set_array_cols(cols as f64); + self.default_cols = cols; + } self.step = RectStep::RowSp { rows, cols }; None } @@ -222,6 +250,10 @@ impl CadCommand for ArrayRectCommand { } RectStep::ColSp { rows, cols, row_sp } => (rows, cols, row_sp, self.default_col_sp), }; + // Defaults are only bounded one count at a time. + if !array_items_within_limit(&[rows as u64, cols as u64]) { + return vec![]; + } Self::build_transforms(rows, cols, row_sp, col_sp, self.plane) .iter() .flat_map(|t| { @@ -274,7 +306,7 @@ impl ArrayPolarCommand { handles, wire_models, step: PolarStep::Center, - default_count: defaults::get_array_p_count() as u32, + default_count: (defaults::get_array_p_count() as u32).clamp(2, MAX_ARRAY_ITEMS as u32), default_angle: defaults::get_array_p_angle(), plane: WorkingPlane::default(), } @@ -330,6 +362,9 @@ impl CadCommand for ArrayPolarCommand { self.default_count } else { let v = t.parse::().unwrap_or(self.default_count).max(2); + if !array_items_within_limit(&[v as u64]) { + return Some(array_limit_error()); + } defaults::set_array_p_count(v as f64); self.default_count = v; v @@ -461,7 +496,7 @@ impl ArrayPathCommand { wire_models, all_entities, step: PathStep::SelectPath, - default_count: defaults::get_array_path_count() as u32, + default_count: (defaults::get_array_path_count() as u32).clamp(2, MAX_ARRAY_ITEMS as u32), pick_pt: DVec3::ZERO, } } @@ -686,6 +721,9 @@ impl CadCommand for ArrayPathCommand { self.default_count } else { let v = t.parse::().unwrap_or(self.default_count).max(2); + if !array_items_within_limit(&[v as u64]) { + return Some(array_limit_error()); + } defaults::set_array_path_count(v as f64); self.default_count = v; v @@ -891,6 +929,9 @@ impl CadCommand for Array3DCommand { } else { t.parse::().unwrap_or(2).max(1) }; + if !array_items_within_limit(&[v as u64]) { + return Some(array_limit_error()); + } self.step = Array3DStep::Cols { rows: v }; Some(CmdResult::NeedPoint) } @@ -900,6 +941,9 @@ impl CadCommand for Array3DCommand { } else { t.parse::().unwrap_or(2).max(1) }; + if !array_items_within_limit(&[rows as u64, v as u64]) { + return Some(array_limit_error()); + } self.step = Array3DStep::Levels { rows, cols: v }; Some(CmdResult::NeedPoint) } @@ -909,6 +953,9 @@ impl CadCommand for Array3DCommand { } else { t.parse::().unwrap_or(2).max(1) }; + if !array_items_within_limit(&[rows as u64, cols as u64, v as u64]) { + return Some(array_limit_error()); + } self.step = Array3DStep::RowSp { rows, cols, @@ -987,3 +1034,33 @@ impl CadCommand for Array3DCommand { // ── Autocomplete registry ───────────────────────────────── inventory::submit!(crate::command::CommandRegistration { names: &["ARRAY3D", "3DARRAY"] }); + +#[cfg(test)] +mod item_limit_tests { + use super::*; + + #[test] + fn item_limit_counts_every_dimension_without_overflow() { + assert!(array_items_within_limit(&[MAX_ARRAY_ITEMS])); + assert!(!array_items_within_limit(&[MAX_ARRAY_ITEMS + 1])); + assert!(!array_items_within_limit(&[1_000, 1_000])); + assert!(!array_items_within_limit(&[u32::MAX as u64, u32::MAX as u64, u32::MAX as u64])); + } + + #[test] + fn oversized_counts_are_rejected_at_the_prompt() { + let mut rect = ArrayRectCommand::new(vec![Handle::new(1)], Vec::new()); + assert!(rect.on_text_input("100000").is_none()); + assert!(matches!(rect.on_text_input("100000"), Some(CmdResult::ReportError(_)))); + assert!(matches!(rect.step, RectStep::Cols { rows: 100_000 })); + + let mut polar = ArrayPolarCommand::new(vec![Handle::new(1)], Vec::new()); + polar.step = PolarStep::Count { center: DVec3::ZERO }; + assert!(matches!(polar.on_text_input("4294967295"), Some(CmdResult::ReportError(_)))); + + let mut cube = Array3DCommand::new(vec![Handle::new(1)]); + assert!(matches!(cube.on_text_input("300"), Some(CmdResult::NeedPoint))); + assert!(matches!(cube.on_text_input("300"), Some(CmdResult::NeedPoint))); + assert!(matches!(cube.on_text_input("2"), Some(CmdResult::ReportError(_)))); + } +} diff --git a/src/modules/draw/modify/copy.rs b/src/modules/draw/modify/copy.rs index 95bce7e51..9b192ece4 100644 --- a/src/modules/draw/modify/copy.rs +++ b/src/modules/draw/modify/copy.rs @@ -199,6 +199,9 @@ impl CadCommand for CopyCommand { fn on_text_input(&mut self, text: &str) -> Option { if self.awaiting_count { if let Ok(n) = text.trim().parse::() { + if !super::array::array_items_within_limit(&[n as u64]) { + return Some(super::array::array_limit_error()); + } if n >= 2 { self.array_count = Some(n); } @@ -277,6 +280,18 @@ mod tests { cmd.options().into_iter().map(|o| o.keyword).collect() } + #[test] + fn array_count_above_the_item_limit_is_rejected() { + let mut cmd = CopyCommand::new(vec![Handle::new(1)], vec![]); + cmd.awaiting_count = true; + assert!(matches!( + cmd.on_text_input("18446744073709551615"), + Some(CmdResult::ReportError(_)) + )); + assert!(cmd.awaiting_count, "the count prompt stays up"); + assert_eq!(cmd.array_count, None); + } + #[test] fn mode_single_exits_after_one_copy() { let mut cmd = CopyCommand::new(vec![Handle::new(1)], vec![]); diff --git a/src/modules/draw/modify/explode.rs b/src/modules/draw/modify/explode.rs index 5243e8af2..6ef505171 100644 --- a/src/modules/draw/modify/explode.rs +++ b/src/modules/draw/modify/explode.rs @@ -88,6 +88,16 @@ pub fn explode_entity(entity: &EntityType, document: &CadDocument) -> Vec explode_polyline2d(p), EntityType::Polyline(p) => explode_polyline(p), EntityType::Polyline3D(p) => explode_polyline3d(p), + // A MINSERT expands to one copy of the block per cell; leave arrays + // too large to materialize intact instead of exhausting memory. + EntityType::Insert(ins) + if !super::array::array_items_within_limit(&[ + ins.row_count as u64, + ins.column_count as u64, + ]) => + { + vec![] + } EntityType::Insert(ins) => ins .explode_from_document(document) .into_iter() @@ -1478,6 +1488,18 @@ mod tests { use super::*; use acadrust::entities::DimensionLinear; + #[test] + fn minsert_too_large_to_materialize_is_left_intact() { + let mut doc = CadDocument::new(); + let mut block = acadrust::tables::BlockRecord::new("B1"); + block.handle = doc.allocate_handle(); + doc.block_records.add(block).unwrap(); + let mut insert = acadrust::entities::Insert::new("B1", Vector3::new(0.0, 0.0, 0.0)); + insert.row_count = u16::MAX; + insert.column_count = u16::MAX; + assert!(explode_entity(&EntityType::Insert(insert), &doc).is_empty()); + } + /// A dimension created without a geometry block gets a real `*D` block on /// bake, its `block_name` resolves to that block, and a second bake is a /// no-op (already has a valid block). diff --git a/src/modules/draw/modify/fillet.rs b/src/modules/draw/modify/fillet.rs index 6bc3b5a52..5ebd86941 100644 --- a/src/modules/draw/modify/fillet.rs +++ b/src/modules/draw/modify/fillet.rs @@ -1062,10 +1062,13 @@ fn fillet_line_arc( + (p1[1] + a * u[1] - click_line[1]).powi(2); let db = (p1[0] + b * u[0] - click_line[0]).powi(2) + (p1[1] + b * u[1] - click_line[1]).powi(2); - da.partial_cmp(&db).unwrap() + da.total_cmp(&db) })?; let ix = p1[0] + t_best * u[0]; let iy = p1[1] + t_best * u[1]; + if !(ix.is_finite() && iy.is_finite()) { + return None; + } // Trim line to intersection let new_line = trim_line_to_point(line, [ix, iy], click_line)?; @@ -1154,9 +1157,11 @@ fn fillet_arc_arc( let ip = *pts.iter().min_by(|a, b| { (a[0] - cx) .hypot(a[1] - cy) - .partial_cmp(&(b[0] - cx).hypot(b[1] - cy)) - .unwrap() + .total_cmp(&(b[0] - cx).hypot(b[1] - cy)) })?; + if !(ip[0].is_finite() && ip[1].is_finite()) { + return None; + } let ia1 = arc_angle_at(c1, ip); let ia2 = arc_angle_at(c2, ip); @@ -2368,6 +2373,26 @@ mod tests { cmd.options().into_iter().map(|o| o.keyword).collect() } + /// The upper half of a radius-5 circle at (5, 0), scaled about the origin + /// by an infinite factor: the center becomes (inf, NaN). + fn overflowed_arc(sweep_sign: f64) -> ArcEnt { + let mut arc = ArcEnt::new(); + arc.center = acadrust::types::Vector3::new(f64::INFINITY, f64::NAN, 0.0); + arc.radius = f64::INFINITY; + arc.start_angle = 0.0; + arc.end_angle = sweep_sign * std::f64::consts::PI; + arc + } + + #[test] + fn zero_radius_fillet_rejects_non_finite_geometry() { + let line = LineEnt::from_coords(f64::NAN, f64::NAN, 0.0, f64::INFINITY, f64::INFINITY, 0.0); + let arc = overflowed_arc(1.0); + assert!(fillet_line_arc(&line, [5.0, 5.0], &arc, [5.0, 5.0], 0.0, 0.0).is_none()); + let other = overflowed_arc(-1.0); + assert!(fillet_arc_arc(&arc, [5.0, 5.0], &other, [5.0, -5.0], 0.0, 0.0).is_none()); + } + #[test] fn fillet_undo_option_appears_after_a_fillet_and_refreshes_from_document() { let mut command = FilletCommand::new( diff --git a/src/modules/draw/modify/scale.rs b/src/modules/draw/modify/scale.rs index 1fc8d242f..a9ce25b20 100644 --- a/src/modules/draw/modify/scale.rs +++ b/src/modules/draw/modify/scale.rs @@ -179,12 +179,12 @@ impl CadCommand for ScaleCommand { return Some(CmdResult::NeedPoint); } let factor: f64 = t.replace(',', ".").parse().ok()?; - (factor > 0.0).then(|| self.commit(base, factor)) + (factor.is_finite() && factor > 0.0).then(|| self.commit(base, factor)) } Step::RefFirst { base } => { let base = *base; let ref_dist: f64 = t.replace(',', ".").parse().ok()?; - if ref_dist > 0.0 { + if ref_dist.is_finite() && ref_dist > 0.0 { self.step = Step::RefNew { base, ref_dist }; return Some(CmdResult::NeedPoint); } @@ -194,7 +194,8 @@ impl CadCommand for ScaleCommand { Step::RefNew { base, ref_dist } => { let (base, ref_dist) = (*base, *ref_dist); let new_len: f64 = t.replace(',', ".").parse().ok()?; - (new_len > 0.0).then(|| self.commit(base, new_len / ref_dist)) + let factor = new_len / ref_dist; + (factor.is_finite() && factor > 0.0).then(|| self.commit(base, factor)) } Step::Base => None, } From 1ac9309f858a2c1f2c2740a984283c91a1b2f71d Mon Sep 17 00:00:00 2001 From: Christopher Overbeck Date: Thu, 17 Sep 2026 10:05:29 -0500 Subject: [PATCH 3/4] Guard constraint solving and DWG export against degenerate geometry Retaining a line's length added a P2PDistance constraint, whose gradient divides by the current distance. For a zero-length line the solver got a NaN Jacobian and its SVD fallback never returned, freezing the editor (e.g. ECONSTRAINT on a line drawn by clicking one point twice). Hold coincident endpoints together with per-axis differences instead. Control-point splines read without knots are given a degree their control points can carry (at most n - 1) and a clamped knot vector when a drawing loads. Generating knots for fewer points than the degree underflowed during DWG write: a panic in debug builds, an unbounded allocation in release, and reachable from autosave. A lone control point with no knots or fit points is purged as corrupt. Co-Authored-By: Claude Opus 5 (1M context) --- src/app/commands/draw.rs | 19 +++++++++++ src/io/mod.rs | 64 +++++++++++++++++++++++++++++++++++ src/scene/parametric_solve.rs | 33 ++++++++++++++---- 3 files changed, 110 insertions(+), 6 deletions(-) diff --git a/src/app/commands/draw.rs b/src/app/commands/draw.rs index 67a06ab81..b7ab4904a 100644 --- a/src/app/commands/draw.rs +++ b/src/app/commands/draw.rs @@ -2346,3 +2346,22 @@ mod region_tests { } } } + +#[cfg(test)] +mod degenerate_constraint_tests { + use crate::app::OpenCADStudio; + + /// Retaining a zero-length line's size used to hand the solver a NaN + /// Jacobian, and its SVD fallback never returned. + #[test] + fn equal_constraint_with_a_zero_length_line_returns() { + let mut app = OpenCADStudio::new_for_test(); + app.automation_op(r#"{"op":"new"}"#); + app.automation_op(r#"{"op":"run","cmd":"LINE 0,0 0,0 "}"#); + app.automation_op(r#"{"op":"run","cmd":"LINE 5,5 20,7 "}"#); + app.automation_op(r#"{"op":"select","type":"Line"}"#); + app.automation_op(r#"{"op":"run","cmd":"ECONSTRAINT"}"#); + let lines = app.automation_op(r#"{"op":"query","type":"Line","detail":"geometry"}"#); + assert_eq!(lines["ok"], true, "{lines}"); + } +} diff --git a/src/io/mod.rs b/src/io/mod.rs index fdc687529..d7be16cf8 100644 --- a/src/io/mod.rs +++ b/src/io/mod.rs @@ -2146,8 +2146,12 @@ pub(crate) fn is_entity_corrupt(e: &EntityType) -> bool { && (s.knots.iter().any(|k| !k.is_finite()) || s.knots.windows(2).any(|w| w[1] < w[0]) || (!compact_periodic_knots && s.knots.len() != n + deg + 1)); + // A single control point with no knots and no fit points cannot + // form a curve, and no knot vector exists for it. + let lone_point = n == 1 && s.knots.is_empty() && s.fit_points.is_empty(); n >= MAX_VERTS || degree_bad + || lone_point || s.control_points.iter().any(|p| !finite_vec3(p)) || knots_bad } @@ -2155,8 +2159,32 @@ pub(crate) fn is_entity_corrupt(e: &EntityType) -> bool { } } +/// Give control-point splines stored without knots a degree their control +/// points can carry, and the matching clamped knot vector. +/// +/// A degree-p curve needs at least p + 1 control points. Knot generation +/// (here, and in the DWG writer for knot-less splines) subtracts in `usize` +/// and underflows when there are fewer; in release builds the wrapped count +/// drives an unbounded allocation. Lowering the degree keeps the geometry the +/// points describe: two points are a straight segment at any degree. +fn normalize_knotless_splines(doc: &mut CadDocument) { + for entity in doc.entities_mut() { + let EntityType::Spline(spline) = entity else { + continue; + }; + let n = spline.control_points.len(); + if !spline.knots.is_empty() || n < 2 || spline.degree < 1 { + continue; + } + let degree = (spline.degree as usize).min(n - 1); + spline.degree = degree as i32; + spline.knots = acadrust::entities::Spline::generate_clamped_knots(degree, n); + } +} + pub fn purge_corrupt_entities(doc: &mut CadDocument) -> usize { use crate::par::prelude::*; + normalize_knotless_splines(doc); // Detection is pure and read-only; the per-vertex finite/extent checks on // large polylines dominate, so fan the scan out across cores. Gather // entity references in one pass, test in parallel, then remove serially @@ -2363,6 +2391,42 @@ mod corrupt_guard_tests { use acadrust::entities::{Arc, Circle, EntityType, Spline}; use acadrust::types::Vector3; + fn knotless_spline(degree: i32, points: usize) -> Spline { + let mut spline = Spline::new(); + spline.degree = degree; + spline.control_points = (0..points) + .map(|index| Vector3::new(index as f64, index as f64, 0.0)) + .collect(); + spline + } + + // Degree 3 over two control points with no knot vector: generating knots + // for it underflows, and writing it to DWG used to panic (debug) or + // allocate without bound (release). + #[test] + fn knotless_spline_with_too_few_points_is_lowered_and_saves_as_dwg() { + let mut doc = CadDocument::new(); + doc.add_entity(EntityType::Spline(knotless_spline(3, 2))); + doc.add_entity(EntityType::Spline(knotless_spline(3, 1))); + assert_eq!(purge_corrupt_entities(&mut doc), 1, "the lone point is dropped"); + let spline = doc + .entities() + .find_map(|entity| match entity { + EntityType::Spline(spline) => Some(spline.clone()), + _ => None, + }) + .expect("the two-point spline is kept"); + assert_eq!(spline.degree, 1); + assert_eq!(spline.knots, vec![0.0, 0.0, 1.0, 1.0]); + + let path = std::env::temp_dir().join(format!( + "ocs_knotless_spline_{}.dwg", + std::process::id() + )); + save_as_version(&doc, &path, acadrust::DxfVersion::AC1032).expect("save"); + let _ = std::fs::remove_file(&path); + } + // Small but finite arcs are valid records. Kernel tessellation is bounded, // so opening must retain them instead of treating their size as corruption. #[test] diff --git a/src/scene/parametric_solve.rs b/src/scene/parametric_solve.rs index 30e56a8fc..bffeb669a 100644 --- a/src/scene/parametric_solve.rs +++ b/src/scene/parametric_solve.rs @@ -1830,6 +1830,29 @@ fn retained_radius(entity: &EntityType) -> Option { } } +/// Hold the distance between two points at `length`. +/// +/// `P2PDistance` divides by the current distance, so a zero-length segment +/// gives the solver a NaN Jacobian. Keep coincident endpoints together with +/// per-axis differences instead. +fn retain_segment_length( + sys: &mut System, + a: GPoint, + b: GPoint, + current_length: f64, + length: f64, +) { + if current_length > f64::EPSILON && length > f64::EPSILON { + let target = sys.add_param(length, true); + sys.add_constraint(Rc::new(P2PDistance::new(a, b, target))); + return; + } + let dx = sys.add_param(0.0, true); + let dy = sys.add_param(0.0, true); + sys.add_constraint(Rc::new(Difference::new(a.x, b.x, dx))); + sys.add_constraint(Rc::new(Difference::new(a.y, b.y, dy))); +} + /// Visit vertices and segments in stored drawing order, including the closing edge last. fn polyline_ref_order( document: &acadrust::CadDocument, @@ -2139,8 +2162,7 @@ fn solve_scope( .get(handle) .and_then(|entity| retained_line_length(entity, None)) .unwrap_or(current_length); - let target = sys.add_param(length, true); - sys.add_constraint(Rc::new(P2PDistance::new(line.p1, line.p2, target))); + retain_segment_length(&mut sys, line.p1, line.p2, current_length, length); } EntityGeom::Polyline { points, @@ -2169,8 +2191,7 @@ fn solve_scope( .get(handle) .and_then(|entity| retained_line_length(entity, Some(index))) .unwrap_or(current_length); - let target = sys.add_param(length, true); - sys.add_constraint(Rc::new(P2PDistance::new(a, b, target))); + retain_segment_length(&mut sys, a, b, current_length, length); } } EntityGeom::Circle(circle) => { @@ -2346,8 +2367,8 @@ fn solve_scope( }) }); if directional && !retain_size { - let length = sys.add_param((x2 - x1).hypot(y2 - y1), true); - sys.add_constraint(Rc::new(P2PDistance::new(line.p1, line.p2, length))); + let length = (x2 - x1).hypot(y2 - y1); + retain_segment_length(&mut sys, line.p1, line.p2, length, length); } else if !directional { let dx = sys.add_param(x2 - x1, true); let dy = sys.add_param(y2 - y1, true); From 37059cb0d6ebfd81ba593ee6442f5d1c19b1d5da Mon Sep 17 00:00:00 2001 From: Christopher Overbeck Date: Thu, 17 Sep 2026 10:12:39 -0500 Subject: [PATCH 4/4] Repair knot-less splines on every load path load_file, used by headless export, xrefs and block import, finalizes a drawing without purging, so the spline degree repair never ran there and `--export drawing.dxf out.dwg` still hit the knot underflow. Run the repair when any load is finalized. Co-Authored-By: Claude Opus 5 (1M context) --- src/io/mod.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/io/mod.rs b/src/io/mod.rs index d7be16cf8..38a115cbe 100644 --- a/src/io/mod.rs +++ b/src/io/mod.rs @@ -1000,6 +1000,7 @@ fn finalize_loaded_outcome( ) -> Result { let doc = &mut outcome.document; normalize_block_origins(doc); + normalize_knotless_splines(doc); if outcome.stats.source_format == Some(acadrust::SourceFormat::Dxf) { fix_dxf_dimension_rotations(doc); fix_dxf_layout_plot_settings(doc); @@ -2427,6 +2428,31 @@ mod corrupt_guard_tests { let _ = std::fs::remove_file(&path); } + // `load_file` (headless export, xrefs, block import) does not purge, so + // the spline repair has to happen when any load is finalized. + #[test] + fn knotless_spline_is_repaired_by_every_load_path() { + const DXF: &str = "0\nSECTION\n2\nENTITIES\n0\nSPLINE\n8\n0\n70\n0\n71\n3\n72\n0\n73\n2\n74\n0\n\ +10\n0\n20\n0\n30\n0\n10\n1\n20\n1\n30\n0\n0\nENDSEC\n0\nEOF\n"; + let dir = std::env::temp_dir(); + let source = dir.join(format!("ocs_knotless_load_{}.dxf", std::process::id())); + let target = dir.join(format!("ocs_knotless_load_{}.dwg", std::process::id())); + std::fs::write(&source, DXF).unwrap(); + let doc = load_file(&source).expect("load"); + let _ = std::fs::remove_file(&source); + let spline = doc + .entities() + .find_map(|entity| match entity { + EntityType::Spline(spline) => Some(spline), + _ => None, + }) + .expect("spline"); + assert_eq!(spline.degree, 1); + assert_eq!(spline.knots.len(), 4); + save_as_version(&doc, &target, acadrust::DxfVersion::AC1032).expect("save"); + let _ = std::fs::remove_file(&target); + } + // Small but finite arcs are valid records. Kernel tessellation is bounded, // so opening must retain them instead of treating their size as corruption. #[test]