From 7726e9c6effb18d4d4ab3444cc3a66c2e87550e7 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 08:28:28 -0500 Subject: [PATCH] xls: charge expanded shared-formula text to the import's text allowance (#3256) * xls: charge expanded shared-formula text to the import's text allowance 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. * address review: keep resolving array formulas after the shared-formula text allowance is spent * address review: keep the shared-formula text cut-off across worksheets * address review: expand shared formulas only after every sheet's mandatory text is charged * address review: refuse duplicate sheet names at import; adapt tests to read_xls_budgeted --- crates/xlsx/src/tests/xls_tests.rs | 69 +++++++++++++++++++++ crates/xlsx/src/xls.rs | 96 ++++++++++++++++++++++++++---- 2 files changed, 154 insertions(+), 11 deletions(-) diff --git a/crates/xlsx/src/tests/xls_tests.rs b/crates/xlsx/src/tests/xls_tests.rs index 762e512e0..d12cc7961 100644 --- a/crates/xlsx/src/tests/xls_tests.rs +++ b/crates/xlsx/src/tests/xls_tests.rs @@ -761,3 +761,72 @@ 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_budgeted(&file, crate::xls::Budget::with_text_left(*n), Default::default()).is_ok()).unwrap(); + let (wb, report) = crate::xls::read_xls_budgeted(&file, crate::xls::Budget::with_text_left(left), Default::default()).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_budgeted(&file, crate::xls::Budget::default(), Default::default()).unwrap(); + assert!(ftext(&wb, "B6").is_some()); +} + +/// Shared-formula text is optional: an early sheet's expanded formulas must not use the allowance +/// a later sheet's inline strings need, or the whole import fails for text that fits. +#[test] +fn shared_formulas_do_not_starve_a_later_sheets_text() { + let shared = cat(&[&[0x4C], &u16s(&[0, 0xFFFF]), &ptg_int(10), &[0x05]]); + let boundsheet = |pos: u32, name: &str| rec(0x0085, &cat(&[&pos.to_le_bytes(), &[0, 0, name.len() as u8, 0], name.as_bytes()])); + let globals = |p: [u32; 2]| cat(&[&bof(0x0005), &boundsheet(p[0], "One"), &boundsheet(p[1], "Two"), &eof()]); + let one = cat(&[ + &bof(0x0010), + &formula(4, 1, num(10.0), &exp(4, 1)), + &rec(0x04BC, &cat(&[&u16s(&[4, 5]), &[1, 1, 0, 2], &(shared.len() as u16).to_le_bytes(), &shared])), + &formula(5, 1, num(20.0), &exp(4, 1)), + &eof(), + ]); + let two = cat(&[&bof(0x0010), &rec(0x0204, &cat(&[&cell(0, 0, 0), &xlstr("abcdefgh")])), &eof()]); + let p1 = globals([0; 2]).len() as u32; + let file = compound("Workbook", &cat(&[&globals([p1, p1 + one.len() as u32]), &one, &two])); + // The smallest allowance that reads every string leaves nothing for expanded formulas. + let left = (0..1000).find(|n| crate::xls::read_xls_budgeted(&file, crate::xls::Budget::with_text_left(*n), Default::default()).is_ok()).unwrap(); + let (wb, _) = crate::xls::read_xls_budgeted(&file, crate::xls::Budget::with_text_left(left), Default::default()).unwrap(); + assert_eq!(v(&wb, 1, "A1"), Value::text("abcdefgh")); + assert_eq!(ftext(&wb, "B6"), None); + assert_eq!(v(&wb, 0, "B6"), Value::Number(20.0)); + let (wb, _) = crate::xls::read_xls_budgeted(&file, crate::xls::Budget::default(), Default::default()).unwrap(); + assert!(ftext(&wb, "B6").is_some()); +} + +/// Two readable worksheets with the same name are refused at import, as they were when each sheet +/// was added to the workbook as it was read. +#[test] +fn xls_duplicate_sheet_names_are_refused() { + let boundsheet = |name: &str| rec(0x0085, &cat(&[&0u32.to_le_bytes(), &[0, 0, name.len() as u8, 0], name.as_bytes()])); + let sheet = cat(&[&bof(0x0010), &eof()]); + let globals = |a: &str, b: &str| cat(&[&bof(0x0005), &boundsheet(a), &boundsheet(b), &eof()]); + let file = |a: &str, b: &str| { + let p = globals(a, b).len() as u32; + let mut g = cat(&[ + &bof(0x0005), + &rec(0x0085, &cat(&[&p.to_le_bytes(), &[0, 0, a.len() as u8, 0], a.as_bytes()])), + &rec(0x0085, &cat(&[&(p + sheet.len() as u32).to_le_bytes(), &[0, 0, b.len() as u8, 0], b.as_bytes()])), + &eof(), + ]); + g.extend(&sheet); + g.extend(&sheet); + compound("Workbook", &g) + }; + let err = crate::xls::read_xls_budgeted(&file("One", "one"), crate::xls::Budget::default(), Default::default()).unwrap_err().to_string(); + assert!(err.contains("already taken"), "{err}"); + assert!(crate::xls::read_xls_budgeted(&file("One", "Two"), crate::xls::Budget::default(), Default::default()).is_ok()); +} diff --git a/crates/xlsx/src/xls.rs b/crates/xlsx/src/xls.rs index 1c9285bae..9049c6b12 100644 --- a/crates/xlsx/src/xls.rs +++ b/crates/xlsx/src/xls.rs @@ -78,11 +78,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 } } } @@ -92,6 +94,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 { @@ -106,6 +113,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 { @@ -182,6 +206,9 @@ pub(crate) fn read_xls_budgeted(bytes: &[u8], mut budget: Budget, functions: Fun // BoundSheet8 index → model sheet index. let mut model_index: Vec> = Vec::with_capacity(g.sheets.len()); + let mut parsed = Vec::new(); + // Names of the readable sheets so far: `wb.sheets` is only filled once every sheet is parsed. + let mut names: Vec<&str> = Vec::new(); for bs in &g.sheets { if bs.kind != 0 { report.warn(match bs.kind { @@ -193,17 +220,26 @@ pub(crate) fn read_xls_budgeted(bytes: &[u8], mut budget: Budget, functions: Fun continue; } wb.check_sheet_name(&bs.name, None).map_err(invalid)?; - let Some((mut sheet, calc)) = worksheet(&stream, bs, &g, &styles, &mut budget, &mut report)? else { + if names.iter().any(|n| n.eq_ignore_ascii_case(&bs.name)) { + return Err(invalid("That name is already taken. Try a different one.")); + } + let Some(st) = worksheet(&stream, bs, &g, &styles, &mut budget, &mut report)? else { report.warn("Dialog sheets and unreadable sheets were skipped."); model_index.push(None); continue; }; - sheet.visibility = bs.visibility; + names.push(&bs.name); + model_index.push(Some(parsed.len())); + parsed.push((bs.visibility, st)); + } + // Every sheet's mandatory text is charged by now; shared formulas take what is left. + for (visibility, st) in parsed { + let (mut sheet, calc) = finish_sheet(st, &mut budget, &mut report); + sheet.visibility = visibility; // Every worksheet repeats the workbook's calculation settings; Excel applies the first. if wb.sheets.is_empty() { wb.calc = calc; } - model_index.push(Some(wb.sheets.len())); wb.sheets.push(Arc::new(sheet)); } if wb.sheets.is_empty() { @@ -508,6 +544,7 @@ fn rk(v: u32) -> Value { /// keep their cached values). fn resolve_pending( st: &mut SheetState<'_>, + budget: &mut Budget, arrays: &HashMap, shared: &HashMap, pending: &[Pending], @@ -520,11 +557,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); @@ -552,14 +605,14 @@ fn blank_array_followers(st: &mut SheetState<'_>, pending: &[Pending]) { } } -fn worksheet( +fn worksheet<'g>( stream: &[u8], bs: &BoundSheet, - g: &Globals, - styles: &[StyleId], + g: &'g Globals, + styles: &'g [StyleId], budget: &mut Budget, report: &mut ReadReport, -) -> Result, IoError> { +) -> Result>, IoError> { if bs.pos >= stream.len() { return Err(invalid(format!("sheet '{}' points outside the Workbook stream", bs.name))); } @@ -604,10 +657,16 @@ fn worksheet( } sheet_record(&rec, &mut st, budget, report)?; } - // Shared and array formulas: their definitions follow the first cell that uses them. + Ok(Some(st)) +} + +/// Attaches a sheet's shared and array formulas (their definitions follow the first cell that +/// uses them). Run for every sheet only after all sheets' mandatory text is charged, so the +/// optional shared-formula text can't use up the allowance a later sheet's strings need. +fn finish_sheet(mut st: SheetState<'_>, budget: &mut Budget, report: &mut ReadReport) -> (Sheet, CalcSettings) { 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)); @@ -615,7 +674,7 @@ fn worksheet( for why in &st.unsupported { report.warn(format!("Some formulas use {why}; their saved results were kept as values.")); } - Ok(Some((st.sheet, st.calc))) + (st.sheet, st.calc) } /// The shortest WINDOW2 a file holds (the chart-sheet form); a shorter one is truncated. @@ -899,3 +958,18 @@ fn defined_names(g: &Globals, model_index: &[Option], 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"); + } +}