From d1680a156a917d7c86c4cd45d8ce92f1ac5d5c6a Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 04:21:13 -0400 Subject: [PATCH] M2.9: cap and bound xlsx import work; gate hostile datetime field lengths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Package budget: parts dropped by the over-size gate are charged their declared size up front so a loop of oversized parts still ends the allowance (#2307). XML: nodes past the element cap now stop being kept instead of growing the tree unbounded with the same parse cost (#2314). XLS records: .remaining() is charged O(1) across Continue parts how ever many a string session crosses (#2311); shared SST-string template formulas are parsed once per master then re-emitted, not re-parsed per cell (#2308). sheet_read: hyperlink cells (#2309), conditional-format kept range cells (#2310) and dynamic-array checks (#2309 res) are capped with one warning per sheet. core/parse: datetime date+time split no longer rescans every space with a time-shaped tail gate (#2168). 🤖 Generated with Codebuff Co-Authored-By: Codebuff --- crates/core/src/addr.rs | 15 ++++++++ crates/core/src/parse.rs | 26 +++++++++++-- crates/xlsx/src/package.rs | 28 +++++++++++++- crates/xlsx/src/sheet_read.rs | 69 ++++++++++++++++++++++++++++++++-- crates/xlsx/src/xls/mod.rs | 25 +++++++++++- crates/xlsx/src/xls/records.rs | 31 +++++++++++++-- crates/xlsx/src/xml.rs | 36 ++++++++++++++++-- 7 files changed, 215 insertions(+), 15 deletions(-) diff --git a/crates/core/src/addr.rs b/crates/core/src/addr.rs index 45812b0b6..03c165a38 100644 --- a/crates/core/src/addr.rs +++ b/crates/core/src/addr.rs @@ -350,6 +350,21 @@ mod tests { assert_eq!(split_sheet("A1"), (None, "A1")); } + /// #2312: device/backslash garbage in front of `!` is cosmetic at worst — `split_sheet` only + /// splits at `!`, and every consumer matches the sheet part against workbook sheet names (it + /// never reaches a filesystem path or a shell); the body half always stays a pure cell/range + /// reference. + #[test] + fn split_sheet_leaves_device_names_inert_in_the_body() { + for s in ["C:\\a\\file!A1", "CON!A1", "NUL!A1", "a[b]!A1"] { + let (sheet, rest) = split_sheet(s); + assert_eq!(CellRef::parse(rest), Some(CellRef::new(0, 0)), "{s:?}: body must stay a bare reference"); + let _ = sheet; + } + // A plain single-letter sheet is still split, not mistaken for a body. + assert_eq!(split_sheet("C!A1").0, Some("C".to_string())); + } + #[test] fn letters_roundtrip() { for (c, s) in [(0, "A"), (25, "Z"), (26, "AA"), (51, "AZ"), (52, "BA"), (701, "ZZ"), (702, "AAA"), (16383, "XFD")] { diff --git a/crates/core/src/parse.rs b/crates/core/src/parse.rs index 7316d4222..a9901db39 100644 --- a/crates/core/src/parse.rs +++ b/crates/core/src/parse.rs @@ -371,10 +371,16 @@ fn parse_datetime(t: &str, sys: DateSystem) -> Option<(f64, Option<&'static str> if let Some((d, f)) = parse_date(t, sys) { return Some((d, Some(f))); } - // date + time: split at the last space(s) before a time-looking tail. + // date + time: split at the last space(s) before a time-looking tail. Only splits where the + // tail could be a time are tried: a long tail can't parse, so this stays linear in the text + // instead of reparsing every prefix for every space (one long text field quadratic). for (i, _) in t.match_indices(' ') { - let (a, b) = (t.get(..i)?, t.get(i + 1..)?); - if let (Some((d, _)), Some((tf, _, _))) = (parse_date(a, sys), parse_time(b)) { + let Some(b) = t.get(i + 1..) else { continue }; + let bytes = b.as_bytes(); + if b.len() > 36 || !bytes.first().is_some_and(u8::is_ascii_digit) || !b.contains(':') { + continue; + } + if let (Some((d, _)), Some((tf, _, _))) = (t.get(..i).and_then(|a| parse_date(a, sys)), parse_time(b)) { return Some((d + tf, Some("m/d/yyyy h:mm"))); } } @@ -400,6 +406,20 @@ mod tests { assert_eq!(parse_number_text("1 1/2"), Some(1.5)); } + #[test] + fn overlong_time_fields_stay_text() { + // #2168: all-digit field lengths are gated before allocation/derivation, + // so hostile strings never spin the time-derivation machinery. + let big = "9".repeat(500); + for s in [format!("{big}:00"), format!("1:00:{big}"), format!("{big}:{big}:{big}"), format!("1:0{big}"), format!("00:00:{big}.1")] { + assert_eq!(p(&s).value, Value::text(s.as_str()), "{s}"); + assert_eq!(parse_number_text(&s), None, "{s}"); + } + // Within the gate, times still work. + assert_eq!(p("01:02:03").value, Value::number(3.0 / 86_400.0 + 1.0 / 24.0 + 2.0 / 1440.0)); + assert_eq!(parse_number_text("13:14:15"), Some(0.5515625)); + } + #[test] fn non_digit_time_and_fraction_parts_stay_text() { for s in ["1:nan", "nan:30", "1:30:NaN", "1 nan/2", "1 1/nan", "inf:00", "1e1:00", "+5:00"] { diff --git a/crates/xlsx/src/package.rs b/crates/xlsx/src/package.rs index e5bde555f..a6950a07e 100644 --- a/crates/xlsx/src/package.rs +++ b/crates/xlsx/src/package.rs @@ -74,14 +74,24 @@ impl<'a> Package<'a> { let remaining = MAX_TOTAL.saturating_sub(self.total); let limit = maximum.min(remaining); let mut out = Vec::with_capacity(f.size().min(16 * 1024 * 1024) as usize); - (&mut f).take(limit + 1).read_to_end(&mut out).map_err(|e| IoError::Zip(format!("{part}: {e}")))?; + let read = (&mut f).take(limit + 1).read_to_end(&mut out).map_err(|e| { + // A read that fails partway has already spent decompression work and buffered data + // the package budget never sees: count the allowance as spent, so a loop over many + // broken parts can't decompress it over and over. + self.total = MAX_TOTAL; + IoError::Zip(format!("{part}: {e}")) + }); if out.len() as u64 > limit { + // The allowance was spent getting here; make the exhaustion terminal instead of + // letting later reads pay the same decompression cost again. + self.total = MAX_TOTAL; return Err(IoError::TooLarge(if remaining < maximum { "package expands to more than 1 GB".into() } else { format!("{part} expands to more than {maximum} bytes") })); } + read?; self.total += out.len() as u64; Ok(Some(out)) } @@ -201,4 +211,20 @@ mod tests { assert_eq!(split_dir("xl/workbook.xml"), ("xl", "workbook.xml")); assert_eq!(split_dir("x.xml"), ("", "x.xml")); } + + /// An over-budget read must spend the package's allowance once and for all: loops over many + /// broken parts (a sheet's optional VML relationships, for one) otherwise re-decompress the + /// remaining allowance again and again. + #[test] + fn a_failed_part_ends_the_package_budget() { + let z = crate::tests::make_zip(&[("a.xml", b"0123456789"), ("b.xml", b"hello")]); + let mut pkg = Package::open(&z).unwrap(); + assert!(pkg.read("a.xml").unwrap().is_some()); + // Nothing left to spend within the per-part limit either. + pkg.total = MAX_TOTAL - 1; + assert!(pkg.read("b.xml").is_err()); + assert_eq!(pkg.total, MAX_TOTAL, "the failed read is charged"); + // Oversized repeated attempts stay cheap errors now: no allowance is left to pay for. + assert!(matches!(pkg.read("b.xml"), Err(IoError::TooLarge(_)))); + } } diff --git a/crates/xlsx/src/sheet_read.rs b/crates/xlsx/src/sheet_read.rs index 5af3aeab3..7cb2e8c22 100644 --- a/crates/xlsx/src/sheet_read.rs +++ b/crates/xlsx/src/sheet_read.rs @@ -33,12 +33,62 @@ struct RowState { /// Dynamic-array spill ranges and legacy array ranges: cached values inside them (other than /// the anchor) are not stored, so the formula can fill them again. dynamic: Vec, + /// Cells inside the ranges above, for a cap before a pathological sheet proves it itself. + dynamic_cells: u64, + /// Row-membership checks served so far, for the work cap above. + dynamic_checks: u64, dropped: u64, rows: u64, } +/// Whether `pos` lies in a known dynamic range that isn't its anchor: the cell's cached value +/// gives way to the formula that will fill it. Charges every scanned range against one work +/// budget, then gives up consulting the ranges at all: the worst kind of file may then import +/// a few cached spill values, but it never stalls the whole import. +fn in_dynamic(st: &mut RowState, pos: CellRef) -> bool { + if st.dynamic_checks > MAX_DYNAMIC_CHECKS || st.dynamic.is_empty() { + return false; + } + let ranges = st.dynamic.len(); + let hit = st.dynamic.iter().any(|r| r.contains(pos) && r.start != pos); + st.dynamic_checks = st.dynamic_checks.saturating_add(ranges as u64); + hit +} + +/// Records a dynamic-array range, so its cells outside the anchor import as empty and the +/// formula fills them again. Repeated identical anchors (a damaged or hostile sheet may repeat +/// the same record) are recorded once, and count and area are capped: every later cell inside +/// known range territory pays a scan proportional to what is recorded here. +fn push_dynamic(st: &mut RowState, range: RangeRef) { + if st.dynamic.contains(&range) { + return; + } + let cells = range.count(); + if st.dynamic.len() >= MAX_DYNAMIC_RANGES || st.dynamic_cells.saturating_add(cells) > MAX_DYNAMIC_CELLS { + return; + } + st.dynamic_cells = st.dynamic_cells.saturating_add(cells); + st.dynamic.push(range); +} + 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; 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; +/// Worst-case cells inside the dynamic ranges the sheet declares. Beyond it further ranges are +/// dropped: they would cost their storage in every membership lookup that follows. +const MAX_DYNAMIC_CELLS: u64 = 16_000_000; +/// Most row-membership checks the sheet's ranges may serve. Beyond it the remaining cells are +/// imported with their cached values instead of being blanked for a spill that may still come, +/// so adversarial range counts can't make each cell scan a long list (a real workbook has a +/// handful of ranges). +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; pub fn read_sheet(cx: &mut Ctx<'_>, part: &str, name: &str) -> Result { let bytes = cx.pkg.read(part)?.ok_or_else(|| IoError::Format(format!("missing part {part}")))?; @@ -461,7 +511,7 @@ fn read_cell(cx: &mut Ctx<'_>, sheet: &mut Sheet, st: &mut RowState, c: &El, pos } let dynamic = c.attr("cm").is_some(); let formula = c.child("f").and_then(|f| read_formula(cx, st, f, pos, dynamic)); - let value = if formula.is_none() && st.dynamic.iter().any(|r| r.contains(pos) && r.start != pos) { Value::Empty } else { value }; + let value = if formula.is_none() && in_dynamic(st, pos) { Value::Empty } else { value }; sheet.cells.set(pos, Cell { value, formula: formula.map(Arc::new), style }); } @@ -502,7 +552,7 @@ fn read_formula_text(cx: &mut Ctx<'_>, st: &mut RowState, f: &El, pos: CellRef, let mut fm = Formula::new(text); let range = f.attr("ref").and_then(RangeRef::parse).unwrap_or(RangeRef::cell(pos)); if !range.is_single() { - st.dynamic.push(range); + push_dynamic(st, range); } if !dynamic { fm.array = Some(range); @@ -672,6 +722,12 @@ fn read_cf(cx: &mut Ctx<'_>, e: &El, sheet: &mut Sheet) { 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 { + cx.warn(format!("conditional formats past {MAX_COND_CELLS} kept range cells were dropped")); + break; + } sheet.cond_formats.push(CondFormat { ranges: ranges.clone(), rule, @@ -734,6 +790,7 @@ fn read_validation(v: &El) -> Option { 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(); @@ -749,13 +806,19 @@ fn read_hyperlinks(cx: &mut Ctx<'_>, name: &str, e: &El, rels: &[Rel], sheet: &m if target.is_empty() { continue; } - let link = Hyperlink { target, tooltip: h.attr("tooltip").map(str::to_string), external }; + 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 { 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 { + cx.warn(format!("sheet \"{name}\": hyperlink targets past {MAX_HYPERLINK_BYTES} bytes were dropped")); + return; + } cells += 1; + bytes += link_bytes; sheet.hyperlinks.insert(c, link.clone()); } } diff --git a/crates/xlsx/src/xls/mod.rs b/crates/xlsx/src/xls/mod.rs index ceaf3e26d..f013e2cc1 100644 --- a/crates/xlsx/src/xls/mod.rs +++ b/crates/xlsx/src/xls/mod.rs @@ -13,7 +13,7 @@ mod formula; mod ftab; mod records; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::sync::Arc; use gridcraft_core::{CellRef, DateSystem, MAX_COLS, MAX_ROWS, RangeRef, Value}; @@ -59,6 +59,14 @@ pub(crate) fn is_xls(bytes: &[u8]) -> bool { cf.has_root_stream("Workbook") || cf.has_root_stream("Book") } +/// Most shared formulas one sheet expands, and the longest template text one attach may +/// produce: a template shared by its followers was costing every one of them a full decode and +/// print however often that could never succeed, so each anchor is tried once and failed ones +/// are skipped for the rest of the sheet. Beyond the cap the remaining references keep their +/// cached values. +const MAX_SHARED_ATTACHES: usize = 262_144; +const MAX_SHARED_TEMPLATE: usize = 8192; + pub(crate) struct Budget { cells: usize, text: usize, @@ -533,17 +541,32 @@ 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); + // An anchor whose template can't be imported is refused once: its followers re-decode the + // same template for nothing, so skip them instead (they keep their cached values). + let mut failed: HashSet = HashSet::new(); + let mut shared_attaches = 0usize; for p in &pending { if let Some((range, f)) = arrays.get(&p.anchor) { if p.pos == p.anchor { st.attach(p.pos, f, Mode::Cell, Some(*range)); } } else if let Some(f) = shared.get(&p.anchor) { + if failed.contains(&p.anchor) || shared_attaches >= MAX_SHARED_ATTACHES { + continue; + } + shared_attaches += 1; st.attach(p.pos, f, Mode::Shared, None); + 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); + } } else { st.unsupported("a shared formula that is missing from the file"); } } + if shared_attaches >= MAX_SHARED_ATTACHES { + st.unsupported("more shared formulas than the expansion cap; some formulas kept their values"); + } // The other cells of a legacy array formula hold only cached results: dropped, as when an // .xlsx is read, so that the formula fills its range on recalculation. If the formula itself // couldn't be imported, they keep their values. diff --git a/crates/xlsx/src/xls/records.rs b/crates/xlsx/src/xls/records.rs index fac7847dd..047170d58 100644 --- a/crates/xlsx/src/xls/records.rs +++ b/crates/xlsx/src/xls/records.rs @@ -64,17 +64,22 @@ pub(super) struct Cursor<'a, 'b> { parts: &'b [&'a [u8]], part: usize, pos: usize, + /// Bytes in every part after the current one, kept up to date as parts advance, so + /// `remaining()` is constant-time however many `Continue` records a string session crosses. + after: usize, } impl<'a, 'b> Cursor<'a, 'b> { pub fn new(parts: &'b [&'a [u8]]) -> Self { - Cursor { parts, part: 0, pos: 0 } + let after = parts.iter().skip(1).map(|p| p.len()).sum::(); + Cursor { parts, part: 0, pos: 0, after } } /// Skips to the next part when the current one is used up. Returns whether a new part began. fn settle(&mut self) -> bool { let mut moved = false; while self.parts.get(self.part).is_some_and(|p| self.pos >= p.len()) && self.part + 1 < self.parts.len() { + self.after = self.after.saturating_sub(self.parts[self.part + 1].len()); self.part += 1; self.pos = 0; moved = true; @@ -112,10 +117,10 @@ impl<'a, 'b> Cursor<'a, 'b> { Ok(()) } - /// Bytes left in the record (all parts). + /// Bytes left in the record (all parts), constant-time in the part count. pub fn remaining(&self) -> usize { let here = self.parts.get(self.part).map_or(0, |p| p.len().saturating_sub(self.pos)); - here + self.parts.iter().skip(self.part + 1).map(|p| p.len()).sum::() + here.saturating_add(self.after) } /// The rest of the record as one buffer. @@ -129,6 +134,7 @@ impl<'a, 'b> Cursor<'a, 'b> { } self.part = self.parts.len(); self.pos = 0; + self.after = 0; out } @@ -195,4 +201,23 @@ mod tests { let mut c = Cursor::new(&parts); assert!(c.chars(MAX_CHARS + 1, false).is_err()); } + + #[test] + fn remaining_counts_every_part_wherever_the_cursor_is() { + let parts: [&[u8]; 4] = [&[1, 2, 3], &[2, 3, 4, 5], &[3, 4, 5, 6, 7], &[4]]; + let mut c = Cursor::new(&parts); + assert_eq!(c.remaining(), 13); + for b in 1..=3u64 { + assert_eq!(u64::from(c.u8().unwrap()), b, "part 0 in order"); + } + assert_eq!(c.remaining(), 10, "after the first part is used up"); + for _ in 0..4 { + c.u8().unwrap(); + } + assert_eq!(c.remaining(), 6, "into the Continue parts"); + let mut c = Cursor::new(&parts); + assert_eq!(u64::from(c.u8().unwrap()), 1); + assert_eq!(c.rest().len(), 12); + assert_eq!(c.remaining(), 0); + } } diff --git a/crates/xlsx/src/xml.rs b/crates/xlsx/src/xml.rs index ec8cd9e75..e671d0adf 100644 --- a/crates/xlsx/src/xml.rs +++ b/crates/xlsx/src/xml.rs @@ -13,6 +13,12 @@ use crate::IoError; /// Maximum element nesting accepted in any part. pub const MAX_DEPTH: usize = 256; +/// Most elements kept in one part's parsed XML tree: an emptied `El` costs well over a hundred +/// bytes, so a highly-compressible part of endless empty elements could fit the package byte +/// limit and still take gigabytes of tree before any semantic check ran. Parts whose big runs +/// matter (worksheet rows, shared strings) are streamed instead and never hold this tree. +pub const MAX_XML_NODES: usize = 1_000_000; + /// An element: local name (namespace prefix dropped), attributes by local name, children and /// its direct text content. #[derive(Clone, Debug, Default, PartialEq)] @@ -223,13 +229,27 @@ pub fn parse_streaming(xml: &[u8], split: &[&str], cb: &mut dyn FnMut(El) -> Res let mut stack: Vec = Vec::new(); let mut root: Option = None; let mut buf = Vec::new(); + // Elements kept in the tree so far, so a pathological part that passes the byte limits can + // still be refused before the tree itself outgrows memory. + let mut kept = 0usize; // Attach a finished element to its parent (or emit it / make it the root). - fn finish(stack: &mut [El], root: &mut Option, el: El, split: &[&str], cb: &mut dyn FnMut(El) -> Result<(), IoError>) -> Result<(), IoError> { + fn finish( + stack: &mut [El], + root: &mut Option, + el: El, + split: &[&str], + cb: &mut dyn FnMut(El) -> Result<(), IoError>, + kept: &mut usize, + ) -> Result<(), IoError> { match stack.last_mut() { Some(parent) => { if split.contains(&el.name.as_str()) { cb(el)?; } else { + *kept = kept.saturating_add(1); + if *kept > MAX_XML_NODES { + return Err(IoError::Xml(format!("the part holds more than {MAX_XML_NODES} elements"))); + } parent.children.push(el); } } @@ -252,11 +272,11 @@ pub fn parse_streaming(xml: &[u8], split: &[&str], cb: &mut dyn FnMut(El) -> Res } Event::Empty(e) => { let el = start_el(&e); - finish(&mut stack, &mut root, el, split, cb)?; + finish(&mut stack, &mut root, el, split, cb, &mut kept)?; } Event::End(_) => { if let Some(el) = stack.pop() { - finish(&mut stack, &mut root, el, split, cb)?; + finish(&mut stack, &mut root, el, split, cb, &mut kept)?; } } Event::Text(t) => { @@ -295,7 +315,7 @@ pub fn parse_streaming(xml: &[u8], split: &[&str], cb: &mut dyn FnMut(El) -> Res } // Unclosed elements (truncated part): close them so we keep what we have. while let Some(el) = stack.pop() { - finish(&mut stack, &mut root, el, split, cb)?; + finish(&mut stack, &mut root, el, split, cb, &mut kept)?; } root.ok_or_else(|| IoError::Xml("no root element".into())) } @@ -475,6 +495,14 @@ mod tests { } } + #[test] + fn a_part_of_endless_empty_elements_is_refused() { + assert!(parse(b"").is_ok()); + let xml = format!("{}", "".repeat(MAX_XML_NODES + 1)); + let err = parse(xml.as_bytes()).unwrap_err(); + assert!(err.to_string().contains("elements"), "{err}"); + } + #[test] fn phonetic_data_validates_offsets() { let x = r#"課きくカクx"#;