A spent cell budget stops reading tables, rather than keeping one each

Keeping a 1 x 1 table for every further `<Table>` was unbounded in the one
direction the budget exists to close. `<Table></Table>` is fifteen bytes, so
the 512 MiB a part may decompress to holds some 36 million of them, and a
`Cell` is 1,360 bytes: tens of gigabytes, with every table inside both caps.
The test I wrote said as much without my noticing -- its invariant was
`reserved <= MAX + tables`, and `tables` had no bound.

`budgeted_size` now returns `None` once the budget is spent and the table is
not read at all, so the cells one import reserves are at most the budget,
exactly. The invariant is now `reserved <= MAX_IMPORT_TABLE_CELLS`, with no
term that grows with the file.

The warnings needed the same treatment, for the same reason: a file of 400,000
tables each claiming one row and two columns got a line about every one of
them. At most 20 are named, then one line saying the rest were the same, and
the file is told once that its tables ran out of cells between them.

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<Swatch>,
@@ -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,

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()));

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 `<Table>`
+/// was unbounded in the one direction the budget was meant to close: `<Table></Table>` 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> {
     /// `<Table>`: 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<Table> {
         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 `<Table></Table>` 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}");
         }
     }
 }