diff --git a/crates/doc/src/assembly.rs b/crates/doc/src/assembly.rs index f6df3fdf..97c362b1 100644 --- a/crates/doc/src/assembly.rs +++ b/crates/doc/src/assembly.rs @@ -507,7 +507,7 @@ impl Document { return Err(DocError::Invalid("too many occurrences".into())); } let id = self.next_id.max(1); - self.next_id = id + 1; + self.next_id = id.checked_add(1).ok_or_else(|| DocError::Invalid("too many items in the design".into()))?; let cname = self.components.iter().find(|c| c.id == component).map(|c| c.name.clone()).unwrap_or_default(); let k = self.occurrences.iter().filter(|o| o.component == component).count() + 1; self.occurrences.push(crate::Occurrence { id, component, parent, name: format!("{cname}:{k}"), transform, grounded: false }); diff --git a/crates/doc/src/document.rs b/crates/doc/src/document.rs index 1e20425b..e156f0cb 100644 --- a/crates/doc/src/document.rs +++ b/crates/doc/src/document.rs @@ -2041,7 +2041,7 @@ impl Document { return Err(DocError::Invalid("too many components".into())); } let id = self.next_id.max(1); - self.next_id = id + 1; + self.next_id = id.checked_add(1).ok_or_else(|| DocError::Invalid("too many items in the design".into()))?; let name = match name.map(str::trim) { Some(n) if !n.is_empty() => n.to_string(), _ => { @@ -2153,7 +2153,7 @@ impl Document { 0 }; let id = self.next_id.max(1); - self.next_id = id + 1; + self.next_id = id.checked_add(1).ok_or_else(|| DocError::Invalid("too many items in the design".into()))?; let name = match name { Some(n) if !n.trim().is_empty() => n.trim().to_string(), _ => self.unique_name(kind.base_name()), diff --git a/crates/doc/src/format.rs b/crates/doc/src/format.rs index 2ba2ad7d..7c0f3301 100644 --- a/crates/doc/src/format.rs +++ b/crates/doc/src/format.rs @@ -455,6 +455,12 @@ pub fn read(s: &str) -> Result { return Err(DocError::Invalid(format!("damaged design: occurrence `{}` has an invalid placement", o.name))); } } + if !all_unique(d.features.iter().map(|f| f.id)) + || !all_unique(d.components.iter().map(|c| c.id)) + || !all_unique(d.occurrences.iter().map(|o| o.id)) + { + return Err(DocError::Invalid("damaged design: duplicate feature, component or occurrence id".into())); + } let component_ids: std::collections::BTreeSet<_> = d.components.iter().map(|component| component.id).chain(std::iter::once(0)).collect(); for feature in &d.features { if let crate::FeatureKind::Sketch { sketch, .. } = &feature.kind { @@ -476,10 +482,21 @@ pub fn read(s: &str) -> Result { // Fill in what older designs (and hand-edited files) may lack; a no-op otherwise. crate::format_offsets::validate_typed(&d)?; normalise(&mut d)?; + // `next_id` is trusted by nothing: a hand-edited or generated file may omit it or leave it stale, + // and the next feature, component or occurrence would then reuse an id that is already taken. + let largest = + d.features.iter().map(|f| f.id).chain(d.components.iter().map(|c| c.id)).chain(d.occurrences.iter().map(|o| o.id)).max().unwrap_or(0); + let floor = largest.checked_add(1).ok_or_else(|| DocError::Invalid("damaged design: the id space is exhausted".into()))?; + d.next_id = d.next_id.max(floor).max(1); d.format = format_string(FORMAT); Ok(d) } +fn all_unique(ids: impl Iterator) -> bool { + let mut seen = std::collections::BTreeSet::new(); + ids.into_iter().all(|id| seen.insert(id)) +} + /// The caps interactive editing enforces, applied to the raw JSON so an oversized sketch is /// never deserialised. Every sketch in the timeline is solved when the design opens, and the /// solver sizes its dense matrices by the sketch, so a file with hundreds of thousands of @@ -1594,4 +1611,63 @@ mod tests { assert!(error.contains("sketch too large"), "{key}: {error}"); } } + + fn two_components_one_occurrence() -> Document { + let mut d = Document::new("ids"); + let a = d.add_component(None, 0).unwrap(); + d.add_component(None, 0).unwrap(); + d.add_occurrence(a, 0, crate::IDENTITY).unwrap(); + d + } + + fn edited(d: &Document, f: impl FnOnce(&mut Value)) -> String { + let mut v: Value = serde_json::from_str(&d.to_json()).unwrap(); + f(&mut v); + v.to_string() + } + + #[test] + fn missing_or_stale_next_id_does_not_reuse_existing_ids() { + let d = two_components_one_occurrence(); + let largest = d.components.iter().map(|c| c.id).chain(d.occurrences.iter().map(|o| o.id)).max().unwrap(); + for stale in [None, Some(0), Some(1)] { + let json = edited(&d, |v| { + let o = v.as_object_mut().unwrap(); + match stale { + Some(n) => o.insert("next_id".into(), n.into()), + None => o.remove("next_id"), + }; + }); + let mut back = read(&json).unwrap(); + assert!(back.next_id > largest, "next_id {} after load with {stale:?}", back.next_id); + let fresh = back.add_component(None, 0).unwrap(); + assert!(fresh > largest, "new component reused id {fresh}"); + } + } + + #[test] + fn duplicate_component_or_occurrence_ids_are_a_damaged_design() { + let d = two_components_one_occurrence(); + let dup_component = edited(&d, |v| v["components"][1]["id"] = v["components"][0]["id"].clone()); + let dup_occurrence = edited(&d, |v| { + let first = v["occurrences"][0].clone(); + v["occurrences"].as_array_mut().unwrap().push(first); + }); + for json in [dup_component, dup_occurrence] { + let error = read(&json).unwrap_err().to_string(); + assert!(error.contains("damaged design"), "{error}"); + } + } + + #[test] + fn exhausted_id_space_is_an_error_not_a_panic() { + let d = two_components_one_occurrence(); + let json = edited(&d, |v| v["next_id"] = u64::MAX.into()); + let mut back = read(&json).unwrap(); + let before = back.components.len(); + assert!(back.add_component(None, 0).is_err()); + assert_eq!(back.components.len(), before); + let json = edited(&d, |v| v["components"][1]["id"] = u64::MAX.into()); + assert!(read(&json).unwrap_err().to_string().contains("damaged design")); + } }