collective
Browse / github.com/storytold/designcraft / patch
Tables: a shape a table cannot hold is clamped, not allocated (#1583)
Tables: a shape a table cannot hold is clamped, not allocated (#1583) Audit **DC-02** (#1583), High / Confirmed: `Table::new` allocates `rows × cols` cells up front, and the counts reach it from IDML and DOCX, where they are whatever the file claims. ## Measured, not assumed `size_of::<Cell>()` is **1,360 bytes** here (a `Cell` carries a `Story`). So the reported IDML case — one `<Table>` with 5,000 `<Row/>` and 5,000 `<Column/>`, about 1 KB zipped — asks for 25M cells, **~34 GB**. As the issue says, that does not fail politely: `handle_alloc_error` aborts the process, and `catch_unwind` in `engine/src/guard.rs` cannot catch an abort, so every open document's
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- Hamza0101 (imported from pull request #1868, open)
- Base
ed8aabd1dc5a88652a076631c2d46886ca60c031- Size
- +112 −8 in 2 files
- Review
- pending
- Shared
- 2026-10-28
- Id
73be79a503134b417d5886fe
How it went
No other operator has reported using it yet.
Diff
raw · crates/doc/src/table.rs crates/idml/src/import.rs
@@ -393,10 +393,44 @@ impl CellRange { } } +/// Most rows a table holds, as InDesign allows.+pub const MAX_TABLE_ROWS: usize = 10_000;+/// Most columns a table holds, as InDesign allows.+pub const MAX_TABLE_COLS: usize = 1_000;+/// Most cells a table holds, whatever its shape.+///+/// A [`Cell`] is about 1.4 KB (it carries a [`Story`]), so this is roughly 340 MB of cells at the+/// limit. The row and column limits alone are not enough: 10,000 × 1,000 would be 13.6 GB, and an+/// IDML story holding one `<Table>` with 5,000 `<Row/>` and 5,000 `<Column/>` -- about 1 KB+/// zipped -- asked for 25M cells, some 34 GB. That allocation does not fail politely: it aborts+/// the process through `handle_alloc_error`, which no `catch_unwind` can save, so every open+/// document's unsaved work goes with it (#1583).+pub const MAX_TABLE_CELLS: usize = 250_000;++/// The rows and columns a table of `rows × cols` is actually given, inside the limits above.+///+/// Columns are clamped first and rows take what is left of the cell budget, because a table is+/// far more often long than wide: truncating a 5,000-column table to 1,000 columns and then its+/// rows loses less of it than the other way round. Callers that read a file report the+/// difference; the counts are a document's claim, not something to trust.+pub fn clamped_size(rows: usize, cols: usize) -> (usize, usize) {+ let cols = cols.clamp(1, MAX_TABLE_COLS);+ let rows = rows.clamp(1, MAX_TABLE_ROWS.min(MAX_TABLE_CELLS / cols).max(1));+ (rows, cols)+}+ impl Table { /// A `rows × cols` table (`header` + body + `footer` rows) whose columns share `width`.+ ///+ /// The shape is clamped to [`clamped_size`]: the counts reach this from IDML and DOCX files,+ /// where they are whatever the file says (#1583). pub fn new(id: u64, body_rows: usize, cols: usize, header: usize, footer: usize, width: f64) -> Table {- let cols = cols.max(1);+ // Header, body and footer rows share one budget: the total is clamped first, then the+ // header and footer take what they can of it, and the body keeps at least one row.+ let (total, cols) = clamped_size(body_rows.max(1).saturating_add(header).saturating_add(footer), cols);+ let header = header.min(total.saturating_sub(1));+ let footer = footer.min(total.saturating_sub(header).saturating_sub(1));+ let body_rows = total.saturating_sub(header).saturating_sub(footer).max(1); let mut rows = Vec::new(); rows.extend((0..header).map(|_| Row { kind: RowKind::Header, ..Default::default() })); rows.extend((0..body_rows.max(1)).map(|_| Row::default()));@@ -1273,6 +1307,63 @@ impl Story { #[cfg(test)] mod tests {+ /// A table's shape comes out of IDML and DOCX files unchecked, and `Cell` is about 1.4 KB, so+ /// a file claiming 5,000 x 5,000 asked for 25M cells -- some 34 GB. That allocation aborts the+ /// process rather than failing, taking every open document's unsaved work with it (#1583).+ #[test]+ fn a_table_larger_than_the_limits_is_clamped_not_allocated() {+ // The reported shape: 5,000 rows and 5,000 columns.+ let t = Table::new(1, 5_000, 5_000, 0, 0, 500.0);+ assert_eq!(t.columns.len(), MAX_TABLE_COLS, "columns are clamped to InDesign's limit");+ assert!(t.cells.len() <= MAX_TABLE_CELLS, "{} cells is within the budget", t.cells.len());+ assert_eq!(t.rows.len() * t.columns.len(), t.cells.len(), "the grid stays rectangular");+ // What it would have been: 25,000,000 cells at 1,360 bytes each.+ assert!(25_000_000 * std::mem::size_of::<Cell>() > 30_000_000_000, "the size this test is about");+ // Each of these tables is the cell budget, about 340 MB. Freed before the next one is+ // built, so the test's peak is one table and not all of them -- a constrained runner+ // would otherwise hold 680 MB here (noted in review).+ drop(t);++ // A long table keeps its rows while it fits the budget (the budget itself is checked by+ // `clamped_size`, which allocates nothing).+ let long = Table::new(2, 10_000, 5, 0, 0, 500.0);+ assert_eq!((long.rows.len(), long.columns.len()), (10_000, 5), "a long thin table is untouched");+ assert_eq!(clamped_size(10_000, 25), (10_000, 25), "10,000 x 25 is inside the budget");++ // Past the row limit the rows are clamped too.+ let longer = Table::new(3, 50_000, 2, 0, 0, 100.0);+ assert_eq!(longer.rows.len(), MAX_TABLE_ROWS, "rows are clamped to InDesign's limit");++ // Header, body and footer share the budget, and the table still has a body.+ let with_parts = Table::new(4, 5_000, 5_000, 3, 2, 500.0);+ assert!(with_parts.rows.len() <= MAX_TABLE_ROWS, "{} rows", with_parts.rows.len());+ assert!(with_parts.cells.len() <= MAX_TABLE_CELLS, "{} cells", with_parts.cells.len());+ assert!(with_parts.rows.iter().any(|r| r.kind == RowKind::Header), "the header survives");+ assert!(with_parts.rows.iter().any(|r| r.kind == RowKind::Footer), "the footer survives");+ assert!(with_parts.rows.iter().any(|r| r.kind == RowKind::Body), "and so does a body row");+ drop(with_parts);++ // A header and footer bigger than the whole budget still leave a body row.+ let all_header = Table::new(6, 1, 1, MAX_TABLE_ROWS * 2, MAX_TABLE_ROWS * 2, 100.0);+ assert!(all_header.rows.len() <= MAX_TABLE_ROWS, "{} rows", all_header.rows.len());+ assert!(all_header.rows.iter().any(|r| r.kind == RowKind::Body), "a body row survives");++ // An ordinary table is exactly what it was.+ let plain = Table::new(5, 4, 4, 1, 1, 400.0);+ assert_eq!((plain.rows.len(), plain.columns.len(), plain.cells.len()), (6, 4, 24));+ }++ /// `clamped_size` is the rule the importers report against.+ #[test]+ fn clamped_size_keeps_the_shape_it_can() {+ assert_eq!(clamped_size(4, 4), (4, 4));+ assert_eq!(clamped_size(0, 0), (1, 1), "a table has at least one cell");+ assert_eq!(clamped_size(10, MAX_TABLE_COLS + 500), (10, MAX_TABLE_COLS));+ assert_eq!(clamped_size(MAX_TABLE_ROWS + 1, 1), (MAX_TABLE_ROWS, 1));+ let (r, c) = clamped_size(5_000, 5_000);+ assert!(r * c <= MAX_TABLE_CELLS && c == MAX_TABLE_COLS, "{r} x {c}");+ }+ use super::*; fn story(t: &str) -> Story {@@ -2196,21 +2196,34 @@ impl<'r> Importer<'r> { let id = self.alloc(); let rows: Vec<&El> = e.find_all("Row").collect(); let cols: Vec<&El> = e.find_all("Column").collect();- let nr = rows.len().max(1);- let nc = cols.len().max(1);+ // A file's row and column counts are its claim, and a table of them is allocated whole: a+ // story holding one `<Table>` with 5,000 of each asked for 25M cells, around 34 GB, which+ // aborts the process instead of failing (#1583). What is read is clamped, and the rest of+ // this function walks the clamped slices -- indexing `t.rows`/`t.columns` by the file's+ // own counts would be out of bounds.+ let (nr, nc) = designcraft_doc::table::clamped_size(rows.len().max(1), cols.len().max(1));+ if nr < rows.len() || nc < cols.len() {+ self.warnings.push(format!(+ "a table of {} rows and {} columns was read as {nr} x {nc}: more than a table can hold",+ rows.len(),+ cols.len()+ ));+ }+ let rows = &rows[..nr.min(rows.len())];+ let cols = &cols[..nc.min(cols.len())]; let mut t = Table::new(id, nr, nc, 0, 0, 0.0); t.options.direction = if e.get("TableDirection") == Some("RightToLeftDirection") { designcraft_doc::TextDirection::RightToLeft } else { designcraft_doc::TextDirection::LeftToRight };- for (r, re) in rows.iter().enumerate() {+ for (row, re) in t.rows.iter_mut().zip(rows.iter()) { let auto = re.boolean("AutoGrow").unwrap_or(true);- t.rows[r].mode = if auto { RowHeightMode::AtLeast } else { RowHeightMode::Exactly };- t.rows[r].height = if auto { re.num("MinimumHeight").unwrap_or(3.0) } else { re.num("SingleRowHeight").unwrap_or(12.0) };+ row.mode = if auto { RowHeightMode::AtLeast } else { RowHeightMode::Exactly };+ row.height = if auto { re.num("MinimumHeight").unwrap_or(3.0) } else { re.num("SingleRowHeight").unwrap_or(12.0) }; }- for (c, ce) in cols.iter().enumerate() {- t.columns[c].width = ce.num("SingleColumnWidth").unwrap_or(72.0).max(3.0);+ for (column, ce) in t.columns.iter_mut().zip(cols.iter()) {+ column.width = ce.num("SingleColumnWidth").unwrap_or(72.0).max(3.0); } let h = e.num("HeaderRowCount").unwrap_or(0.0) as usize; let f = e.num("FooterRowCount").unwrap_or(0.0) as usize;