collective
Browse / github.com/storytold/designcraft / patch
Import: a shared string and a footnote are not copied per reference (#1584)
Import: a shared string and a footnote are not copied per reference (#1584) Audit **DC-03** (#1584), High / Confirmed — two amplifications where one piece of a file is copied per reference. Both are measured by their controls rather than argued. ## XLSX: the shared string A `t="s"` cell materialises its **own copy** of the shared string, and every cell is collected before the grid's `MAX_CELLS` ever applies. Control, with the budget removed, a 64 KB shared string referenced 2,000 times: ``` the table holds at most the budget: 131072000 bytes ``` 125 MB of cell text from a 64 KB string. The issue's case — 1 MB and ~3M references — is terabytes, and an allocation t
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- Hamza0101 (imported from pull request #1871, open)
- Base
ed8aabd1dc5a88652a076631c2d46886ca60c031- Size
- +131 −3 in 2 files
- Review
- pending
- Shared
- 2026-10-28
- Id
85c5ae0300a11502fee3c82c
How it went
No other operator has reported using it yet.
Diff
raw · crates/textimport/src/docx.rs crates/textimport/src/xlsx.rs
@@ -236,6 +236,12 @@ struct Ctx { para_names: HashMap<String, String>, char_names: HashMap<String, String>, footnotes: HashMap<String, El>,+ /// Footnote ids already imported, and how much note text has been built. A reference clones+ /// its note and walks it again, so one 1 MB footnote referenced 100k times meant ~100 GB+ /// (#1584). Word gives each note its own id and references it once; a second reference to the+ /// same id gets the mark without the text.+ notes_done: std::collections::HashSet<String>,+ note_bytes: usize, warnings: Vec<String>, next_table: u64, numbering: Numbering,@@ -412,11 +418,33 @@ impl Ctx { "noBreakHyphen" => "\u{2011}".into(), "softHyphen" => "\u{AD}".into(), "footnoteReference" if allow_notes => {+ const MAX_NOTE_BYTES: usize = 8 << 20;+ let id = e.attr("id").unwrap_or_default().to_string();+ // The note's text is built once per id, and only while the budget lasts. A+ // repeat reference, or one past the budget, still gets its mark and its+ // number -- the reference is part of the text -- with an empty note (#1584).+ let again = !id.is_empty() && !self.notes_done.insert(id.clone());+ let spent = self.note_bytes >= MAX_NOTE_BYTES;+ if again || spent {+ let len = st.len();+ if st.insert_note(len, "", ParaFormat::default()).is_some() {+ let message = if again {+ format!("Footnote {id} is referenced more than once in the Word file: the text is kept at its first reference.")+ } else {+ format!("The Word file's footnotes are longer than {} MB: the rest were imported empty.", MAX_NOTE_BYTES / (1 << 20))+ };+ if !self.warnings.contains(&message) {+ self.warnings.push(message);+ }+ }+ continue;+ } if let Some(note) = e.attr("id").and_then(|id| self.footnotes.get(id).cloned()) { let mut ns = Story::new(StoryId(0)); let counter = std::mem::take(&mut self.model_counter); self.blocks(¬e, &mut ns, false); self.model_counter = counter;+ self.note_bytes = self.note_bytes.saturating_add(ns.text.len()); // Word puts the reference mark in the note text: we generate the number. let len = st.len(); match st.insert_note(len, "", ParaFormat::default()) {@@ -485,6 +513,8 @@ pub fn import(bytes: &[u8]) -> Result<Imported, ImportError> { para_names: HashMap::new(), char_names: HashMap::new(), footnotes: HashMap::new(),+ notes_done: std::collections::HashSet::new(),+ note_bytes: 0, warnings: vec![], next_table: 0, numbering: Numbering::default(),@@ -674,6 +704,38 @@ mod tests { assert_eq!(size, None, "a size the engine cannot mean is dropped, so the style's size stands"); } + /// A footnote reference clones its note and walks it again, so one large footnote referenced+ /// many times multiplied: the audit's case is a 1 MB note referenced 100k times, around+ /// 100 GB, which aborts rather than fails (#1584). The text is kept at the first reference;+ /// later ones get the mark and an empty note, and the report says so.+ #[test]+ fn a_footnote_referenced_many_times_is_imported_once() {+ let note_text = "x".repeat(2_000);+ let footnotes = format!(r#"<w:footnotes {W}><w:footnote w:id="2"><w:p><w:r><w:t>{note_text}</w:t></w:r></w:p></w:footnote></w:footnotes>"#);+ // One note, referenced 50 times.+ let refs = r#"<w:r><w:footnoteReference w:id="2"/></w:r>"#.repeat(50);+ let doc = format!(r#"<w:document {W}><w:body><w:p>{refs}</w:p></w:body></w:document>"#);+ let i = import(&docx(&doc, "", &footnotes)).unwrap();++ // Every reference is still in the text: the marks are the document's content.+ assert_eq!(i.story.notes.len(), 50, "all 50 references are kept");+ let with_text = i.story.notes.iter().filter(|n| !n.text.text.is_empty()).count();+ assert_eq!(with_text, 1, "only the first reference carries the note's text");+ let total: usize = i.story.notes.iter().map(|n| n.text.text.len()).sum();+ assert!(total <= note_text.len() + 64, "the note text is stored once, not 50 times: {total} bytes");+ assert!(i.warnings.iter().any(|w| w.contains("referenced more than once")), "the report says what it did: {:?}", i.warnings);++ // A document whose notes each have their own id is unaffected.+ let many = (2..6).map(|id| format!(r#"<w:footnote w:id="{id}"><w:p><w:r><w:t>note {id}</w:t></w:r></w:p></w:footnote>"#)).collect::<String>();+ let footnotes = format!(r#"<w:footnotes {W}>{many}</w:footnotes>"#);+ let refs = (2..6).map(|id| format!(r#"<w:r><w:footnoteReference w:id="{id}"/></w:r>"#)).collect::<String>();+ let doc = format!(r#"<w:document {W}><w:body><w:p>{refs}</w:p></w:body></w:document>"#);+ let i = import(&docx(&doc, "", &footnotes)).unwrap();+ assert_eq!(i.story.notes.len(), 4);+ assert!(i.story.notes.iter().all(|n| n.text.text.contains("note")), "each distinct note keeps its text");+ assert!(!i.warnings.iter().any(|w| w.contains("referenced more than once")), "{:?}", i.warnings);+ }+ #[test] fn imports_styles_runs_footnotes_and_tables() { let styles = format!(@@ -212,19 +212,41 @@ pub fn import(bytes: &[u8]) -> Result<Imported, ImportError> { .transpose()? .map(|sst| sst.els().filter(|e| e.name == "si").map(string_item).collect()) .unwrap_or_default();+ // A `t="s"` cell holds an index into the shared strings and each one is materialised as its+ // own `String`, so one large shared string referenced by many cells multiplies: a 1 MB string+ // and 3M references is terabytes, allocated before the grid cap below ever applies, and an+ // allocation that large aborts the process rather than failing (#1584). Collection carries a+ // byte budget and a cell count of its own.+ const MAX_TEXT_BYTES: usize = 64 << 20;+ const MAX_COLLECTED_CELLS: usize = 400_000; let mut cells: Vec<((usize, usize), String)> = Vec::new();+ let mut text_bytes = 0usize;+ let mut cut = false; if let Some(data) = sheet.child("sheetData") {- for (ri, row) in data.els().filter(|e| e.name == "row").enumerate() {+ 'rows: for (ri, row) in data.els().filter(|e| e.name == "row").enumerate() { for (ci, c) in row.els().filter(|e| e.name == "c").enumerate() { let at = c.attr("r").and_then(cell_ref).unwrap_or((ri, ci)); let v = value(c, &shared);- if !v.is_empty() {- cells.push((at, v));+ if v.is_empty() {+ continue;+ }+ if cells.len() >= MAX_COLLECTED_CELLS || text_bytes.saturating_add(v.len()) > MAX_TEXT_BYTES {+ cut = true;+ break 'rows; }+ text_bytes = text_bytes.saturating_add(v.len());+ cells.push((at, v)); } } } let mut warnings = Vec::new();+ if cut {+ warnings.push(format!(+ "the worksheet holds more than {} cells or {} MB of text: the rest was not read",+ MAX_COLLECTED_CELLS,+ MAX_TEXT_BYTES / (1 << 20)+ ));+ } if cells.is_empty() { warnings.push("the worksheet is empty".into()); }@@ -293,6 +315,50 @@ mod tests { buf.into_inner() } + /// A `t="s"` cell materialises its own copy of the shared string, so one big string with many+ /// references multiplies: the audit's case is a 1 MB string and 3M cells, terabytes, built+ /// before the grid cap applies (#1584). Collection now carries its own byte budget.+ #[test]+ fn a_shared_string_referenced_many_times_does_not_multiply() {+ // A 64 KB shared string referenced 2,000 times would be 128 MB of cell text; the budget+ // is 64 MB, so collection stops and says so.+ let big = "y".repeat(64 * 1024);+ let cells: String = (1..=2_000).map(|r| format!(r#"<row r="{r}"><c r="A{r}" t="s"><v>0</v></c></row>"#)).collect();+ let mut buf = std::io::Cursor::new(Vec::new());+ {+ let mut z = zip::ZipWriter::new(&mut buf);+ let o = zip::write::SimpleFileOptions::default();+ for (name, body) in [+ (+ "xl/workbook.xml",+ r#"<workbook xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main" xmlns:r="http://schemas.openxmlformats.org/officeDocument/2006/relationships"><sheets><sheet name="Big" sheetId="1" r:id="rId1"/></sheets></workbook>"#.to_string(),+ ),+ (+ "xl/_rels/workbook.xml.rels",+ r#"<Relationships xmlns="http://schemas.openxmlformats.org/package/2006/relationships"><Relationship Id="rId1" Type="x" Target="worksheets/sheet1.xml"/></Relationships>"#.to_string(),+ ),+ ("xl/sharedStrings.xml", format!("<sst><si><t>{big}</t></si></sst>")),+ ("xl/worksheets/sheet1.xml", format!("<worksheet><sheetData>{cells}</sheetData></worksheet>")),+ ] {+ z.start_file(name, o).unwrap();+ z.write_all(body.as_bytes()).unwrap();+ }+ z.finish().unwrap();+ }+ let imp = import(&buf.into_inner()).unwrap();+ let table = imp.story.tables.values().next().expect("a table");+ let text: usize = (0..table.nrows())+ .flat_map(|r| (0..table.ncols()).map(move |c| (r, c)))+ .filter_map(|(r, c)| table.cell(r, c).map(|cell| cell.text.text.len()))+ .sum();+ assert!(text <= 64 << 20, "the table holds at most the budget: {text} bytes");+ assert!(imp.warnings.iter().any(|w| w.contains("was not read")), "the report says the sheet was cut: {:?}", imp.warnings);++ // An ordinary workbook is unaffected and reports nothing of the sort.+ let plain = import(&workbook()).unwrap();+ assert!(!plain.warnings.iter().any(|w| w.contains("was not read")), "{:?}", plain.warnings);+ }+ #[test] fn first_sheet_used_range_becomes_a_table() { let imp = import(&workbook()).unwrap();