diff --git a/crates/idml/src/import.rs b/crates/idml/src/import.rs index c8e0512b5..ae7364647 100644 --- a/crates/idml/src/import.rs +++ b/crates/idml/src/import.rs @@ -142,6 +142,11 @@ struct Importer<'r> { /// Cells every table in this import has reserved. One table is capped, but a file chooses /// how many tables it has, so the cap has to be document-wide as well (noted in review). table_cells: usize, + /// Tables whose size the file claimed and did not get, warned about so far. A file chooses + /// how many tables it has, so a line each is its own flood (noted in review). + table_warnings: usize, + /// Whether the file has already been told its tables ran out of cells between them. + table_budget_spent: bool, settings: DocSettings, default_margins: Option<(Margins, Columns)>, swatches: Vec, @@ -229,6 +234,8 @@ impl<'r> Importer<'r> { previews, next_id: 0, table_cells: 0, + table_warnings: 0, + table_budget_spent: false, settings: DocSettings::default(), default_margins: None, swatches: specials, diff --git a/crates/idml/src/import/story.rs b/crates/idml/src/import/story.rs index 694a2dbbd..72cde49b1 100644 --- a/crates/idml/src/import/story.rs +++ b/crates/idml/src/import/story.rs @@ -168,8 +168,11 @@ impl<'r> Importer<'r> { b.fresh = true; } "Table" => { - let t = self.table(c); - b.push_table(t, pf, cf); + // `None` once the file-wide cell budget is spent: the table is not read + // at all rather than kept as a 1 x 1 (noted in review). + if let Some(t) = self.table(c) { + b.push_table(t, pf, cf); + } } "HyperlinkTextDestination" | "ParagraphDestination" => { b.anchors.push((c.get("Self").unwrap_or("").to_string(), c.get("Name").unwrap_or("Anchor").to_string())); diff --git a/crates/idml/src/import/table.rs b/crates/idml/src/import/table.rs index 3b1051b66..91ac287f9 100644 --- a/crates/idml/src/import/table.rs +++ b/crates/idml/src/import/table.rs @@ -7,22 +7,31 @@ use super::*; /// importer collects at most. const MAX_IMPORT_TABLE_CELLS: usize = 400_000; +/// Clamp warnings one import writes before it says the rest were the same. +const MAX_TABLE_WARNINGS: usize = 20; + /// How large a table of `rows` x `cols` may be read when `reserved` cells have already been -/// taken, and whether the file-wide budget is what cut it down. +/// taken, and whether the file-wide budget is what cut it down -- or `None` when the budget is +/// spent and the table is not read at all. /// /// The per-table cap leaves a file free to hold many capped tables, so each is also clamped -/// against what is left. One row and one column are always kept: the table is then still in the -/// document, still says where its text went, and a file pays one cell for a table it really -/// does contain -- which its own size already bounds. -fn budgeted_size(rows: usize, cols: usize, reserved: usize) -> (usize, usize, bool) { +/// against what is left. While there is budget, one row and one column are always kept: the table +/// is then still in the document, still says where its text went, and a file pays one cell for a +/// table it really does contain. +/// +/// Past that there is nothing left to pay with. Keeping a 1 x 1 table for every further `` +/// was unbounded in the one direction the budget was meant to close: `
` is fifteen +/// bytes, so the 512 MiB a part may decompress to holds some 36 million of them, and a `Cell` +/// each is tens of gigabytes with every table inside both caps (noted in review). +fn budgeted_size(rows: usize, cols: usize, reserved: usize) -> Option<(usize, usize, bool)> { let (mut nr, mut nc) = designcraft_doc::table::clamped_size(rows, cols); - let left = MAX_IMPORT_TABLE_CELLS.saturating_sub(reserved); + let left = MAX_IMPORT_TABLE_CELLS.checked_sub(reserved).filter(|left| *left > 0)?; let over_budget = nr.saturating_mul(nc) > left; if over_budget { - nc = nc.min(left.max(1)); + nc = nc.min(left); nr = nr.min((left / nc).max(1)); } - (nr, nc, over_budget) + Some((nr, nc, over_budget)) } /// Preserve the legacy uniform field only when all four sides explicitly specify every @@ -45,8 +54,7 @@ pub(super) fn uniform_cell_stroke(edges: &[designcraft_doc::CellStrokeAttrs; 4]) impl<'r> Importer<'r> { /// ``: rows, columns and cells (cell `Name` is `column:row`). - pub(super) fn table(&mut self, e: &El) -> Table { - let id = self.alloc(); + pub(super) fn table(&mut self, e: &El) -> Option
{ let rows: Vec<&El> = e.find_all("Row").collect(); let cols: Vec<&El> = e.find_all("Column").collect(); // A file's row and column counts are its claim, and a table of them is allocated whole: a @@ -57,12 +65,31 @@ impl<'r> Importer<'r> { // // Reserved before `Table::new`, so a cell's own story cannot spend the budget this table // is counting on. - let (nr, nc, over_budget) = budgeted_size(rows.len().max(1), cols.len().max(1), self.table_cells); + let Some((nr, nc, over_budget)) = budgeted_size(rows.len().max(1), cols.len().max(1), self.table_cells) else { + // Said once: the file that reaches this is the one with millions of tables in it. + if !self.table_budget_spent { + self.table_budget_spent = true; + self.warnings.push(format!( + "the tables in this file claim more than {MAX_IMPORT_TABLE_CELLS} cells between them: the rest of them were not read" + )); + } + return None; + }; self.table_cells = self.table_cells.saturating_add(nr.saturating_mul(nc)); if nr < rows.len() || nc < cols.len() { let why = if over_budget { "more than the tables in one file can hold between them" } else { "more than a table can hold" }; - self.warnings.push(format!("a table of {} rows and {} columns was read as {nr} x {nc}: {why}", rows.len(), cols.len())); + self.table_warnings += 1; + match self.table_warnings.cmp(&MAX_TABLE_WARNINGS) { + std::cmp::Ordering::Less => { + self.warnings.push(format!("a table of {} rows and {} columns was read as {nr} x {nc}: {why}", rows.len(), cols.len())) + } + std::cmp::Ordering::Equal => { + self.warnings.push(format!("more than {MAX_TABLE_WARNINGS} tables were read smaller than this file claims")) + } + std::cmp::Ordering::Greater => {} + } } + let id = self.alloc(); 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); @@ -151,7 +178,7 @@ impl<'r> Importer<'r> { for rg in regions { let _ = t.merge(rg); } - t + Some(t) } /// A cell's local fill, insets, alignment, rotation and edge strokes. @@ -207,45 +234,52 @@ mod budget_tests { fn the_tables_in_one_file_are_capped_between_them() { let mut reserved = 0; let mut sizes = Vec::new(); - for _ in 0..3 { - let (nr, nc, over) = budgeted_size(500, 1000, reserved); - reserved += nr * nc; - sizes.push((nr, nc, over)); + for _ in 0..4 { + let size = budgeted_size(500, 1000, reserved); + if let Some((nr, nc, _)) = size { + reserved += nr * nc; + } + sizes.push(size); } - assert_eq!(sizes[0], (250, 1000, false), "the first table is held by the per-table cap alone"); - assert_eq!(sizes[1], (150, 1000, true), "the second gets what is left of the file's budget"); - assert_eq!(sizes[2], (1, 1, true), "and the third is kept, so its text still has somewhere to be"); - assert_eq!(reserved, MAX_IMPORT_TABLE_CELLS + 1, "400,000 cells and the one the last table keeps"); + assert_eq!(sizes[0], Some((250, 1000, false)), "the first table is held by the per-table cap alone"); + assert_eq!(sizes[1], Some((150, 1000, true)), "the second gets what is left of the file's budget"); + assert_eq!(sizes[2], None, "and the third is not read: there is nothing left to pay for it"); + assert_eq!(sizes[3], None, "nor any after it"); + assert_eq!(reserved, MAX_IMPORT_TABLE_CELLS, "the budget is spent exactly, not overspent"); } - /// The budget holds for any sequence of claimed sizes, and a table is never read as nothing: - /// a 0 x n table has no cells to hold the text the file puts in it. + /// The budget is a hard limit, not a floor: keeping one cell for every further table was + /// unbounded in the one direction the budget exists to close, since a `
` is + /// fifteen bytes and a part may decompress to 512 MiB (noted in review). #[test] - fn a_table_is_never_read_as_empty_and_the_budget_always_holds() { + fn a_spent_budget_stops_reading_tables_altogether() { let claims = [(0, 0), (1, 1), (7, 3), (usize::MAX, usize::MAX), (1, usize::MAX), (usize::MAX, 1), (10_000, 1_000), (2, 0)]; let mut reserved = 0usize; - let mut tables = 0usize; + let mut read = 0usize; + let mut skipped = 0usize; // Every claim, several times over, in an order that exhausts the budget part way through. for _ in 0..4 { for (rows, cols) in claims { - let (nr, nc, _) = budgeted_size(rows.max(1), cols.max(1), reserved); - assert!(nr >= 1 && nc >= 1, "{rows} x {cols} was read as {nr} x {nc}"); - reserved += nr * nc; - tables += 1; - assert!( - reserved <= MAX_IMPORT_TABLE_CELLS + tables, - "{tables} tables reserved {reserved} cells, past the budget by more than the one cell each keeps" - ); + match budgeted_size(rows.max(1), cols.max(1), reserved) { + Some((nr, nc, _)) => { + assert!(nr >= 1 && nc >= 1, "{rows} x {cols} was read as {nr} x {nc}: a table has nowhere to put its text"); + reserved += nr * nc; + read += 1; + } + None => skipped += 1, + } + assert!(reserved <= MAX_IMPORT_TABLE_CELLS, "{read} tables reserved {reserved} cells, past the budget"); } } - assert!(reserved >= MAX_IMPORT_TABLE_CELLS, "this sequence does reach the budget: {reserved}"); + assert_eq!(reserved, MAX_IMPORT_TABLE_CELLS, "this sequence does reach the budget"); + assert!(skipped > 0, "and does go on asking after it: {skipped} tables were not read"); } /// Below the budget nothing is clamped by it, so an ordinary file is read exactly as before. #[test] fn an_ordinary_file_is_not_touched_by_the_budget() { for (rows, cols) in [(1, 1), (2, 3), (50, 20), (250, 1000)] { - assert_eq!(budgeted_size(rows, cols, 0), (rows, cols, false), "{rows} x {cols}"); + assert_eq!(budgeted_size(rows, cols, 0), Some((rows, cols, false)), "{rows} x {cols}"); } } }