From 1a322faa0bc3d9efdbf32908582bd2a2bd344ff7 Mon Sep 17 00:00:00 2001 From: ffishman <20211677+ffishman@users.noreply.github.com> Date: Fri, 30 Oct 2026 09:08:20 -0300 Subject: [PATCH] fix(dwg): scan the bridge header in one pass; leave out proxies of a binary DXF header_value kept an offset for every line up to the first ENDSEC: an input without one cost about 24 bytes per line (0.8 GB for 32 MiB of short lines, some 25 GB at the 1 GiB input limit). It now reads (group code, value) pairs in one pass and stops at 0/ENDSEC, so memory no longer grows with the input (36 MB for the same 32 MiB, the input itself). dxf_class_names doesn't read a binary DXF's CLASSES, so its proxies kept their DXF numbers, which name other classes in the DWG. CADCraft's own bridge is always ASCII; for a binary DXF given to the conversion functions, the proxies are now left out, as a pre-2013 DWG leaves them, instead of naming the wrong classes. Both reported by CodeRabbit on #2471. --- crates/dwg/src/lib.rs | 109 +++++++++++++++++++++++++---------------- crates/io/src/tests.rs | 20 ++++++++ 2 files changed, 88 insertions(+), 41 deletions(-) diff --git a/crates/dwg/src/lib.rs b/crates/dwg/src/lib.rs index ce4133569..cd260baa4 100644 --- a/crates/dwg/src/lib.rs +++ b/crates/dwg/src/lib.rs @@ -588,22 +588,28 @@ mod native { } /// The byte offset and the trimmed value of header variable `name` in an ASCII DXF: the value - /// line after the group code that follows the `9` group naming it. Only the HEADER section - /// (up to the first `ENDSEC`) is searched; lines may end in `\r\n`. + /// of the group that follows the `9` group naming it. The file is read as (group code, + /// value) line pairs, in one pass with no buffering, up to the first `0`/`ENDSEC`, the end + /// of the HEADER section: a large input without one costs time, not memory (a line index of + /// a 32 MiB file used 0.8 GB). Lines may end in `\r\n`. fn header_value<'a>(dxf: &'a [u8], name: &[u8]) -> Option<(usize, &'a [u8])> { - let mut lines: Vec<(usize, &[u8])> = Vec::new(); let mut offset = 0usize; - for line in dxf.split(|b| *b == b'\n') { - let trimmed = line.trim_ascii(); - let lead = line.len() - line.trim_ascii_start().len(); - lines.push((offset.saturating_add(lead), trimmed)); + let mut lines = dxf.split(|b| *b == b'\n').map(|line| { + let at = offset.saturating_add(line.len() - line.trim_ascii_start().len()); offset = offset.saturating_add(line.len()).saturating_add(1); - if trimmed == b"ENDSEC" { - break; + (at, line.trim_ascii()) + }); + while let (Some((_, code)), Some((_, value))) = (lines.next(), lines.next()) { + if code == b"0" && value == b"ENDSEC" { + return None; + } + if code == b"9" && value == name { + let (_, code) = lines.next()?; + let (at, value) = lines.next()?; + return (code != b"0" && code != b"9").then_some((at, value)); } } - let at = lines.windows(2).position(|w| w[0].1 == b"9" && w[1].1 == name)?; - lines.get(at.checked_add(3)?).copied() + None } /// The current dimension and text styles' names from their handles: a DWG stores the @@ -642,11 +648,13 @@ mod native { /// The DXF names of an ASCII DXF's CLASS records, in CLASSES order: a proxy's class number /// (group 91) is 500 plus its class's place here (DXF Reference, ACAD_PROXY_OBJECT). Empty for - /// a binary DXF or one without a CLASSES section. - pub(super) fn dxf_class_names(dxf: &[u8]) -> Vec { + /// one without a CLASSES section. `None` for a binary DXF, whose classes this doesn't read: + /// CADCraft's own bridge DXF is always ASCII, and only a binary DXF given to the public + /// conversion functions comes here. + pub(super) fn dxf_class_names(dxf: &[u8]) -> Option> { let mut names = Vec::new(); if dxf.starts_with(b"AutoCAD Binary DXF") { - return names; + return None; } let mut lines = dxf.split(|b| *b == b'\n').map(|l| l.trim_ascii()); let (mut in_classes, mut in_class) = (false, false); @@ -662,7 +670,7 @@ mod native { _ => {} } } - names + Some(names) } /// Proxy objects' and entities' class numbers as the DWG numbers its classes. A DXF proxy @@ -676,28 +684,33 @@ mod native { /// proxy's class is not one of them: the proxy would name a missing class (the ODA refuses /// the file: "No ClassId") or, by its old number, some other class. Such a DWG leaves the /// proxies out instead, as it does other programs' data it can't write (`not_saved` reports - /// the entities). Returns how many proxies were left out. - fn proxy_classes(doc: &mut acadrust::CadDocument, classes: &[String]) -> usize { - if doc.version < acadrust::DxfVersion::AC1027 { - let entities: Vec = doc - .entities() - .filter_map(|e| match e { - acadrust::entities::EntityType::Extended(x) if matches!(x.data, acadrust::entities::ExtendedEntityData::Proxy(_)) => { - Some(x.common.handle) - } - _ => None, - }) - .collect(); - let objects: Vec = - doc.objects.iter().filter_map(|(h, o)| matches!(o, acadrust::objects::ObjectType::ProxyObject(_)).then_some(*h)).collect(); - for h in &entities { - doc.remove_entity(*h); + /// the entities). So does a DXF whose classes are unknown (`classes` is `None`, a binary + /// DXF): its proxies' numbers can't be mapped, and kept as they are they would name the + /// wrong classes and the ODA would refuse the file. Returns how many proxies were left out. + fn proxy_classes(doc: &mut acadrust::CadDocument, classes: Option<&[String]>) -> usize { + let classes = match classes { + Some(c) if doc.version >= acadrust::DxfVersion::AC1027 => c, + _ => { + let entities: Vec = doc + .entities() + .filter_map(|e| match e { + acadrust::entities::EntityType::Extended(x) if matches!(x.data, acadrust::entities::ExtendedEntityData::Proxy(_)) => { + Some(x.common.handle) + } + _ => None, + }) + .collect(); + let objects: Vec = + doc.objects.iter().filter_map(|(h, o)| matches!(o, acadrust::objects::ObjectType::ProxyObject(_)).then_some(*h)).collect(); + for h in &entities { + doc.remove_entity(*h); + } + for h in &objects { + doc.objects.remove(h); + } + return entities.len() + objects.len(); } - for h in &objects { - doc.objects.remove(h); - } - return entities.len() + objects.len(); - } + }; let number = |id: i32| -> Option { let name = classes.get(usize::try_from(id.checked_sub(500)?).ok()?)?; doc.classes.get_by_name(name).map(|c| i32::from(c.class_number)) @@ -737,7 +750,6 @@ mod native { 0 } - /// The current layer (`$CLAYER`) by its name, the same way as [`current_dimstyle`]: a DWG /// stores the handle of the layer, and the one read with the DXF needn't be the layer's in /// the document, so the drawing reopened with layer "0" current (#2252). @@ -789,7 +801,7 @@ mod native { // The fix-ups run on the third-party reader's document, so a panic in one is an error, not a crash. guarded(AssertUnwindSafe(|| { super::fail_in_test("convert"); - proxy_classes(&mut doc, &classes); + proxy_classes(&mut doc, classes.as_deref()); other_layout_owners(&mut doc); table_style_rows(&mut doc, true); dimension_flags(&mut doc); @@ -1286,6 +1298,21 @@ mod tests { } /// A kept version not newer than the declared one, a binary DXF and a DXF without `$ACADVER` + /// The header is read as group code and value pairs up to the first `0`/`ENDSEC`: a `$ACADVER` + /// name whose value would come after it, or a file that never ends its HEADER, is no version + /// to replace, and `\r\n` lines keep the value's offset right. + #[cfg(not(target_arch = "wasm32"))] + #[test] + fn declare_version_reads_the_header_in_pairs_up_to_endsec() { + let cut = b" 0\r\nSECTION\r\n 2\r\nHEADER\r\n 9\r\n$ACADVER\r\n 0\r\nENDSEC\r\n 1\r\nAC1015\r\n"; + assert_eq!(native::declare_version(cut, acadrust::DxfVersion::AC1032), None); + let endless = vec![b'\n'; 1 << 20]; + assert_eq!(native::declare_version(&endless, acadrust::DxfVersion::AC1032), None); + let crlf = b" 0\r\nSECTION\r\n 2\r\nHEADER\r\n 9\r\n$ACADVER\r\n 1\r\n AC1015\r\n 0\r\nENDSEC\r\n"; + let out = native::declare_version(crlf, acadrust::DxfVersion::AC1032).unwrap(); + assert_eq!(out, b" 0\r\nSECTION\r\n 2\r\nHEADER\r\n 9\r\n$ACADVER\r\n 1\r\n AC1032\r\n 0\r\nENDSEC\r\n"); + } + /// are read as they are. #[cfg(not(target_arch = "wasm32"))] #[test] @@ -1338,8 +1365,8 @@ mod tests { #[test] fn dxf_class_names_in_classes_order() { let dxf = b" 0\r\nSECTION\r\n 2\r\nHEADER\r\n 0\r\nENDSEC\r\n 0\r\nSECTION\r\n 2\r\nCLASSES\r\n 0\r\nCLASS\r\n 1\r\nWIDGETA\r\n 2\r\nAcDbWidgetA\r\n 3\r\nApp\r\n 0\r\nCLASS\r\n 1\r\nWIDGETB\r\n 2\r\nAcDbWidgetB\r\n 0\r\nENDSEC\r\n 0\r\nSECTION\r\n 2\r\nENTITIES\r\n 0\r\nTEXT\r\n 1\r\nNOT_A_CLASS\r\n 0\r\nENDSEC\r\n 0\r\nEOF\r\n"; - assert_eq!(native::dxf_class_names(dxf), ["WIDGETA", "WIDGETB"]); - assert!(native::dxf_class_names(b" 0\nSECTION\n 2\nHEADER\n 0\nENDSEC\n 0\nEOF\n").is_empty()); - assert!(native::dxf_class_names(b"AutoCAD Binary DXF\r\n\x1a\0").is_empty()); + assert_eq!(native::dxf_class_names(dxf).unwrap(), ["WIDGETA", "WIDGETB"]); + assert_eq!(native::dxf_class_names(b" 0\nSECTION\n 2\nHEADER\n 0\nENDSEC\n 0\nEOF\n"), Some(Vec::new())); + assert_eq!(native::dxf_class_names(b"AutoCAD Binary DXF\r\n\x1a\0"), None); } } diff --git a/crates/io/src/tests.rs b/crates/io/src/tests.rs index ec29cc0fa..7ffd02398 100644 --- a/crates/io/src/tests.rs +++ b/crates/io/src/tests.rs @@ -2858,6 +2858,26 @@ fn proxies_keep_their_class_in_dwg() { } } +/// A binary DXF given to the DWG conversion directly: its CLASSES aren't read, so the proxies' +/// numbers can't be mapped to the DWG's classes. Kept as they were, they named other classes +/// (the ODA refuses such a file), so the DWG leaves them out, as a pre-2013 DWG does, and the +/// rest of the drawing is written. +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn a_binary_dxf_leaves_its_proxies_out_of_the_dwg() { + use acadrust::entities::{EntityType, ExtendedEntityData}; + use acadrust::objects::ObjectType; + let d = read(&proxy_file(), "p.dxf").unwrap(); + let binary = write_with(&d, "p.dxf", &SaveOptions { version: Some(Version::R2018), binary: true }).unwrap(); + assert!(binary.starts_with(b"AutoCAD Binary DXF")); + let dwg = cadcraft_dwg::dxf_to_dwg_as(&binary, "AC1032").unwrap(); + let doc = acadrust::DwgReader::from_stream(std::io::Cursor::new(dwg)).read().unwrap(); + let proxies = doc.objects.values().filter(|o| matches!(o, ObjectType::ProxyObject(_))).count() + + doc.entities().filter(|e| matches!(e, EntityType::Extended(x) if matches!(x.data, ExtendedEntityData::Proxy(_)))).count(); + assert_eq!(proxies, 0, "no proxy with an unmapped class number"); + assert!(doc.layers.get("0").is_some(), "the drawing itself is written"); +} + /// A name that isn't valid Unicode still picks its format by extension. #[cfg(unix)] #[test]