From ff35c6b89db7c3572292579803da4b876af15ec7 Mon Sep 17 00:00:00 2001 From: Michael Higgins <55243596+Higgs32584@users.noreply.github.com> Date: Fri, 6 Nov 2026 04:52:53 -0500 Subject: [PATCH] xlsx: stop memory amplification from declared sizes (#2173) (#2197) * xlsx: stop memory amplification from declared sizes (#2173) Hyperlinks: drop targets over 8,192 bytes before copying them into each cell, cut ScreenTips to 255 characters, and count the 10,000-cell cap across all blocks of a sheet. Conditional formats: cap the sqref copies made by extra rules at 1M ranges per sheet. Styles: build each custom number format once and share its Arc; cut font names to 255 characters. Shared formulas: keep cached values for dependents of over-long masters, and charge each expansion against a 2 GiB per-workbook budget. Co-Authored-By: Claude Opus 5.5 * Import the hyperlink caps in sheet_read/parts after the merge Upstream switched parts.rs to an explicit super:: import list. Co-Authored-By: Claude Opus 5.5 * xlsx: charge conditional-format ranges per sheet; cap number format codes (#2173) - read_cf keeps a cumulative per-sheet count of the range cells retained rules hold, instead of multiplying the current block by the rule count, and charges only rules that are kept. - Custom and inline number format codes longer than 255 characters (Excel's limit) are dropped with a warning; their cells show General. - The shared NumFmt map is built only for ids a cell or differential format refers to. Co-Authored-By: Claude Opus 5.5 * xlsx: keep over-long number format codes as stored (#2173) an_overlong_number_format_code_does_not_stop_the_open requires a code past 255 characters to round-trip; sharing one NumFmt per referenced id already removes the amplification. Keep the referenced-id sharing. Co-Authored-By: Claude Opus 5.5 * xlsx: bound only what a shared formula costs beyond an ordinary cell; count characters for the length warning Review follow-up for #2197 (@skylartaylor): - Each dependent of a shared formula now pays only its cost beyond an ordinary formula cell (SHARED_FORMULA_FREE_COST, 4 KiB by shared_cost), and the per-workbook budget for that excess drops from 2 GiB to 256 MiB. Many ordinary shared formulas still all expand; a long master copied into thousands of cells stops at the budget and keeps the cached values. - The over-long-master warning now uses the parser's own test (bytes, then characters), so an unparseable multibyte master no longer gets a false length warning. Refs #2173 --------- Co-authored-by: Claude Opus 5.5 Co-authored-by: FredApps --- crates/xlsx/src/read.rs | 4 + crates/xlsx/src/sheet_read.rs | 119 ++++++++++++++++++++- crates/xlsx/src/sheet_read/parts.rs | 35 ++++--- crates/xlsx/src/sheet_read/rules.rs | 18 ++-- crates/xlsx/src/styles.rs | 49 +++++++-- crates/xlsx/src/tests/malformed.rs | 154 ++++++++++++++++++++++++++++ 6 files changed, 344 insertions(+), 35 deletions(-) diff --git a/crates/xlsx/src/read.rs b/crates/xlsx/src/read.rs index 93efb7f02..5bdc98216 100644 --- a/crates/xlsx/src/read.rs +++ b/crates/xlsx/src/read.rs @@ -35,6 +35,9 @@ pub struct Ctx<'a> { pub cell_pictures: Vec>>, /// x14 conditional-format ids of the current sheet's data bars, with the index of the rule in `cond_formats`. pub cf_ext_ids: Vec<(String, usize)>, + /// Bytes charged so far for shared formulas expanded into dependent cells (see + /// [`crate::sheet_read::MAX_SHARED_FORMULA_BYTES`]). + pub shared_formula_bytes: u64, /// Whether GridCraft calculates a function, when the caller says: cell formulas wrapped by /// Google Sheets' export are then read as the formula inside (see [`crate::read_xlsx_with`]). pub known_function: Option<&'a dyn Fn(&str) -> bool>, @@ -255,6 +258,7 @@ pub fn read_xlsx<'a>(bytes: &'a [u8], known_function: Option<&'a dyn Fn(&str) -> num_fmts: Default::default(), cell_pictures: vec![], cf_ext_ids: vec![], + shared_formula_bytes: 0, known_function, python: Default::default(), }; diff --git a/crates/xlsx/src/sheet_read.rs b/crates/xlsx/src/sheet_read.rs index f2047b6ee..e68d14389 100644 --- a/crates/xlsx/src/sheet_read.rs +++ b/crates/xlsx/src/sheet_read.rs @@ -31,6 +31,16 @@ struct Shared { anchor: CellRef, expr: Option, text: String, + /// Approximate bytes one dependent cell costs (its printed text plus its expression tree); + /// what exceeds [`SHARED_FORMULA_FREE_COST`] is charged against [`MAX_SHARED_FORMULA_BYTES`]. + cost: u64, +} + +/// What one sheet's hyperlinks have copied so far, across all its `` blocks. +#[derive(Default)] +struct LinkBudget { + cells: u64, + bytes: u64, } #[derive(Default)] @@ -120,10 +130,23 @@ fn settle_python_spills_past_budget(st: &RowState, sheet: &mut Sheet) { } } -const MAX_HYPERLINK_CELLS: u64 = 10_000; /// Target and tooltip bytes kept for hyperlinks of one sheet, over all cells: the same URL is /// copied for every cell of its range, so length × cells must stay bounded whatever the file. const MAX_HYPERLINK_BYTES: u64 = 8 * 1024 * 1024; +/// Most cells one sheet's hyperlinks may cover (each cell gets its own copy of the link). +const MAX_HYPERLINK_CELLS: u64 = 10_000; +/// Longest hyperlink target kept, in bytes (Excel's own limit is about 2,079 characters). +const MAX_HYPERLINK_TARGET: usize = 8_192; +/// Longest hyperlink ScreenTip kept, in characters (Excel's limit). +const MAX_TOOLTIP_CHARS: usize = 255; +/// What a dependent of a shared formula may cost for free: an ordinary formula cell's worth (by +/// [`shared_cost`], a formula of about 40 operators and operands), so a sheet of many ordinary +/// shared formulas is never cut short however many cells share them. +const SHARED_FORMULA_FREE_COST: u64 = 4096; +/// Budget, per workbook, for what shared formulas cost beyond [`SHARED_FORMULA_FREE_COST`] per +/// dependent cell: a long master copied into many cells is where the expansion multiplies the +/// file's bytes, and this bounds it to well within a web build's memory. +pub const MAX_SHARED_FORMULA_BYTES: u64 = 256 << 20; const MAX_COLUMN_ASSIGNMENTS: u64 = 1_000_000; /// Most dynamic-array and legacy-array ranges of one sheet. const MAX_DYNAMIC_RANGES: usize = 65_536; @@ -182,6 +205,8 @@ pub fn read_sheet(cx: &mut Ctx<'_>, part: &str, name: &str) -> Result<(Sheet, cr drop(bytes); let mut fit_to_page = false; + let mut link_budget = LinkBudget::default(); + let mut cf_kept_cells = 0u64; let mut column_assignments_left = MAX_COLUMN_ASSIGNMENTS; cx.cf_ext_ids.clear(); for e in &root.children { @@ -208,7 +233,7 @@ pub fn read_sheet(cx: &mut Ctx<'_>, part: &str, name: &str) -> Result<(Sheet, cr } } } - "conditionalFormatting" => read_cf(cx, e, &mut sheet), + "conditionalFormatting" => read_cf(cx, e, &mut sheet, &mut cf_kept_cells), "dataValidations" => { for v in e.kids("dataValidation") { if let Some(v) = read_validation(v) { @@ -216,7 +241,7 @@ pub fn read_sheet(cx: &mut Ctx<'_>, part: &str, name: &str) -> Result<(Sheet, cr } } } - "hyperlinks" => read_hyperlinks(cx, name, e, &rels, &mut sheet), + "hyperlinks" => read_hyperlinks(cx, name, e, &rels, &mut sheet, &mut link_budget), "autoFilter" => sheet.autofilter = read_autofilter(cx, e), "printOptions" => { sheet.print.gridlines = e.flag("gridLines", false); @@ -641,13 +666,30 @@ fn read_formula_text(cx: &mut Ctx<'_>, st: &mut RowState, f: &El, pos: CellRef, let si = f.attr_u32("si")?; if !text.is_empty() { let expr = gridcraft_formula::parse(text).ok(); - st.shared.insert(si, Shared { anchor: pos, expr, text: text.to_string() }); + let cost = shared_cost(text, expr.as_ref()); + st.shared.insert(si, Shared { anchor: pos, expr, text: text.to_string(), cost }); Some(Formula::new(text)) } else { let Some(sh) = st.shared.get(&si) else { cx.warn("a shared formula refers to a missing master cell; kept the cached value"); return None; }; + // The parser's own test: bytes first (cheap), then characters, so a multibyte + // master that failed to parse for another reason gets no false length warning. + if sh.expr.is_none() + && sh.text.len() > gridcraft_formula::MAX_FORMULA_LEN + && sh.text.chars().count() > gridcraft_formula::MAX_FORMULA_LEN + { + cx.warn(format!( + "a shared formula is longer than {} characters; kept the cached values of the cells that share it", + gridcraft_formula::MAX_FORMULA_LEN + )); + return None; + } + if !charge(&mut cx.shared_formula_bytes, extra_shared_cost(sh.cost), MAX_SHARED_FORMULA_BYTES) { + cx.warn("shared formulas expand to too much data; kept the cached values of the remaining cells that share them"); + return None; + } Some(match &sh.expr { Some(e) => { let dr = pos.row as i64 - sh.anchor.row as i64; @@ -720,6 +762,41 @@ pub fn parse_iso_datetime(s: &str, sys: DateSystem) -> Option { Some(serial) } +/// What one dependent of a shared formula costs: its printed text plus its expression tree. +fn shared_cost(text: &str, expr: Option<&Expr>) -> u64 { + let mut nodes = 0u64; + if let Some(e) = expr { + e.walk(&mut |_| nodes = nodes.saturating_add(1)); + } + let node = std::mem::size_of::() as u64; + (text.len() as u64).saturating_mul(2).saturating_add(nodes.saturating_mul(node)).saturating_add(64) +} + +/// What one dependent of a shared formula costing `cost` is charged: its cost beyond an ordinary +/// formula cell's. +fn extra_shared_cost(cost: u64) -> u64 { + cost.saturating_sub(SHARED_FORMULA_FREE_COST) +} + +/// Adds `cost` to `used` when that stays within `cap`; `false` (and `used` unchanged) otherwise. +fn charge(used: &mut u64, cost: u64, cap: u64) -> bool { + match used.checked_add(cost) { + Some(total) if total <= cap => { + *used = total; + true + } + _ => false, + } +} + +/// The first `max` characters of `s`. +fn truncate_chars(s: &str, max: usize) -> String { + match s.char_indices().nth(max) { + Some((at, _)) => s.get(..at).unwrap_or(s).to_string(), + None => s.to_string(), + } +} + pub fn parse_sqref(s: Option<&str>) -> Vec { s.unwrap_or("").split_whitespace().filter_map(RangeRef::parse).collect() } @@ -780,6 +857,40 @@ fn read_ext(cx: &mut Ctx<'_>, e: &El, sheet: &mut Sheet) { mod tests { use super::*; + #[test] + fn shared_formula_budget_stops_expansion() { + let expr = gridcraft_formula::parse("SUM(1,2,3)").ok(); + let cost = shared_cost("SUM(1,2,3)", expr.as_ref()); + assert!(cost > 64 + 4 * std::mem::size_of::() as u64); + let cap = cost * 3; + let mut used = 0; + assert!((0..3).all(|_| charge(&mut used, cost, cap))); + assert!(!charge(&mut used, cost, cap)); + assert_eq!(used, cap); + assert!(!charge(&mut used, u64::MAX, u64::MAX)); + } + + #[test] + fn ordinary_shared_formulas_are_free_and_long_ones_pay_their_excess() { + let text = r#"IF(AND(A1>0,B1<>""),VLOOKUP(C1,Sheet2!$A$1:$D$500,4,FALSE)*SUM(D1:D10)/COUNT(D1:D10),IFERROR(E1/F1,0))"#; + let expr = gridcraft_formula::parse(text).ok(); + assert!(expr.is_some()); + assert_eq!(extra_shared_cost(shared_cost(text, expr.as_ref())), 0, "an ordinary formula costs nothing extra"); + let long = vec![format!("SUM({}1)", "1,".repeat(199)); 10].join("+"); + let expr = gridcraft_formula::parse(&long).ok(); + assert!(expr.is_some()); + let cost = shared_cost(&long, expr.as_ref()); + assert_eq!(extra_shared_cost(cost), cost - SHARED_FORMULA_FREE_COST); + // A few thousand copies of a long master no longer fit the budget. + assert!(extra_shared_cost(cost).saturating_mul(10_000) > MAX_SHARED_FORMULA_BYTES); + } + + #[test] + fn truncate_keeps_whole_characters() { + assert_eq!(truncate_chars("ab\u{e9}d", 3), "ab\u{e9}"); + assert_eq!(truncate_chars("ab", 3), "ab"); + } + #[test] fn iso_dates() { assert_eq!(parse_iso_datetime("1900-01-01", DateSystem::D1900), Some(1.0)); diff --git a/crates/xlsx/src/sheet_read/parts.rs b/crates/xlsx/src/sheet_read/parts.rs index 3ff672289..309ab4a02 100644 --- a/crates/xlsx/src/sheet_read/parts.rs +++ b/crates/xlsx/src/sheet_read/parts.rs @@ -1,17 +1,25 @@ use super::{ - AutoFilter, CellRef, Comment, Ctx, El, FilterCriterion, HashMap, Hyperlink, MAX_COLS, MAX_HYPERLINK_BYTES, MAX_HYPERLINK_CELLS, MAX_ROWS, - NoteBox, NoteCorner, RangeRef, Rel, Sheet, Table, TableColumn, TotalsFn, XmlColumnPr, from_file, xml, + AutoFilter, CellRef, Comment, Ctx, El, FilterCriterion, HashMap, Hyperlink, LinkBudget, MAX_COLS, MAX_HYPERLINK_BYTES, MAX_HYPERLINK_CELLS, + MAX_HYPERLINK_TARGET, MAX_ROWS, MAX_TOOLTIP_CHARS, NoteBox, NoteCorner, RangeRef, Rel, Sheet, Table, TableColumn, TotalsFn, XmlColumnPr, + from_file, truncate_chars, 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; +/// `budget` counts the cells and bytes this sheet's hyperlinks have copied so far, across +/// `` blocks. +pub(super) fn read_hyperlinks(cx: &mut Ctx<'_>, name: &str, e: &El, rels: &[Rel], sheet: &mut Sheet, budget: &mut LinkBudget) { 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(); + let rel_target = h.attr("id").and_then(|id| rels.iter().find(|r| r.id == id)).map(|r| r.target.as_str()).unwrap_or(""); + let location = h.attr("location").filter(|l| !l.is_empty()); + // Each covered cell gets its own copy of the link: refuse oversized targets before copying. + if rel_target.len().saturating_add(location.map_or(0, str::len)) > MAX_HYPERLINK_TARGET { + cx.warn(format!("sheet \"{name}\": hyperlinks longer than {MAX_HYPERLINK_TARGET} characters were dropped")); + continue; + } + let mut target = rel_target.to_string(); // A relationship is a file or web page; a bare `location` is a place in this workbook. let external = !target.is_empty(); - if let Some(loc) = h.attr("location").filter(|l| !l.is_empty()) { + if let Some(loc) = location { if target.is_empty() { target = loc.trim_start_matches('#').to_string(); } else { @@ -21,19 +29,20 @@ pub(super) fn read_hyperlinks(cx: &mut Ctx<'_>, name: &str, e: &El, rels: &[Rel] if target.is_empty() { continue; } - 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; + let tooltip = h.attr("tooltip").map(|t| truncate_chars(t, MAX_TOOLTIP_CHARS)); + let link_bytes = (target.len() + tooltip.as_ref().map_or(0, String::len)) as u64; + let link = Hyperlink { target, tooltip, external }; for c in range.iter() { - if cells >= MAX_HYPERLINK_CELLS { + if budget.cells >= MAX_HYPERLINK_CELLS { cx.warn(format!("sheet \"{name}\": hyperlinks past the first {MAX_HYPERLINK_CELLS} cells were dropped")); return; } - if bytes.saturating_add(link_bytes) > MAX_HYPERLINK_BYTES { + if budget.bytes.saturating_add(link_bytes) > MAX_HYPERLINK_BYTES { cx.warn(format!("sheet \"{name}\": hyperlink targets past {MAX_HYPERLINK_BYTES} bytes were dropped")); return; } - cells += 1; - bytes += link_bytes; + budget.cells += 1; + budget.bytes += link_bytes; sheet.hyperlinks.insert(c, link.clone()); } } diff --git a/crates/xlsx/src/sheet_read/rules.rs b/crates/xlsx/src/sheet_read/rules.rs index 4e4500707..c3a3f04f6 100644 --- a/crates/xlsx/src/sheet_read/rules.rs +++ b/crates/xlsx/src/sheet_read/rules.rs @@ -45,11 +45,15 @@ fn cfvos(e: &El) -> Vec { .collect() } -pub(super) fn read_cf(cx: &mut Ctx<'_>, e: &El, sheet: &mut Sheet) { +/// One `conditionalFormatting` block. Every rule keeps its own copy of the block's ranges, so +/// `kept_cells` counts the range cells the sheet's retained rules hold so far, across blocks; a +/// rule that would take it past [`MAX_COND_CELLS`] is dropped. Unsupported rules cost nothing. +pub(super) fn read_cf(cx: &mut Ctx<'_>, e: &El, sheet: &mut Sheet, kept_cells: &mut u64) { let ranges = parse_sqref(e.attr("sqref")); if ranges.is_empty() { return; } + let block_cells = ranges.iter().map(RangeRef::count).fold(0u64, u64::saturating_add); for r in e.kids("cfRule") { let style = Box::new(r.attr_u32("dxfId").and_then(|i| cx.dxfs.get(i as usize)).cloned().unwrap_or_default()); let formulas: Vec = r.kids("formula").map(|f| from_file(f.text.trim())).collect(); @@ -111,15 +115,15 @@ pub(super) fn read_cf(cx: &mut Ctx<'_>, e: &El, sheet: &mut Sheet) { continue; } }; - if let Some(id) = ext_id { - cx.cf_ext_ids.push((id, sheet.cond_formats.len())); - } - let rules = sheet.cond_formats.len(); - let area = ranges.iter().map(RangeRef::count).sum::().saturating_mul(1 + rules as u64); - if area > MAX_COND_CELLS { + let total = kept_cells.saturating_add(block_cells); + if total > MAX_COND_CELLS { cx.warn(format!("conditional formats past {MAX_COND_CELLS} kept range cells were dropped")); break; } + *kept_cells = total; + if let Some(id) = ext_id { + cx.cf_ext_ids.push((id, sheet.cond_formats.len())); + } sheet.cond_formats.push(CondFormat { ranges: ranges.clone(), rule, diff --git a/crates/xlsx/src/styles.rs b/crates/xlsx/src/styles.rs index c1f93aca9..04c9fcdf2 100644 --- a/crates/xlsx/src/styles.rs +++ b/crates/xlsx/src/styles.rs @@ -1,7 +1,7 @@ //! `styles.xml`: reading cell formats and differential formats into model styles, and writing //! the workbook's style table. -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::fmt::Write as _; use gridcraft_model::style::{DxfStated, Protection}; @@ -14,6 +14,9 @@ use crate::xml::{El, esc_attr, num}; // ---------------------------------------------------------------- reading +/// Longest font name kept, in characters (Excel's own limit is 31). +pub const MAX_FONT_NAME_CHARS: usize = 255; + /// Styles read from a file. #[derive(Debug, Default)] pub struct StylesIn { @@ -63,7 +66,11 @@ const MAX_FONT_FAMILY: u8 = 14; 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.to_string(); + // Every cell format that uses the font copies its name: keep it short. + f.name = match n.char_indices().nth(MAX_FONT_NAME_CHARS) { + Some((at, _)) => n.get(..at).unwrap_or_default().to_string(), + None => 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; @@ -277,14 +284,16 @@ 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) -> NumFmt { +/// A number format by id. Custom formats are built once and shared (`NumFmt` is an `Arc`), so +/// many cell formats naming one long code don't each copy it. +fn num_fmt_code(id: u32, custom: &HashMap) -> NumFmt { if let Some(c) = custom.get(&id) { - return NumFmt::new(c); + return c.clone(); } NumFmt::new(builtin_format(id).unwrap_or("General")) } -fn read_xf(xf: &El, fonts: &[Font], fills: &[Fill], borders: &[Borders], fmts: &HashMap, checkboxes: &[bool]) -> Style { +fn read_xf(xf: &El, fonts: &[Font], fills: &[Fill], borders: &[Borders], fmts: &HashMap, 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(); @@ -336,6 +345,20 @@ pub fn read_styles(root: &El, checkboxes: &[bool], minor_font: &str) -> StylesIn } } } + // One shared `NumFmt` per custom id a cell format or differential format refers to (`NumFmt` + // is an `Arc`); declared but unused codes get no second copy. + let mut used: HashSet = HashSet::new(); + for list in ["cellXfs", "cellStyleXfs"] { + if let Some(x) = root.child(list) { + used.extend(x.kids("xf").map(|xf| xf.attr_u32("numFmtId").unwrap_or(0))); + } + } + if let Some(d) = root.child("dxfs") { + used.extend( + d.kids("dxf").filter_map(|x| x.child("numFmt")).filter(|n| n.attr("formatCode").is_none()).filter_map(|n| n.attr_u32("numFmtId")), + ); + } + let shared_fmts: HashMap = fmts.iter().filter(|(id, _)| used.contains(id)).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); @@ -345,10 +368,14 @@ pub fn read_styles(root: &El, checkboxes: &[bool], minor_font: &str) -> StylesIn let fonts: Vec = root.child("fonts").map(|f| f.kids("font").map(body_font).collect()).unwrap_or_default(); let fills: Vec = root.child("fills").map(|f| f.kids("fill").map(|e| read_fill(e, &palette, false)).collect()).unwrap_or_default(); let borders: Vec = 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, &fmts, checkboxes)).collect()).unwrap_or_default(); - let style_xfs: Vec