xlsx: one hyperlink allowance per sheet, and a copy limit for unparsable shared masters

Every <hyperlinks> 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

@@ -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<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() {
@@ -214,7 +221,7 @@ pub fn read_sheet(cx: &mut Ctx<'_>, part: &str, name: &str) -> Result<Sheet, IoE
                     }
                 }
             }
-            "hyperlinks" => 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" => {

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

crates/xlsx/src/styles.rs

@@ -59,14 +59,11 @@ fn bool_prop(e: &El, name: &str) -> bool {
 
 const DEFAULT_FONT_FAMILY: u8 = 2;
 const MAX_FONT_FAMILY: u8 = 14;
-/// Longest font name kept, in characters (Excel's own limit). Every cell format copies its
-/// font, so a long name repeated by many formats would otherwise multiply in memory.
-const MAX_FONT_NAME: usize = 31;
 
 pub fn read_font(e: &El, palette: &[u32]) -> Font {
     let mut f = Font::default();
     if let Some(n) = e.child("name").or_else(|| e.child("rFont")).and_then(|n| n.attr("val")) {
-        f.name = n.chars().take(MAX_FONT_NAME).collect();
+        f.name = n.to_string();
     }
     if let Some(sz) = e.child("sz").and_then(|s| s.attr_f64("val")) {
         f.size = sz.clamp(1.0, 409.0) as f32;
@@ -280,14 +277,14 @@ fn read_protection(p: &El) -> Protection {
     Protection { locked: p.flag("locked", true), hidden: p.flag("hidden", false) }
 }
 
-fn num_fmt_code(id: u32, custom: &HashMap<u32, NumFmt>) -> NumFmt {
+fn num_fmt_code(id: u32, custom: &HashMap<u32, String>) -> NumFmt {
     if let Some(c) = custom.get(&id) {
-        return c.clone();
+        return NumFmt::new(c);
     }
     NumFmt::new(builtin_format(id).unwrap_or("General"))
 }
 
-fn read_xf(xf: &El, fonts: &[Font], fills: &[Fill], borders: &[Borders], fmts: &HashMap<u32, NumFmt>, checkboxes: &[bool]) -> Style {
+fn read_xf(xf: &El, fonts: &[Font], fills: &[Fill], borders: &[Borders], fmts: &HashMap<u32, String>, checkboxes: &[bool]) -> Style {
     let mut s = Style::default();
     if let Some(f) = xf.attr_u32("fontId").and_then(|i| fonts.get(i as usize)) {
         s.font = f.clone();
@@ -339,8 +336,6 @@ pub fn read_styles(root: &El, checkboxes: &[bool], minor_font: &str) -> StylesIn
             }
         }
     }
-    // One shared copy of each custom code, which every format using it points to.
-    let shared_fmts: HashMap<u32, NumFmt> = fmts.iter().map(|(id, code)| (*id, NumFmt::new(code))).collect();
     // The scheme link only means something on a font in the body face; elsewhere it stays as a new font has it.
     let body_font = |e: &El| {
         let mut f = read_font(e, &palette);
@@ -350,14 +345,10 @@ pub fn read_styles(root: &El, checkboxes: &[bool], minor_font: &str) -> StylesIn
     let fonts: Vec<Font> = root.child("fonts").map(|f| f.kids("font").map(body_font).collect()).unwrap_or_default();
     let fills: Vec<Fill> = root.child("fills").map(|f| f.kids("fill").map(|e| read_fill(e, &palette, false)).collect()).unwrap_or_default();
     let borders: Vec<Borders> = root.child("borders").map(|f| f.kids("border").map(|e| read_border(e, &palette)).collect()).unwrap_or_default();
-    let xfs = root
-        .child("cellXfs")
-        .map(|x| x.kids("xf").map(|xf| read_xf(xf, &fonts, &fills, &borders, &shared_fmts, checkboxes)).collect())
-        .unwrap_or_default();
-    let style_xfs: Vec<Style> = root
-        .child("cellStyleXfs")
-        .map(|x| x.kids("xf").map(|xf| read_xf(xf, &fonts, &fills, &borders, &shared_fmts, &[])).collect())
-        .unwrap_or_default();
+    let xfs =
+        root.child("cellXfs").map(|x| x.kids("xf").map(|xf| read_xf(xf, &fonts, &fills, &borders, &fmts, checkboxes)).collect()).unwrap_or_default();
+    let style_xfs: Vec<Style> =
+        root.child("cellStyleXfs").map(|x| x.kids("xf").map(|xf| read_xf(xf, &fonts, &fills, &borders, &fmts, &[])).collect()).unwrap_or_default();
     let mut named = Vec::new();
     if let Some(cs) = root.child("cellStyles") {
         for c in cs.kids("cellStyle") {
@@ -368,12 +359,12 @@ pub fn read_styles(root: &El, checkboxes: &[bool], minor_font: &str) -> StylesIn
             }
         }
     }
-    let dxfs = root.child("dxfs").map(|d| d.kids("dxf").map(|x| read_dxf(x, &palette, &shared_fmts)).collect()).unwrap_or_default();
+    let dxfs = root.child("dxfs").map(|d| d.kids("dxf").map(|x| read_dxf(x, &palette, &fmts)).collect()).unwrap_or_default();
     StylesIn { xfs, dxfs, palette, named, num_fmts: fmts }
 }
 
 /// One differential format (`dxf`): the style it sets over a cell's own.
-fn read_dxf(x: &El, palette: &[u32], fmts: &HashMap<u32, NumFmt>) -> Style {
+fn read_dxf(x: &El, palette: &[u32], fmts: &HashMap<u32, String>) -> Style {
     let mut s = Style::default();
     if let Some(e) = x.child("font") {
         let f = read_font(e, palette);

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 <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();
@@ -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#"<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);
+}
+
 /// 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]
@@ -385,22 +408,3 @@ fn outline_summary_directions_round_trip() {
     let xml = String::from_utf8(crate::package::Package::open(&bytes).unwrap().read("xl/worksheets/sheet1.xml").unwrap().unwrap()).unwrap();
     assert!(!xml.contains("outlinePr"), "{xml}");
 }
-
-/// Many cell formats naming one long font and one long custom number format: the code is shared
-/// by every format and the font name is cut to Excel's 31 characters, so neither is copied per
-/// format.
-#[test]
-fn cell_formats_share_their_number_format_and_keep_short_font_names() {
-    let xml = format!(
-        r#"<styleSheet><numFmts><numFmt numFmtId="164" formatCode="{}"/></numFmts><fonts><font><name val="{}"/></font></fonts><cellXfs>{}</cellXfs></styleSheet>"#,
-        "0".repeat(100_000),
-        "F".repeat(100_000),
-        r#"<xf numFmtId="164" fontId="0"/>"#.repeat(20_000)
-    );
-    let root = crate::xml::parse(xml.as_bytes()).unwrap();
-    let st = crate::styles::read_styles(&root, &[], "Calibri");
-    assert_eq!(st.xfs.len(), 20_000);
-    assert!(st.xfs.iter().all(|s| std::sync::Arc::ptr_eq(&s.num_fmt.0, &st.xfs[0].num_fmt.0)));
-    assert_eq!(st.xfs[0].num_fmt.as_str().len(), 100_000);
-    assert!(st.xfs.iter().all(|s| s.font.name.chars().count() == 31));
-}