model: budget the memory an FBX import keeps, and share instanced meshes (#1234)

The 1 GiB array limit counted wire bytes, but arrays are kept as f64/i64,
so boolean arrays grew 8x and 32-bit arrays 2x past it, and nothing bounded
the total across arrays. Every Model linked to a Geometry also built its own
copy of the mesh.

- A per-import Budget (2 GiB) is charged 8 bytes per array element before
  the array is decompressed or widened, ASCII files are charged for their
  parsed values, and each built mesh is charged for its vertex and index
  data. Going past it fails the import with a clear error instead of an
  allocation abort. parse_within/read_within take a lower limit for tests.
- Models instancing the same geometry with the same materials share one
  mesh; a different material mapping still gets its own.

Fixes #1234

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N9kwMcw2pYQ5fSjKa3ZPnh

crates/model/src/fbx.rs

@@ -32,6 +32,57 @@ pub const BINARY_MAGIC: &[u8] = b"Kaydara FBX Binary  \0\x1a\0";
 const MAX_DEPTH: usize = 64;
 /// Largest decoded array accepted (bytes).
 const MAX_ARRAY_BYTES: usize = 1 << 30;
+/// Most memory one import may keep: parsed arrays as stored (8 bytes per element, whatever the
+/// wire type) plus the meshes built from them (#1234).
+const MAX_IMPORT_BYTES: usize = 1 << 31;
+
+/// What is left of an import's memory budget.
+struct Budget(std::cell::Cell<usize>);
+
+impl Budget {
+    fn new(bytes: usize) -> Budget {
+        Budget(std::cell::Cell::new(bytes))
+    }
+    /// Take `bytes` (None: a size that overflowed) from the budget, or fail the import before
+    /// anything that large is allocated.
+    fn take(&self, bytes: Option<usize>, what: &str) -> Result<()> {
+        let left = self.0.get();
+        match bytes.filter(|&b| b <= left) {
+            Some(b) => {
+                self.0.set(left - b);
+                Ok(())
+            }
+            None => Err(perr(format!("FBX: {what} would take the import past its {} MiB memory limit", MAX_IMPORT_BYTES >> 20))),
+        }
+    }
+}
+
+/// Bytes a mesh keeps.
+fn mesh_bytes(m: &Mesh) -> Option<usize> {
+    m.primitives.iter().try_fold(0usize, |acc, p| {
+        let n = p.positions.len().checked_mul(12)?.checked_add(p.normals.len().checked_mul(12)?)?.checked_add(p.uvs.len().checked_mul(8)?)?;
+        let n = n.checked_add(p.tangents.len().checked_mul(16)?)?.checked_add(p.joints.len().checked_mul(8)?)?.checked_add(p.weights.len().checked_mul(16)?)?;
+        acc.checked_add(n)?.checked_add(p.indices.len().checked_mul(4)?)
+    })
+}
+
+/// Bytes the values under `nodes` keep. Used for ASCII files, which hold array elements as
+/// one `Prop` each and are parsed before they can be counted (the text itself bounds them).
+fn value_bytes(nodes: &[FbxNode]) -> Option<usize> {
+    nodes.iter().try_fold(0usize, |acc, n| {
+        let own = n.props.iter().try_fold(0usize, |a, p| {
+            let extra = match p {
+                Prop::Ints(v) => v.len().checked_mul(8)?,
+                Prop::Floats(v) => v.len().checked_mul(8)?,
+                Prop::Str(v) => v.len(),
+                Prop::Bytes(v) => v.len(),
+                Prop::Int(_) | Prop::Float(_) => 0,
+            };
+            a.checked_add(std::mem::size_of::<Prop>())?.checked_add(extra)
+        })?;
+        acc.checked_add(own)?.checked_add(value_bytes(&n.children)?)
+    })
+}
 
 /// One property value of an FBX node.
 #[derive(Clone, Debug, PartialEq)]
@@ -126,7 +177,17 @@ impl FbxNode {
 /// Read an FBX file (binary or ASCII) into its top-level nodes and the file version
 /// (e.g. 7400).
 pub fn read(bytes: &[u8]) -> Result<(u32, Vec<FbxNode>)> {
-    if bytes.starts_with(BINARY_MAGIC) { read_binary(bytes) } else { read_ascii(&String::from_utf8_lossy(bytes)) }
+    read_within(bytes, &Budget::new(MAX_IMPORT_BYTES))
+}
+
+fn read_within(bytes: &[u8], budget: &Budget) -> Result<(u32, Vec<FbxNode>)> {
+    if bytes.starts_with(BINARY_MAGIC) {
+        read_binary(bytes, budget)
+    } else {
+        let (version, nodes) = read_ascii(&String::from_utf8_lossy(bytes))?;
+        budget.take(value_bytes(&nodes), "its values")?;
+        Ok((version, nodes))
+    }
 }
 
 // ---------------------------------------------------------------------------------------------
@@ -136,6 +197,7 @@ struct Bin<'a> {
     d: &'a [u8],
     /// 64-bit record offsets (version 7500 and later).
     wide: bool,
+    budget: &'a Budget,
 }
 
 impl Bin<'_> {
@@ -207,7 +269,7 @@ impl Bin<'_> {
     }
 
     fn prop(&self, pos: &mut usize, pend: usize) -> Result<Prop> {
-        let sub = Bin { d: self.d.get(..pend).ok_or_else(|| perr("FBX: truncated property"))?, wide: self.wide };
+        let sub = Bin { d: self.d.get(..pend).ok_or_else(|| perr("FBX: truncated property"))?, wide: self.wide, budget: self.budget };
         let t = sub.arr::<1>(pos)?[0];
         Ok(match t {
             b'Y' => Prop::Int(i16::from_le_bytes(sub.arr(pos)?) as i64),
@@ -234,6 +296,9 @@ impl Bin<'_> {
                     _ => 1,
                 };
                 let want = len.checked_mul(size).filter(|&n| n <= MAX_ARRAY_BYTES).ok_or_else(|| perr("FBX: array too large"))?;
+                // Elements are kept as f64/i64 (booleans and 32-bit values widen), so the budget
+                // is charged for what is kept, not for the wire bytes.
+                sub.budget.take(len.checked_mul(8), "an array")?;
                 let data = sub.take(pos, clen)?;
                 let raw = match enc {
                     0 => data.to_vec(),
@@ -263,10 +328,10 @@ fn object_name(b: &[u8]) -> String {
     }
 }
 
-fn read_binary(bytes: &[u8]) -> Result<(u32, Vec<FbxNode>)> {
+fn read_binary(bytes: &[u8], budget: &Budget) -> Result<(u32, Vec<FbxNode>)> {
     let mut pos = BINARY_MAGIC.len();
-    let version = Bin { d: bytes, wide: false }.u32(&mut pos)?;
-    let bin = Bin { d: bytes, wide: version >= 7500 };
+    let version = Bin { d: bytes, wide: false, budget }.u32(&mut pos)?;
+    let bin = Bin { d: bytes, wide: version >= 7500, budget };
     let mut nodes = vec![];
     while pos < bytes.len() {
         match bin.record(&mut pos, 0) {
@@ -728,7 +793,13 @@ fn short_name(full: &str) -> &str {
 
 /// Parse an FBX file into a model. `resolve(uri)` reads texture files next to it.
 pub fn parse(bytes: &[u8], resolve: &dyn Fn(&str) -> Option<Vec<u8>>) -> Result<Model> {
-    let (version, doc) = read(bytes)?;
+    parse_within(bytes, resolve, MAX_IMPORT_BYTES)
+}
+
+/// [`parse`] keeping at most `limit` bytes of arrays and meshes.
+fn parse_within(bytes: &[u8], resolve: &dyn Fn(&str) -> Option<Vec<u8>>, limit: usize) -> Result<Model> {
+    let budget = Budget::new(limit);
+    let (version, doc) = read_within(bytes, &budget)?;
     if version != 0 && version < 7000 {
         return Err(ModelError::Unsupported(format!("FBX {}.{} (only FBX 7.x files are read)", version / 1000, version % 1000 / 100)));
     }
@@ -835,19 +906,28 @@ pub fn parse(bytes: &[u8], resolve: &dyn Fn(&str) -> Option<Vec<u8>>) -> Result<
         Some(materials.len() - 1)
     };
 
-    // One mesh per Model with Mesh geometry (material slots are per Model).
+    // One mesh per Model with Mesh geometry (material slots are per Model). Models instancing the
+    // same geometry with the same materials share one mesh rather than each building a copy.
     let mut meshes: Vec<Mesh> = vec![];
+    let mut mesh_of: HashMap<(i64, Vec<usize>), usize> = HashMap::new();
     for &id in &model_ids {
         let Some(&ni) = node_of.get(&id) else { continue };
         let Some(gid) = links.iter().find(|l| l.1 == id && kind(l.0) == Some("Geometry")).map(|l| l.0) else { continue };
         let Some(geom) = by_id.get(&gid) else { continue };
         let mats: Vec<usize> =
             links.iter().filter(|l| l.1 == id && kind(l.0) == Some("Material")).filter_map(|l| material_index(l.0, &mut materials, &mut textures)).collect();
-        let normal_mapped: Vec<bool> = materials.iter().map(|m| m.normal_tex.is_some()).collect();
         let name = nodes.get(ni).map(|n| n.name.clone()).unwrap_or_default();
-        let Some(mesh) = build_mesh(&name, geom, &mats, &|m| normal_mapped.get(m).copied().unwrap_or(false)) else { continue };
-        meshes.push(mesh);
-        let mesh_index = meshes.len() - 1;
+        let mesh_index = match mesh_of.get(&(gid, mats.clone())) {
+            Some(&i) => i,
+            None => {
+                let normal_mapped: Vec<bool> = materials.iter().map(|m| m.normal_tex.is_some()).collect();
+                let Some(mesh) = build_mesh(&name, geom, &mats, &|m| normal_mapped.get(m).copied().unwrap_or(false)) else { continue };
+                budget.take(mesh_bytes(&mesh), &format!("mesh `{name}`"))?;
+                meshes.push(mesh);
+                mesh_of.insert((gid, mats), meshes.len() - 1);
+                meshes.len() - 1
+            }
+        };
         let geo = by_id.get(&id).and_then(|o| geometric(&Props70::of(o)));
         match geo {
             Some(matrix) => {

crates/model/src/fbx_tests.rs

@@ -325,3 +325,87 @@ fn hostile_fbx_files_fail_cleanly() {
     assert!(matches!(parse(v6.as_bytes(), &|_| None), Err(ModelError::Unsupported(_))));
     assert!(parse(b"Objects: {\n Model: 1, \"Model::Empty\", \"Null\" {\n }\n}\n", &|_| None).is_err());
 }
+
+/// A version-7400 binary FBX holding one top-level node `A` with uncompressed arrays of type
+/// `t` (`b'b'`, `b'i'`, `b'f'`, `b'd'`, `b'l'`) and `len` elements each.
+fn binary_arrays(arrays: &[(u8, usize)]) -> Vec<u8> {
+    let mut props = vec![];
+    for &(t, len) in arrays {
+        let size = match t {
+            b'd' | b'l' => 8,
+            b'f' | b'i' => 4,
+            _ => 1,
+        };
+        props.push(t);
+        props.extend((len as u32).to_le_bytes());
+        props.extend(0u32.to_le_bytes());
+        props.extend(((len * size) as u32).to_le_bytes());
+        props.extend(std::iter::repeat_n(1u8, len * size));
+    }
+    let mut out = BINARY_MAGIC.to_vec();
+    out.extend(7400u32.to_le_bytes());
+    let end = out.len() + 13 + 1 + props.len();
+    out.extend((end as u32).to_le_bytes());
+    out.extend((arrays.len() as u32).to_le_bytes());
+    out.extend((props.len() as u32).to_le_bytes());
+    out.push(1);
+    out.push(b'A');
+    out.extend(props);
+    out
+}
+
+#[test]
+fn fbx_arrays_are_charged_for_the_memory_they_keep() {
+    // A boolean array is one byte per element on the wire but eight once kept as `Prop::Ints`:
+    // the budget counts the eight (#1234).
+    let n = 10_000;
+    let bin = binary_arrays(&[(b'b', n)]);
+    assert!(read_within(&bin, &Budget::new(n * 8)).is_ok());
+    let e = read_within(&bin, &Budget::new(n * 8 - 1)).unwrap_err();
+    assert!(e.to_string().contains("memory limit"), "{e}");
+    // 32-bit floats widen to f64 the same way.
+    assert!(read_within(&binary_arrays(&[(b'f', n)]), &Budget::new(n * 4)).is_err());
+    // Several arrays each within the budget but not together.
+    let bin = binary_arrays(&[(b'i', n), (b'd', n), (b'b', n)]);
+    assert!(read_within(&bin, &Budget::new(n * 24)).is_ok());
+    assert!(read_within(&bin, &Budget::new(n * 20)).is_err());
+    // ASCII values count too.
+    let (_, doc) = read(ASCII.as_bytes()).unwrap();
+    let values = value_bytes(&doc).unwrap();
+    assert!(values > 45 * std::mem::size_of::<Prop>(), "{values}");
+    assert!(parse_within(ASCII.as_bytes(), &|_| None, values - 1).unwrap_err().to_string().contains("memory limit"));
+}
+
+/// [`ASCII`] with two more Models instancing its geometry: `Copy` with the same two materials,
+/// `Red only` with just the first.
+fn instanced_scene() -> String {
+    ASCII
+        .replace(
+            "\tMaterial: 400,",
+            "\tModel: 301, \"Model::Copy\", \"Mesh\" {\n\t}\n\tModel: 302, \"Model::Red only\", \"Mesh\" {\n\t}\n\tMaterial: 400,",
+        )
+        .replace(
+            "\tC: \"OO\",500,300\n",
+            "\tC: \"OO\",500,300\n\tC: \"OO\",301,0\n\tC: \"OO\",100,301\n\tC: \"OO\",400,301\n\tC: \"OO\",500,301\n\tC: \"OO\",302,0\n\tC: \"OO\",100,302\n\tC: \"OO\",400,302\n",
+        )
+}
+
+#[test]
+fn fbx_models_instancing_one_geometry_share_its_mesh() {
+    let m = parse(instanced_scene().as_bytes(), &|_| None).unwrap();
+    let mesh_of = |name: &str| {
+        let n = m.nodes.iter().position(|n| n.name == name).unwrap();
+        // The mesh sits on the node, or on its geometric-offset child.
+        m.nodes[n].mesh.or_else(|| m.nodes[n].children.iter().find_map(|&c| m.nodes[c].mesh)).unwrap()
+    };
+    assert_eq!(mesh_of("Shape"), mesh_of("Copy"), "same geometry and materials: one mesh");
+    assert_ne!(mesh_of("Shape"), mesh_of("Red only"), "different materials: its own mesh");
+    assert_eq!(m.meshes.len(), 2);
+    // A budget with room for one copy of the geometry's mesh, but not three, still imports.
+    let one = parse(ASCII.as_bytes(), &|_| None).unwrap();
+    let mesh = mesh_bytes(&one.meshes[0]).unwrap();
+    let (_, doc) = read(instanced_scene().as_bytes()).unwrap();
+    let values = value_bytes(&doc).unwrap();
+    assert!(parse_within(instanced_scene().as_bytes(), &|_| None, values + 2 * mesh).is_ok());
+    assert!(parse_within(instanced_scene().as_bytes(), &|_| None, values + mesh).is_err());
+}