forked from 1b-agent/opencadstudio-upstream
Fix unbounded MINSERT allocation and scale-multiplied spacing
Two independent MINSERT (array INSERT) bugs found while auditing render_graph.rs: 1. array_offsets() reserved and filled one Vec element per array instance, with row_count/column_count parsed as unchecked u16s. A corrupt or adversarial 65535x65535 pair requests 4,294,836,225 instances (~96 GiB) before any GPU-side chunking limit can help, and before the render graph even reaches the GPU. Fixed with a documented instance budget (20,000) enforced at both the load-time corrupt-entity filter (is_entity_corrupt, matching the existing polyline/spline vertex-count guards) and a failsafe clamp in array_offsets itself, for an insert built or edited some other way. 2. insert_instance_transform composed the per-instance row/column offset translation *before* the INSERT's own transform, so the block's x/y scale multiplied the spacing along with the block contents — an INSERT with x-scale 2 and column spacing 10 rendered 20 units between columns instead of 10. The pinned CAD model's own Insert::array_points() confirms the correct behavior: spacing rotates with the insert but is never scaled. Fixed by orienting the offset with rotation + OCS only, then translating the already- composed (scaled) instance transform by that world-space vector. 7 new tests cover both: pathological/reasonable instance counts at the load-time filter and the render-graph failsafe, and instance transforms for scaled and rotated arrays. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
4ddd19f6e8
commit
8bc9e83c2e
2 files changed
+160
-4
No files matched your search
@@ -2035,7 +2035,17 @@ pub(crate) fn is_entity_corrupt(e: &EntityType) -> bool {
|
|||||||
// rarely use this many — and parser desync produces exactly-100_000-vertex
|
// rarely use this many — and parser desync produces exactly-100_000-vertex
|
||||||
// junk records.
|
// junk records.
|
||||||
const MAX_VERTS: usize = 100_000;
|
const MAX_VERTS: usize = 100_000;
|
||||||
|
// A MINSERT's row/column counts are parsed `u16`s whose unchecked product
|
||||||
|
// (`Insert::instance_count`) drives a per-instance allocation in
|
||||||
|
// `render_graph::array_offsets` and a full per-instance render-graph walk
|
||||||
|
// for every one — a corrupt or adversarial 65535x65535 pair requests over
|
||||||
|
// four billion instances (tens of GiB) long before any GPU-side chunking
|
||||||
|
// limit could help. Kept in sync with the identical failsafe clamp in
|
||||||
|
// `render_graph::array_offsets`, which guards inserts that reach the
|
||||||
|
// render graph some other way (e.g. built at runtime, not loaded).
|
||||||
|
const MAX_MINSERT_INSTANCES: usize = 20_000;
|
||||||
match e {
|
match e {
|
||||||
|
E::Insert(i) => i.instance_count() > MAX_MINSERT_INSTANCES,
|
||||||
E::LwPolyline(p) => {
|
E::LwPolyline(p) => {
|
||||||
!finite_unit_normal(&p.normal)
|
!finite_unit_normal(&p.normal)
|
||||||
|| p.vertices.len() >= MAX_VERTS
|
|| p.vertices.len() >= MAX_VERTS
|
||||||
@@ -2410,4 +2420,33 @@ mod corrupt_guard_tests {
|
|||||||
let s = Spline::from_control_points(3, pts);
|
let s = Spline::from_control_points(3, pts);
|
||||||
assert!(!is_entity_corrupt(&EntityType::Spline(s)));
|
assert!(!is_entity_corrupt(&EntityType::Spline(s)));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// A corrupt or adversarial MINSERT row/column pair (u16, so its unchecked
|
||||||
|
// product can reach into the billions) must be rejected before it can
|
||||||
|
// drive the render graph's per-instance allocation and expansion.
|
||||||
|
#[test]
|
||||||
|
fn rejects_pathological_minsert_counts() {
|
||||||
|
let mut i = acadrust::entities::Insert::new("BLOCK", Vector3::ZERO);
|
||||||
|
i.row_count = u16::MAX;
|
||||||
|
i.column_count = u16::MAX;
|
||||||
|
assert!(is_entity_corrupt(&EntityType::Insert(i)));
|
||||||
|
}
|
||||||
|
|
||||||
|
// An ordinary array insert, well under the budget, is valid source data.
|
||||||
|
#[test]
|
||||||
|
fn keeps_a_reasonable_minsert() {
|
||||||
|
let mut i = acadrust::entities::Insert::new("BLOCK", Vector3::ZERO);
|
||||||
|
i.row_count = 10;
|
||||||
|
i.column_count = 10;
|
||||||
|
i.row_spacing = 5.0;
|
||||||
|
i.column_spacing = 5.0;
|
||||||
|
assert!(!is_entity_corrupt(&EntityType::Insert(i)));
|
||||||
|
}
|
||||||
|
|
||||||
|
// A plain (non-array) INSERT is never treated as a MINSERT-count problem.
|
||||||
|
#[test]
|
||||||
|
fn keeps_a_plain_insert() {
|
||||||
|
let i = acadrust::entities::Insert::new("BLOCK", Vector3::ZERO);
|
||||||
|
assert!(!is_entity_corrupt(&EntityType::Insert(i)));
|
||||||
|
}
|
||||||
}
|
}
|
||||||
+121
-4
@@ -7,7 +7,7 @@
|
|||||||
//! producing their normal wire, hatch, image, wipeout, or mesh model.
|
//! producing their normal wire, hatch, image, wipeout, or mesh model.
|
||||||
|
|
||||||
use acadrust::entities::Insert;
|
use acadrust::entities::Insert;
|
||||||
use acadrust::types::{Color, Transform, Vector3};
|
use acadrust::types::{Color, Matrix3, Matrix4, Transform, Vector3};
|
||||||
use acadrust::{CadDocument, EntityType, Handle};
|
use acadrust::{CadDocument, EntityType, Handle};
|
||||||
use rustc_hash::{FxHashMap, FxHashSet};
|
use rustc_hash::{FxHashMap, FxHashSet};
|
||||||
|
|
||||||
@@ -1042,11 +1042,34 @@ pub fn document_block_uses(document: &CadDocument) -> Vec<BlockUse> {
|
|||||||
uses
|
uses
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// A MINSERT's row/column counts are parsed `u16`s whose unchecked product
|
||||||
|
/// (`Insert::instance_count`) would otherwise drive this function's
|
||||||
|
/// allocation and a full per-instance render-graph walk after it — a corrupt
|
||||||
|
/// or adversarial 65535x65535 pair requests over four billion instances (tens
|
||||||
|
/// of GiB) long before any GPU-side chunking limit could help. The primary
|
||||||
|
/// defense is the load-time filter in `io::is_entity_corrupt` (kept in sync
|
||||||
|
/// with this constant), which drops such an insert before it ever reaches a
|
||||||
|
/// `Scene`; this is the failsafe for an insert built or edited some other way
|
||||||
|
/// (API/scripting) that skips that path.
|
||||||
|
const MAX_MINSERT_INSTANCES: usize = 20_000;
|
||||||
|
|
||||||
pub fn array_offsets(insert: &Insert) -> Vec<[f64; 3]> {
|
pub fn array_offsets(insert: &Insert) -> Vec<[f64; 3]> {
|
||||||
if !insert.is_minsert() {
|
if !insert.is_minsert() {
|
||||||
return vec![[0.0; 3]];
|
return vec![[0.0; 3]];
|
||||||
}
|
}
|
||||||
let mut offsets = Vec::with_capacity(insert.instance_count());
|
let instance_count = insert.instance_count();
|
||||||
|
if instance_count > MAX_MINSERT_INSTANCES {
|
||||||
|
eprintln!(
|
||||||
|
"MINSERT '{}' requests {} instances ({}x{}), exceeding the {} budget; rendering a single instance instead",
|
||||||
|
insert.block_name,
|
||||||
|
instance_count,
|
||||||
|
insert.row_count,
|
||||||
|
insert.column_count,
|
||||||
|
MAX_MINSERT_INSTANCES
|
||||||
|
);
|
||||||
|
return vec![[0.0; 3]];
|
||||||
|
}
|
||||||
|
let mut offsets = Vec::with_capacity(instance_count);
|
||||||
for row in 0..insert.row_count {
|
for row in 0..insert.row_count {
|
||||||
for column in 0..insert.column_count {
|
for column in 0..insert.column_count {
|
||||||
offsets.push([
|
offsets.push([
|
||||||
@@ -1070,8 +1093,20 @@ pub fn insert_instance_transform(
|
|||||||
if offset == [0.0; 3] {
|
if offset == [0.0; 3] {
|
||||||
transform
|
transform
|
||||||
} else {
|
} else {
|
||||||
Transform::from_translation(Vector3::new(offset[0], offset[1], offset[2]))
|
// Row/column spacing is defined in the insert's rotated/OCS-oriented
|
||||||
.then(&transform)
|
// space but is NOT subject to the block's x/y/z scale — this matches
|
||||||
|
// the pinned CAD model's own `Insert::array_points`, which rotates
|
||||||
|
// the offset but never scales it. Composing the raw offset ahead of
|
||||||
|
// the full (scaled) transform, as this used to do, let the block
|
||||||
|
// scale multiply the spacing too (e.g. an x-scale-2 insert with
|
||||||
|
// column spacing 10 would render 20 units between columns). Instead,
|
||||||
|
// orient the offset by rotation + OCS only, then translate the
|
||||||
|
// already-composed (scaled) instance transform by that world-space
|
||||||
|
// vector, applied after — never scaled.
|
||||||
|
let orientation = Transform::from_matrix(Matrix4::rotation_z(insert.rotation))
|
||||||
|
.then(&Transform::from_matrix(Matrix4::from_matrix3(Matrix3::arbitrary_axis(insert.normal))));
|
||||||
|
let world_offset = orientation.apply_rotation(Vector3::new(offset[0], offset[1], offset[2]));
|
||||||
|
transform.then(&Transform::from_translation(world_offset))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1163,4 +1198,86 @@ mod tests {
|
|||||||
assert!((from_insert.x - 12.0).abs() < 1.0e-9);
|
assert!((from_insert.x - 12.0).abs() < 1.0e-9);
|
||||||
assert!((applied.x - 11.0).abs() < 1.0e-9);
|
assert!((applied.x - 11.0).abs() < 1.0e-9);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn array_offsets_clamps_pathological_minsert_counts() {
|
||||||
|
// A corrupt or adversarial row/column pair (parsed as u16, so this is
|
||||||
|
// reachable from a malformed file) must not drive a multi-billion-
|
||||||
|
// element allocation; the primary defense is the load-time filter in
|
||||||
|
// `io::is_entity_corrupt`, this is the render-graph-side failsafe.
|
||||||
|
let mut insert = Insert::new("BLOCK", Vector3::ZERO);
|
||||||
|
insert.row_count = u16::MAX;
|
||||||
|
insert.column_count = u16::MAX;
|
||||||
|
insert.row_spacing = 1.0;
|
||||||
|
insert.column_spacing = 1.0;
|
||||||
|
let offsets = array_offsets(&insert);
|
||||||
|
assert_eq!(offsets, vec![[0.0; 3]], "pathological instance count must clamp to a single instance");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn array_offsets_is_unaffected_for_a_reasonable_minsert() {
|
||||||
|
let mut insert = Insert::new("BLOCK", Vector3::ZERO);
|
||||||
|
insert.row_count = 3;
|
||||||
|
insert.column_count = 4;
|
||||||
|
insert.row_spacing = 5.0;
|
||||||
|
insert.column_spacing = 2.0;
|
||||||
|
let offsets = array_offsets(&insert);
|
||||||
|
assert_eq!(offsets.len(), 12);
|
||||||
|
assert_eq!(offsets[0], [0.0, 0.0, 0.0]);
|
||||||
|
assert_eq!(offsets[1], [2.0, 0.0, 0.0]);
|
||||||
|
assert_eq!(offsets[4], [0.0, 5.0, 0.0]);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn insert_instance_transform_does_not_scale_the_row_column_spacing() {
|
||||||
|
// github discussion-derived report: an INSERT with x-scale 2 and
|
||||||
|
// column spacing 10 was rendering 20 drawing units between columns
|
||||||
|
// instead of 10, because the offset translation was composed ahead
|
||||||
|
// of (and thus multiplied by) the block scale.
|
||||||
|
let document = CadDocument::new();
|
||||||
|
let mut insert = Insert::new("BLOCK", Vector3::new(100.0, 0.0, 0.0));
|
||||||
|
insert.set_x_scale(2.0);
|
||||||
|
insert.row_count = 1;
|
||||||
|
insert.column_count = 2;
|
||||||
|
insert.column_spacing = 10.0;
|
||||||
|
|
||||||
|
let transform = insert_instance_transform(
|
||||||
|
&document,
|
||||||
|
&insert,
|
||||||
|
[10.0, 0.0, 0.0],
|
||||||
|
1.0,
|
||||||
|
BlockScalePolicy::Applied,
|
||||||
|
);
|
||||||
|
// The block's own origin (local point 0,0,0) placed at this instance
|
||||||
|
// must land 10 units from the insert point, not 20.
|
||||||
|
let world = transform.apply(Vector3::ZERO);
|
||||||
|
assert!((world.x - 110.0).abs() < 1.0e-9, "expected spacing 10, got offset {}", world.x - 100.0);
|
||||||
|
|
||||||
|
// Block-local geometry must still pick up the block scale normally.
|
||||||
|
let block_point = transform.apply(Vector3::new(1.0, 0.0, 0.0));
|
||||||
|
assert!((block_point.x - 112.0).abs() < 1.0e-9, "block-local geometry should still be scaled by x_scale 2");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn insert_instance_transform_rotates_the_spacing_with_the_insert() {
|
||||||
|
// Matches the pinned CAD model's own `Insert::array_points`: spacing
|
||||||
|
// is rotated with the insert but never scaled.
|
||||||
|
let document = CadDocument::new();
|
||||||
|
let mut insert = Insert::new("BLOCK", Vector3::ZERO);
|
||||||
|
insert.rotation = std::f64::consts::FRAC_PI_2;
|
||||||
|
insert.row_count = 1;
|
||||||
|
insert.column_count = 2;
|
||||||
|
insert.column_spacing = 10.0;
|
||||||
|
|
||||||
|
let transform = insert_instance_transform(
|
||||||
|
&document,
|
||||||
|
&insert,
|
||||||
|
[10.0, 0.0, 0.0],
|
||||||
|
1.0,
|
||||||
|
BlockScalePolicy::Applied,
|
||||||
|
);
|
||||||
|
let world = transform.apply(Vector3::ZERO);
|
||||||
|
assert!(world.x.abs() < 1.0e-9, "a 90deg rotation should turn column spacing into Y, got x={}", world.x);
|
||||||
|
assert!((world.y - 10.0).abs() < 1.0e-9, "expected the spacing rotated onto Y, got y={}", world.y);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
Reference in new issue
Block a user