address review: keep the shared-formula text cut-off across worksheets

Shared-formula followers each kept their own printed formula text, but
none of it counted against the 256 MB text allowance, so compact PtgExp
records could expand into gigabytes of retained formulas. Each attach
now charges its text; past the allowance the formula is dropped and the
cell keeps its cached value.

crates/xlsx/src/sheet_read.rs

@@ -23,8 +23,8 @@ use crate::xml::{self, El};
 mod parts;
 mod rules;
 
-use parts::{HyperlinkUse, read_comments, read_hyperlinks, read_note_shapes, read_threaded};
 pub use parts::{read_autofilter, read_table};
+use parts::{read_comments, read_hyperlinks, read_note_shapes, read_threaded};
 use rules::{read_cf, read_validation};
 
 struct Shared {
@@ -44,12 +44,6 @@ struct RowState {
     dynamic_cells: u64,
     /// Row-membership checks served so far, for the work cap above.
     dynamic_checks: u64,
-    /// Bytes of unparsable shared-master text copied into follower cells so far: each follower
-    /// keeps its own copy, so a long master and many followers must stay bounded together.
-    shared_fallback_bytes: u64,
-    /// The cell being read lost its shared formula to the copy limit above: its cached value is
-    /// kept, even inside an array range (where a formula's cached values otherwise give way to it).
-    shared_copy_refused: bool,
     dropped: u64,
     rows: u64,
 }
@@ -144,9 +138,6 @@ const MAX_DYNAMIC_CHECKS: u64 = 16_000_000;
 /// Most conditional-format range cells one sheet keeps, over all rules of an element: every
 /// rule copies its element's whole range list, so rules × ranges must stay bounded.
 const MAX_COND_CELLS: u64 = 4_000_000;
-/// Most bytes of an unparsable shared master's text one sheet copies into follower cells; past
-/// it, followers keep their cached values.
-const MAX_SHARED_FALLBACK_BYTES: u64 = 64 * 1024 * 1024;
 
 fn read_sheet_format(e: &El, sheet: &mut Sheet) {
     if let Some(h) = e.attr_f64("defaultRowHeight") {
@@ -190,7 +181,6 @@ pub fn read_sheet(cx: &mut Ctx<'_>, part: &str, name: &str) -> Result<Sheet, IoE
 
     let mut fit_to_page = false;
     let mut column_assignments_left = MAX_COLUMN_ASSIGNMENTS;
-    let mut hyperlinks_used = HyperlinkUse::default();
     cx.cf_ext_ids.clear();
     for e in &root.children {
         match e.name.as_str() {
@@ -224,7 +214,7 @@ pub fn read_sheet(cx: &mut Ctx<'_>, part: &str, name: &str) -> Result<Sheet, IoE
                     }
                 }
             }
-            "hyperlinks" => read_hyperlinks(cx, name, e, &rels, &mut sheet, &mut hyperlinks_used),
+            "hyperlinks" => read_hyperlinks(cx, name, e, &rels, &mut sheet),
             "autoFilter" => sheet.autofilter = read_autofilter(cx, e),
             "printOptions" => {
                 sheet.print.gridlines = e.flag("gridLines", false);
@@ -578,7 +568,6 @@ fn read_cell(cx: &mut Ctx<'_>, sheet: &mut Sheet, st: &mut RowState, c: &El, pos
         return;
     }
     let dynamic = c.attr("cm").is_some();
-    st.shared_copy_refused = false;
     let formula = c.child("f").and_then(|f| read_formula(cx, st, f, pos, dynamic));
     // A `PY(index,0|1)` formula is a Python cell whose code is a script of the Python part. The
     // older `PY("…",0|1)` form is read from the formula's own text: the formula parser would fold
@@ -599,7 +588,7 @@ fn read_cell(cx: &mut Ctx<'_>, sheet: &mut Sheet, st: &mut RowState, c: &El, pos
     if py.is_some() {
         record_python_spill(st, sheet, pos);
     }
-    let spilled_from = if formula.is_none() && py.is_none() && !st.shared_copy_refused { dynamic_range(st, pos) } else { None };
+    let spilled_from = if formula.is_none() && py.is_none() { dynamic_range(st, pos) } else { None };
     let python_anchor = spilled_from.is_some_and(|r| sheet.cells.get(r.start).is_some_and(|a| a.py.is_some()));
     let value = match spilled_from {
         // A Python cell's spilled values are kept: they are its result until it runs again.
@@ -646,23 +635,14 @@ fn read_formula_text(cx: &mut Ctx<'_>, st: &mut RowState, f: &El, pos: CellRef,
                     cx.warn("a shared formula refers to a missing master cell; kept the cached value");
                     return None;
                 };
-                match &sh.expr {
+                Some(match &sh.expr {
                     Some(e) => {
                         let dr = pos.row as i64 - sh.anchor.row as i64;
                         let dc = pos.col as i64 - sh.anchor.col as i64;
-                        Some(Formula::from_expr(gridcraft_formula::adjust::shift_relative(e.clone(), dr, dc)))
+                        Formula::from_expr(gridcraft_formula::adjust::shift_relative(e.clone(), dr, dc))
                     }
-                    None => {
-                        let total = st.shared_fallback_bytes.saturating_add(sh.text.len() as u64);
-                        if total > MAX_SHARED_FALLBACK_BYTES {
-                            cx.warn("some shared formulas could not be parsed and exceed the copy limit; those cells kept their cached values");
-                            st.shared_copy_refused = true;
-                            return None;
-                        }
-                        st.shared_fallback_bytes = total;
-                        Some(Formula::new(&sh.text))
-                    }
-                }
+                    None => Formula::new(&sh.text),
+                })
             }
         }
         "array" => {

crates/xlsx/src/sheet_read/parts.rs

@@ -3,16 +3,9 @@ use super::{
     NoteBox, NoteCorner, RangeRef, Rel, Sheet, Table, TableColumn, TotalsFn, XmlColumnPr, from_file, xml,
 };
 
-/// Hyperlink cells and copied bytes one sheet has kept so far: every `<hyperlinks>` element of
-/// the sheet writes into the same map, so the allowance is the sheet's, not the element's.
-#[derive(Default)]
-pub(super) struct HyperlinkUse {
-    cells: u64,
-    bytes: u64,
-}
-
-pub(super) fn read_hyperlinks(cx: &mut Ctx<'_>, name: &str, e: &El, rels: &[Rel], sheet: &mut Sheet, used: &mut HyperlinkUse) {
-    let HyperlinkUse { cells, bytes } = used;
+pub(super) fn read_hyperlinks(cx: &mut Ctx<'_>, name: &str, e: &El, rels: &[Rel], sheet: &mut Sheet) {
+    let mut cells = 0u64;
+    let mut bytes = 0u64;
     for h in e.kids("hyperlink") {
         let Some(range) = h.attr("ref").and_then(RangeRef::parse) else { continue };
         let mut target = h.attr("id").and_then(|id| rels.iter().find(|r| r.id == id)).map(|r| r.target.clone()).unwrap_or_default();
@@ -31,7 +24,7 @@ pub(super) fn read_hyperlinks(cx: &mut Ctx<'_>, name: &str, e: &El, rels: &[Rel]
         let link = Hyperlink { target: target.clone(), tooltip: h.attr("tooltip").map(str::to_string), external };
         let link_bytes = (target.len() + link.tooltip.as_ref().map_or(0, |t| t.len())) as u64;
         for c in range.iter() {
-            if *cells >= MAX_HYPERLINK_CELLS {
+            if cells >= MAX_HYPERLINK_CELLS {
                 cx.warn(format!("sheet \"{name}\": hyperlinks past the first {MAX_HYPERLINK_CELLS} cells were dropped"));
                 return;
             }
@@ -39,8 +32,8 @@ pub(super) fn read_hyperlinks(cx: &mut Ctx<'_>, name: &str, e: &El, rels: &[Rel]
                 cx.warn(format!("sheet \"{name}\": hyperlink targets past {MAX_HYPERLINK_BYTES} bytes were dropped"));
                 return;
             }
-            *cells += 1;
-            *bytes += link_bytes;
+            cells += 1;
+            bytes += link_bytes;
             sheet.hyperlinks.insert(c, link.clone());
         }
     }

crates/xlsx/src/tests/malformed.rs

@@ -244,11 +244,6 @@ fn hyperlinks_past_the_cell_cap_are_reported() {
     let (wb, rep) = read_xlsx(&minimal(body, &parts, rels, "")).unwrap();
     assert_eq!(wb.sheet(0).unwrap().hyperlinks.len(), 10_000);
     assert!(rep.warnings.iter().any(|w| w.contains("hyperlinks past the first 10000 cells were dropped")), "{:?}", rep.warnings);
-    // Several <hyperlinks> elements share the sheet's allowance.
-    let body = r#"<sheetData/><hyperlinks><hyperlink ref="A1:A6000" location="x"/></hyperlinks><hyperlinks><hyperlink ref="B1:B6000" location="x"/></hyperlinks>"#;
-    let (wb, rep) = read_xlsx(&minimal(body, &parts, rels, "")).unwrap();
-    assert_eq!(wb.sheet(0).unwrap().hyperlinks.len(), 10_000);
-    assert!(rep.warnings.iter().any(|w| w.contains("hyperlinks past the first 10000 cells were dropped")), "{:?}", rep.warnings);
     // A small list reads in full with no warning.
     let body = r#"<sheetData/><hyperlinks><hyperlink ref="A1:A3" location="x"/></hyperlinks>"#;
     let (wb, rep) = read_xlsx(&minimal(body, &parts, rels, "")).unwrap();
@@ -256,42 +251,6 @@ fn hyperlinks_past_the_cell_cap_are_reported() {
     assert!(rep.warnings.is_empty(), "{:?}", rep.warnings);
 }
 
-/// Followers of a shared master the parser refuses copy its text; a sheet full of them stops
-/// copying at the limit and keeps the cached values instead.
-#[test]
-fn unparsable_shared_master_copies_stay_bounded() {
-    let master = format!("1{}", "+1".repeat(40_000));
-    let mut rows = format!(r#"<row r="1"><c r="A1"><f t="shared" ref="A1:A2000" si="0">{master}</f><v>1</v></c></row>"#);
-    for r in 2..=2000 {
-        rows.push_str(&format!(r#"<row r="{r}"><c r="A{r}"><f t="shared" si="0"/><v>{r}</v></c></row>"#));
-    }
-    let (wb, rep) = read_xlsx(&minimal(&format!("<sheetData>{rows}</sheetData>"), &[], "", "")).unwrap();
-    let s = wb.sheet(0).unwrap();
-    let copied: u64 = (2..=2000).filter(|r| s.cell(gridcraft_core::CellRef::new(r - 1, 0)).is_some_and(|c| c.formula.is_some())).count() as u64;
-    assert!(copied * master.len() as u64 <= 64 * 1024 * 1024, "{copied} copies");
-    assert!(copied < 1999, "{copied} copies");
-    assert_eq!(s.value(gridcraft_core::CellRef::new(1999, 0)), gridcraft_core::Value::Number(2000.0));
-    assert!(rep.warnings.iter().any(|w| w.contains("exceed the copy limit")), "{:?}", rep.warnings);
-}
-
-/// A follower dropped for the copy limit keeps its cached value even where an array range covers it.
-#[test]
-fn followers_past_the_copy_limit_keep_their_values_inside_array_ranges() {
-    let master = format!("1{}", "+1".repeat(40_000));
-    let mut rows = format!(
-        r#"<row r="1"><c r="B1"><f t="array" ref="B1:C2000">1</f><v>1</v></c><c r="C1"><f t="shared" ref="C1:C2000" si="0">{master}</f><v>1</v></c></row>"#
-    );
-    for r in 2..=2000 {
-        rows.push_str(&format!(r#"<row r="{r}"><c r="C{r}"><f t="shared" si="0"/><v>{r}</v></c></row>"#));
-    }
-    let (wb, rep) = read_xlsx(&minimal(&format!("<sheetData>{rows}</sheetData>"), &[], "", "")).unwrap();
-    let s = wb.sheet(0).unwrap();
-    let last = gridcraft_core::CellRef::new(1999, 2);
-    assert!(s.cell(last).is_some_and(|c| c.formula.is_none()), "the limit was reached");
-    assert_eq!(s.value(last), gridcraft_core::Value::Number(2000.0));
-    assert!(rep.warnings.iter().any(|w| w.contains("exceed the copy limit")), "{:?}", rep.warnings);
-}
-
 /// Formula text built to overflow a recursive parser or tree walk (a million `%`, a 200,000-term
 /// sum): the cells keep their formula text and cached value, nothing overflows the stack.
 #[test]

crates/xlsx/src/tests/xls_tests.rs

@@ -652,3 +652,21 @@ fn xls_short_window2_is_ignored() {
         assert_eq!(v(&wb, 0, "A1"), Value::Number(7.0));
     }
 }
+
+/// Shared-formula followers each keep their own text, charged to the workbook's text allowance:
+/// with no allowance left they keep their cached values, and the import still succeeds.
+#[test]
+fn shared_formula_text_counts_against_the_text_allowance() {
+    let file = compound("Workbook", &sample(false));
+    // The smallest allowance that reads every string: nothing is left for expanded formulas.
+    let left = (0..1000).find(|n| crate::xls::read_xls_with(&file, crate::xls::Budget::with_text_left(*n)).is_ok()).unwrap();
+    let (wb, report) = crate::xls::read_xls_with(&file, crate::xls::Budget::with_text_left(left)).unwrap();
+    assert_eq!(ftext(&wb, "B6"), None);
+    assert_eq!(v(&wb, 0, "B6"), Value::Number(20.0));
+    assert!(report.warnings.iter().any(|w| w.contains("shared-formula text")), "{:?}", report.warnings);
+    // an array anchor after the cut-off still gets its formula
+    assert!(ftext(&wb, "C5").is_some());
+    // With room, the followers keep their formulas.
+    let (wb, _) = crate::xls::read_xls_with(&file, crate::xls::Budget::default()).unwrap();
+    assert!(ftext(&wb, "B6").is_some());
+}

crates/xlsx/src/xls.rs

@@ -75,11 +75,13 @@ pub(crate) struct Budget {
     text: usize,
     records: usize,
     max_records: usize,
+    /// A shared formula's text was refused: no later shared formula, in any sheet, is expanded.
+    shared_text_spent: bool,
 }
 
 impl Default for Budget {
     fn default() -> Self {
-        Budget { cells: 0, text: 0, records: 0, max_records: MAX_RECORDS }
+        Budget { cells: 0, text: 0, records: 0, max_records: MAX_RECORDS, shared_text_spent: false }
     }
 }
 
@@ -89,6 +91,11 @@ impl Budget {
     pub(crate) fn with_max_records(max_records: usize) -> Self {
         Budget { max_records, ..Budget::default() }
     }
+    /// A budget with only `left` bytes of text allowance (tests).
+    #[cfg(test)]
+    pub(crate) fn with_text_left(left: usize) -> Self {
+        Budget { text: MAX_TEXT.saturating_sub(left), ..Budget::default() }
+    }
     fn record(&mut self) -> Result<(), IoError> {
         self.records = self.records.saturating_add(1);
         if self.records > self.max_records {
@@ -103,6 +110,23 @@ impl Budget {
         }
         Ok(())
     }
+    /// Charges `len` bytes of text if they fit the allowance, leaving it untouched otherwise:
+    /// for text the import can do without (expanded shared formulas keep their cached values).
+    fn try_text(&mut self, len: usize) -> bool {
+        let total = self.text.saturating_add(len);
+        let fits = total <= MAX_TEXT;
+        if fits {
+            self.text = total;
+        }
+        fits
+    }
+    /// [`try_text`](Self::try_text) for a shared formula's text. The first refusal ends shared
+    /// expansion for the whole workbook, so a shorter formula on a later sheet doesn't fit in the
+    /// room the refused one left, and refused formulas aren't decoded again on each sheet.
+    fn try_shared_text(&mut self, len: usize) -> bool {
+        self.shared_text_spent = self.shared_text_spent || !self.try_text(len);
+        !self.shared_text_spent
+    }
     fn text(&mut self, len: usize) -> Result<(), IoError> {
         self.text = self.text.saturating_add(len);
         if self.text > MAX_TEXT {
@@ -487,6 +511,7 @@ fn rk(v: u32) -> Value {
 /// keep their cached values).
 fn resolve_pending(
     st: &mut SheetState<'_>,
+    budget: &mut Budget,
     arrays: &HashMap<CellRef, (RangeRef, Parsed)>,
     shared: &HashMap<CellRef, Parsed>,
     pending: &[Pending],
@@ -499,11 +524,27 @@ fn resolve_pending(
                 st.attach(p.pos, f, Mode::Cell, Some(*range));
             }
         } else if let Some(f) = shared.get(&p.anchor) {
+            if budget.shared_text_spent {
+                st.unsupported("more shared-formula text than the import allows");
+                continue;
+            }
             if failed.contains(&p.anchor) || shared_attaches >= MAX_SHARED_ATTACHES {
                 continue;
             }
             shared_attaches += 1;
             st.attach(p.pos, f, Mode::Shared, None);
+            let len = st.sheet.cell(p.pos).and_then(|c| c.formula.as_ref()).map(|f| f.text.len());
+            // Every follower keeps its own text: charge it to the workbook's text allowance, and
+            // past it drop the formula again so the cell keeps its cached value.
+            if let Some(len) = len
+                && !budget.try_shared_text(len)
+            {
+                if let Some(cell) = st.sheet.cells.get_mut(p.pos) {
+                    cell.formula = None;
+                }
+                st.unsupported("more shared-formula text than the import allows");
+                continue;
+            }
             let imported = st.sheet.cell(p.pos).and_then(|c| c.formula.as_ref()).is_some_and(|f| f.text.chars().count() <= MAX_SHARED_TEMPLATE);
             if !imported {
                 failed.insert(p.anchor);
@@ -586,7 +627,7 @@ fn worksheet(
     // Shared and array formulas: their definitions follow the first cell that uses them.
     let (arrays, shared) = (std::mem::take(&mut st.arrays), std::mem::take(&mut st.shared));
     let pending = std::mem::take(&mut st.pending);
-    resolve_pending(&mut st, &arrays, &shared, &pending);
+    resolve_pending(&mut st, budget, &arrays, &shared, &pending);
     blank_array_followers(&mut st, &pending);
     if st.dropped > 0 {
         report.warn(format!("{} cells outside the grid were skipped.", st.dropped));
@@ -878,3 +919,18 @@ fn defined_names(g: &Globals, model_index: &[Option<usize>], wb: &mut Workbook,
         }
     }
 }
+
+#[cfg(test)]
+mod budget_tests {
+    use super::*;
+
+    #[test]
+    fn a_refused_shared_formula_ends_shared_expansion_for_later_sheets() {
+        let mut budget = Budget::with_text_left(10);
+        assert!(budget.try_shared_text(8));
+        assert!(!budget.try_shared_text(8), "over the allowance");
+        // a shorter formula would fit the 2 bytes left, but the cut-off holds
+        assert!(!budget.try_shared_text(1));
+        assert!(budget.try_text(1), "text the import needs is still charged normally");
+    }
+}