From 377838556121e4f5bf239c922a0bc11a370c81bb Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 04:26:06 +0000 Subject: [PATCH] xlsx: one hyperlink allowance per sheet, and a copy limit for unparsable shared masters Every element of a worksheet started a fresh cell and byte allowance although all of them fill the same map. The sheet now keeps one allowance across its elements. Followers of a shared master the parser refuses each copied its full text; a sheet full of followers of a long master multiplied that text without bound. The copies are now charged against a 64 MiB per-sheet limit, past which the followers keep their cached values. --- crates/xlsx/src/sheet_read.rs | 27 +++++++++++++++++++++------ crates/xlsx/src/sheet_read/parts.rs | 19 +++++++++++++------ crates/xlsx/src/tests/malformed.rs | 23 +++++++++++++++++++++++ 3 files changed, 57 insertions(+), 12 deletions(-) diff --git a/crates/xlsx/src/sheet_read.rs b/crates/xlsx/src/sheet_read.rs index 73c98386a..5b2ffe61f 100644 --- a/crates/xlsx/src/sheet_read.rs +++ b/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,6 +44,9 @@ 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, dropped: u64, rows: u64, } @@ -138,6 +141,9 @@ 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") { @@ -181,6 +187,7 @@ pub fn read_sheet(cx: &mut Ctx<'_>, part: &str, name: &str) -> Result, part: &str, name: &str) -> Result read_hyperlinks(cx, name, e, &rels, &mut sheet), + "hyperlinks" => read_hyperlinks(cx, name, e, &rels, &mut sheet, &mut hyperlinks_used), "autoFilter" => sheet.autofilter = read_autofilter(cx, e), "printOptions" => { sheet.print.gridlines = e.flag("gridLines", false); @@ -635,14 +642,22 @@ 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; }; - Some(match &sh.expr { + 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; - Formula::from_expr(gridcraft_formula::adjust::shift_relative(e.clone(), dr, dc)) + Some(Formula::from_expr(gridcraft_formula::adjust::shift_relative(e.clone(), dr, dc))) } - None => Formula::new(&sh.text), - }) + 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"); + return None; + } + st.shared_fallback_bytes = total; + Some(Formula::new(&sh.text)) + } + } } } "array" => { diff --git a/crates/xlsx/src/sheet_read/parts.rs b/crates/xlsx/src/sheet_read/parts.rs index 3ff672289..9f053f9a4 100644 --- a/crates/xlsx/src/sheet_read/parts.rs +++ b/crates/xlsx/src/sheet_read/parts.rs @@ -3,9 +3,16 @@ use super::{ NoteBox, NoteCorner, RangeRef, Rel, Sheet, Table, TableColumn, TotalsFn, XmlColumnPr, from_file, xml, }; -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; +/// Hyperlink cells and copied bytes one sheet has kept so far: every `` 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; 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(); @@ -24,7 +31,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; } @@ -32,8 +39,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()); } } diff --git a/crates/xlsx/src/tests/malformed.rs b/crates/xlsx/src/tests/malformed.rs index 0fb9f1b43..0d232187a 100644 --- a/crates/xlsx/src/tests/malformed.rs +++ b/crates/xlsx/src/tests/malformed.rs @@ -244,6 +244,11 @@ 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 elements share the sheet's allowance. + let body = r#""#; + 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#""#; let (wb, rep) = read_xlsx(&minimal(body, &parts, rels, "")).unwrap(); @@ -251,6 +256,24 @@ 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#"{master}1"#); + for r in 2..=2000 { + rows.push_str(&format!(r#"{r}"#)); + } + let (wb, rep) = read_xlsx(&minimal(&format!("{rows}"), &[], "", "")).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); +} + /// 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]