From e177bf1c44581da8bf29ebf7e261b8a988449c05 Mon Sep 17 00:00:00 2001 From: Tyrel Haveman <1434399+tyrel@users.noreply.github.com> Date: Wed, 21 Oct 2026 23:41:28 -0700 Subject: [PATCH 1/2] calc: hardening: overlong digits, NaN indexes, REGEXREPLACE output (#2163, #2162) - Text coercion: digits too many for a finite number are not a number, and a fraction must come out finite, so "1 <400 digits>/<400 digits>" (inf/inf) no longer coerces to NaN (test overlong_digits_stay_text). - VLOOKUP/HLOOKUP and the D-functions' field index refuse NaN, which passed their "< 1" checks and became index 0, then underflowed (test nan_index_is_a_value_error; hostile_inputs_never_panic now also passes NaN and infinity to every function). - REGEXREPLACE replaces match by match and gives up once the result can only be too long, instead of building every replacement first: the 32,767 x 32,767 worst case went from 1.05 GB peak to 7 MB (test regexreplace_bounds_its_result). #2165 (MAP's size check) is already fixed on main by the broadcast cap (836b2698, #2182), whose test covers MAP. Also adds my contributors/people.toml entry. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019Nnsfu3QGqjD1aFZbBgUWP --- ROADMAP.md | 4 +++- contributors/people.toml | 3 +++ crates/core/src/parse.rs | 19 +++++++++++++++++-- crates/functions/src/database.rs | 5 +++-- crates/functions/src/lib.rs | 31 +++++++++++++++++++++++++++++++ crates/functions/src/lookup.rs | 5 +++-- crates/functions/src/text.rs | 16 +++++++++++++++- 7 files changed, 75 insertions(+), 8 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 2e04e9123..1faf680ca 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -2,7 +2,7 @@ **Stage: alpha** · next: beta, ~25 points of "ready for real work" and ~160–240 h away -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-21 · **Change:** minor (Page Setup paper sizes: JIS B4/B5/B6, the Japanese postcards and envelopes, and A6; long flat operator chains up to Excel's formula limit) · **Target:** Microsoft Excel (Microsoft 365) +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-22 · **Change:** trivial (progress log: formula hardening, #2163/#2162; previously minor: Page Setup paper sizes: JIS B4/B5/B6, the Japanese postcards and envelopes, and A6; long flat operator chains up to Excel's formula limit) · **Target:** Microsoft Excel (Microsoft 365) GridCraft aims at full Microsoft Excel parity, and at being better in places: faster, open (XLSX is the native format), fully scriptable by agents (MCP, control channel, CLI), and on the web. @@ -135,6 +135,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | What landed | |---|---| +| 2026-10-22 | Formula hardening: digits too many for a finite number are text, so `"1 <400 digits>/<400 digits>"` no longer coerces to NaN; a NaN column index in VLOOKUP, HLOOKUP and the D-functions is `#VALUE!` instead of an index underflow; REGEXREPLACE stops building a result as soon as it can only be too long (a worst case went from 1 GB to 7 MB) (#2163, #2162) | | 2026-10-21 | Page Setup paper sizes match Japanese Excel: `B4`/`B5`/`B6` are the JIS sizes (ISO B4/B5/B6 stay reachable by name), so a Japanese B5 worksheet prints at 182×257 mm; Japanese Postcard (100×148 mm), Double Postcard (148×200 mm), Envelope #3/#4 (120×235/90×205 mm), Kaku #2/#3 (240×332/216×277 mm) and A6 are added, both sizes reach the PDF page (`file.exportPdf`/`file.print` MediaBox) and the size survives an XLSX round-trip; the Page Layout Size menu lists them. `B4`/`B5` used to be the ISO sizes, which is changed here | | 2026-10-20 | Japanese interface catalog covers every key the Brazilian Portuguese catalog translates: the 398-label shared core plus 545 dialog, message, Backstage and menu strings (943 total, ~43% of ~2,200). A test parses `pt.rs`'s source, so the two catalogs stay in step | | 2026-10-20 | Interface translated into Dutch (518 labels: ribbon, context menus, File view, dialog titles, Options); `nl`, `nl-NL`, `nl-BE` locale tags resolve to Dutch | @@ -220,6 +221,7 @@ Ranked; estimates in Opus 5.5 agent-hours. Detail and the full beta-gate list in | Date | Change | Summary | |---|---|---| +| 2026-10-22 | trivial | Progress log: formula hardening (#2163, #2162) | | 2026-10-21 | minor | Page Setup paper sizes: JIS B4/B5/B6 (ISO by name), Japanese postcards and envelopes, A6; PDF page size follows, XLSX round-trips | | 2026-10-20 | minor | Dutch (nl) listed under other shipped languages; progress log row | | 2026-10-19 | minor | Progress log: long flat operator chains (#916) | diff --git a/contributors/people.toml b/contributors/people.toml index 62635c8ae..88e496fc7 100644 --- a/contributors/people.toml +++ b/contributors/people.toml @@ -93,3 +93,6 @@ sync_github_name = false [people.bilalmughal] sync_github_name = true + +[people.tyrel] +real_name = "Tyrel Haveman" diff --git a/crates/core/src/parse.rs b/crates/core/src/parse.rs index b58586aef..3f2b97a8a 100644 --- a/crates/core/src/parse.rs +++ b/crates/core/src/parse.rs @@ -167,7 +167,8 @@ fn parse_plain(t: &str) -> Option<(f64, Option<&'static str>)> { } /// A plain ASCII number: digits, with an optional `.digits` fraction when `frac` is set. Rejects -/// `nan`, `inf`, signs and exponents that `f64::from_str` would accept. +/// `nan`, `inf`, signs and exponents that `f64::from_str` would accept, and digits too many for a +/// finite number (#2163). fn plain_number(s: &str, frac: bool) -> Option { let (int, dec) = match s.split_once('.') { Some((i, d)) if frac => (i, Some(d)), @@ -177,7 +178,7 @@ fn plain_number(s: &str, frac: bool) -> Option { if !digits(int) || dec.is_some_and(|d| !digits(d)) { return None; } - s.parse().ok() + s.parse().ok().filter(|x: &f64| x.is_finite()) } /// `1/2` (only with a whole part, else it's a date), `1 1/2`, `0 3/4`. @@ -192,6 +193,9 @@ fn parse_fraction(t: &str) -> Option { return None; } let v = w + a / b; + if !v.is_finite() { + return None; + } Some(if neg { -v } else { v }) } @@ -366,6 +370,17 @@ mod tests { parse_input(s, DateSystem::D1900) } + /// #2163: digits too many for a finite number don't make one (∞/∞ was NaN). + #[test] + fn overlong_digits_stay_text() { + let big = "9".repeat(400); + for s in [format!("1 {big}/{big}"), format!("1 {big}/2"), format!("1 1/{big}0"), format!("{big}:00"), format!("1:00:{big}.5")] { + assert_eq!(p(&s).value, Value::text(s.as_str()), "{s}"); + assert_eq!(parse_number_text(&s), None, "{s}"); + } + assert_eq!(parse_number_text("1 1/2"), Some(1.5)); + } + #[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/functions/src/database.rs b/crates/functions/src/database.rs index 67132e98f..455b8a1d6 100644 --- a/crates/functions/src/database.rs +++ b/crates/functions/src/database.rs @@ -35,10 +35,11 @@ fn field_index(db: &Array, a: &[Arg]) -> R> { match v { Value::Number(n) => { let i = n.trunc(); - if i < 1.0 || i > db.cols as f64 { + // NaN too: it would pass both comparisons and become index 0 (#2163). + if i.is_nan() || i < 1.0 || i > db.cols as f64 { return Err(CellError::Value); } - Ok(Some(i as usize - 1)) + Ok(Some((i as usize).saturating_sub(1))) } Value::Text(t) => find_column(db, &t).map(Some).ok_or(CellError::Value), Value::Bool(_) => Err(CellError::Value), diff --git a/crates/functions/src/lib.rs b/crates/functions/src/lib.rs index d7ccc8726..f870f3fe2 100644 --- a/crates/functions/src/lib.rs +++ b/crates/functions/src/lib.rs @@ -264,6 +264,34 @@ mod tests { } } + /// #2163: a NaN column index is refused (it used to become 0 and underflow to `usize::MAX`). + #[test] + fn nan_index_is_a_value_error() { + use crate::util::testutil::*; + use gridcraft_core::CellError; + let table = rf(arr(vec![vec![tv("Field"), tv("Other")], vec![nv(1.0), nv(2.0)]])); + let nan = || av(Value::Number(f64::NAN)); + assert_eq!(ev("VLOOKUP", vec![n(1.0), table.clone(), nan()]), Value::Error(CellError::Value)); + assert_eq!(ev("HLOOKUP", vec![t("Field"), table.clone(), nan()]), Value::Error(CellError::Value)); + assert_eq!(ev("DSUM", vec![table.clone(), nan(), table.clone()]), Value::Error(CellError::Value)); + assert_eq!(ev("VLOOKUP", vec![n(1.0), table, n(2.0)]), Value::Number(2.0)); + } + + /// #2162: REGEXREPLACE gives up on a too-long result while building it, and otherwise replaces + /// as before (every match, `$n` groups). + #[test] + fn regexreplace_bounds_its_result() { + use crate::util::testutil::*; + use gridcraft_core::CellError; + let many = "a".repeat(32_767); + assert_eq!(ev("REGEXREPLACE", vec![t(&many), t("."), t(&"b".repeat(32_767))]), Value::Error(CellError::Value)); + assert_eq!(ev("REGEXREPLACE", vec![t(&many), t("a"), t("b")]), Value::text("b".repeat(32_767))); + assert_eq!(ev("REGEXREPLACE", vec![t("a1b22c"), t("[0-9]+"), t("#")]), Value::text("a#b#c")); + assert_eq!(ev("REGEXREPLACE", vec![t("2026-10-22"), t("(\\d+)-(\\d+)-(\\d+)"), t("$3/$2/$1")]), Value::text("22/10/2026")); + assert_eq!(ev("REGEXREPLACE", vec![t("abc"), t("x*"), t("-")]), Value::text("-a-b-c-")); + assert_eq!(ev("REGEXREPLACE", vec![t("a1b2"), t("[0-9]"), t("#"), n(2.0)]), Value::text("a1b#")); + } + #[test] fn hostile_inputs_never_panic() { use crate::util::testutil::*; @@ -279,6 +307,9 @@ mod tests { n(-1e308), n(1e10), n(2.5), + // Numbers no formula should make, but code can (#2163). + av(Value::Number(f64::NAN)), + av(Value::Number(f64::INFINITY)), t(""), t("abc"), t("*?~"), diff --git a/crates/functions/src/lookup.rs b/crates/functions/src/lookup.rs index f9c0b6f44..acb23f2ea 100644 --- a/crates/functions/src/lookup.rs +++ b/crates/functions/src/lookup.rs @@ -134,10 +134,11 @@ fn vh_lookup(args: &[Arg], vertical: bool) -> R { return Err(*e); } let idx = num(args, 2)?; - if idx < 1.0 { + // NaN too: it would pass `idx < 1.0` and become index 0 (#2163). + if idx.is_nan() || idx < 1.0 { return Err(CellError::Value); } - let idx = idx.trunc() as usize - 1; + let idx = (idx.trunc() as usize).saturating_sub(1); let approx = opt_bool(args, 3, true)?; let (keys, len) = if vertical { (column(&table, 0), table.cols) } else { (row(&table, 0), table.rows) }; if idx >= len { diff --git a/crates/functions/src/text.rs b/crates/functions/src/text.rs index 71a6ccb4b..096e32990 100644 --- a/crates/functions/src/text.rs +++ b/crates/functions/src/text.rs @@ -769,7 +769,21 @@ fn regexreplace(a: &[Arg], _c: &mut dyn Ctx) -> R { let occ = opt_int(a, 3, 0)?; let re = build_regex(&text(a, 1)?, case_arg(a, 4)?)?; if occ == 0 { - return check_len(re.replace_all(&s, rep.as_str()).into_owned()); + // Match by match, giving up once the text can only be too long (a character is at most + // four bytes) rather than building every replacement first (#2162). + let mut out = String::new(); + let mut last = 0; + for cap in re.captures_iter(&s) { + let Some(m) = cap.get(0) else { continue }; + out.push_str(s.get(last..m.start()).unwrap_or_default()); + cap.expand(&rep, &mut out); + last = m.end(); + if out.len() > MAX_TEXT * 4 { + return Err(CellError::Value); + } + } + out.push_str(s.get(last..).unwrap_or_default()); + return check_len(out); } let caps: Vec = re.captures_iter(&s).collect(); let k = occ.unsigned_abs() as usize; From 742a6e8535dde599073ddffb9e55a3f2b58cd83e Mon Sep 17 00:00:00 2001 From: Tyrel Haveman <1434399+tyrel@users.noreply.github.com> Date: Wed, 21 Oct 2026 23:45:50 -0700 Subject: [PATCH 2/2] functions: REGEXREPLACE bounds each replacement as it expands it (#2162) From the local CodeRabbit review: the result was bounded per match, but one match's expansion was built whole first, so a replacement repeating $0 10,000 times over a 32,767-character match allocated about 330 MB before the check. The replacement is now split into pieces as regex's Captures::expand reads it (template_expands_like_regex checks they agree) and appended piece by piece within the bound: 327 MB peak became 7 MB (regexreplace_bounds_its_result). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019Nnsfu3QGqjD1aFZbBgUWP --- crates/functions/src/lib.rs | 3 ++ crates/functions/src/text.rs | 100 ++++++++++++++++++++++++++++++----- 2 files changed, 91 insertions(+), 12 deletions(-) diff --git a/crates/functions/src/lib.rs b/crates/functions/src/lib.rs index f870f3fe2..e4f7a1f24 100644 --- a/crates/functions/src/lib.rs +++ b/crates/functions/src/lib.rs @@ -290,6 +290,9 @@ mod tests { assert_eq!(ev("REGEXREPLACE", vec![t("2026-10-22"), t("(\\d+)-(\\d+)-(\\d+)"), t("$3/$2/$1")]), Value::text("22/10/2026")); assert_eq!(ev("REGEXREPLACE", vec![t("abc"), t("x*"), t("-")]), Value::text("-a-b-c-")); assert_eq!(ev("REGEXREPLACE", vec![t("a1b2"), t("[0-9]"), t("#"), n(2.0)]), Value::text("a1b#")); + // One match whose replacement repeats it 10,000 times: refused before it is built. + assert_eq!(ev("REGEXREPLACE", vec![t(&many), t("^.*$"), t(&"$0".repeat(10_000))]), Value::Error(CellError::Value)); + assert_eq!(ev("REGEXREPLACE", vec![t(&many), t("^.*$"), t(&"$0".repeat(10_000)), n(1.0)]), Value::Error(CellError::Value)); } #[test] diff --git a/crates/functions/src/text.rs b/crates/functions/src/text.rs index 096e32990..95eb6784d 100644 --- a/crates/functions/src/text.rs +++ b/crates/functions/src/text.rs @@ -763,24 +763,87 @@ fn convert_replacement(r: &str) -> String { out } +/// A piece of a replacement: literal text or a capture group (by number or name). +enum Piece { + Text(String), + Index(usize), + Name(String), +} + +/// A replacement split into pieces as `regex::Captures::expand` reads it: `$$` is `$`, `$name` takes +/// the longest run of `[_0-9A-Za-z]`, `${name}` ends at `}` (`${}` is empty), a name of digits is a +/// group number, and a `$` that starts no reference is itself. +fn template(rep: &str) -> Vec { + let mut out = Vec::new(); + let mut lit = String::new(); + let mut rest = rep; + while let Some(i) = rest.find('$') { + lit.push_str(&rest[..i]); + rest = &rest[i + 1..]; + if let Some(r) = rest.strip_prefix('$') { + lit.push('$'); + rest = r; + continue; + } + // A braced name is a reference even when empty (it expands to nothing). + let braced = rest.strip_prefix('{').and_then(|b| b.find('}').map(|j| (&b[..j], &b[j + 1..]))); + let (name, after) = braced.unwrap_or_else(|| { + let j = rest.find(|c: char| !(c == '_' || c.is_ascii_alphanumeric())).unwrap_or(rest.len()); + (&rest[..j], &rest[j..]) + }); + if braced.is_none() && name.is_empty() { + lit.push('$'); + continue; + } + if !lit.is_empty() { + out.push(Piece::Text(std::mem::take(&mut lit))); + } + out.push(match name.parse::() { + Ok(n) => Piece::Index(n), + Err(_) => Piece::Name(name.to_string()), + }); + rest = after; + } + lit.push_str(rest); + if !lit.is_empty() { + out.push(Piece::Text(lit)); + } + out +} + +/// Append a match's replacement to `out`, piece by piece; `None` once `out` would pass `limit` +/// bytes, before anything that long is built (#2162). +fn expand_capped(cap: ®ex::Captures, pieces: &[Piece], out: &mut String, limit: usize) -> Option<()> { + for p in pieces { + let s = match p { + Piece::Text(t) => t.as_str(), + Piece::Index(i) => cap.get(*i).map_or("", |m| m.as_str()), + Piece::Name(n) => cap.name(n).map_or("", |m| m.as_str()), + }; + if out.len().saturating_add(s.len()) > limit { + return None; + } + out.push_str(s); + } + Some(()) +} + fn regexreplace(a: &[Arg], _c: &mut dyn Ctx) -> R { let s = text(a, 0)?; - let rep = convert_replacement(&text(a, 2)?); + let rep = template(&convert_replacement(&text(a, 2)?)); let occ = opt_int(a, 3, 0)?; let re = build_regex(&text(a, 1)?, case_arg(a, 4)?)?; + // Built match by match within a bound past which the text can only be too long (a character + // is at most four bytes), instead of building every replacement first (#2162). + let limit = MAX_TEXT * 4; + let mut out = String::new(); if occ == 0 { - // Match by match, giving up once the text can only be too long (a character is at most - // four bytes) rather than building every replacement first (#2162). - let mut out = String::new(); let mut last = 0; for cap in re.captures_iter(&s) { let Some(m) = cap.get(0) else { continue }; out.push_str(s.get(last..m.start()).unwrap_or_default()); - cap.expand(&rep, &mut out); + expand_capped(&cap, &rep, &mut out, limit).ok_or(CellError::Value)?; last = m.end(); - if out.len() > MAX_TEXT * 4 { - return Err(CellError::Value); - } } out.push_str(s.get(last..).unwrap_or_default()); return check_len(out); @@ -790,10 +853,9 @@ fn regexreplace(a: &[Arg], _c: &mut dyn Ctx) -> R { let idx = if occ > 0 { k.checked_sub(1) } else { caps.len().checked_sub(k) }; let Some(cap) = idx.and_then(|i| caps.get(i)) else { return text_val(s) }; let Some(m) = cap.get(0) else { return text_val(s) }; - let mut out = String::new(); - out.push_str(&s[..m.start()]); - cap.expand(&rep, &mut out); - out.push_str(&s[m.end()..]); + out.push_str(s.get(..m.start()).unwrap_or_default()); + expand_capped(cap, &rep, &mut out, limit).ok_or(CellError::Value)?; + out.push_str(s.get(m.end()..).unwrap_or_default()); check_len(out) } @@ -971,6 +1033,20 @@ pub(crate) fn specs() -> Vec { mod tests { use gridcraft_core::{CellError, Value}; + /// The pieces expand exactly as `regex::Captures::expand` does. + #[test] + fn template_expands_like_regex() { + let re = regex::Regex::new(r"(?\d+)-(\d+)").unwrap(); + let cap = re.captures("x 2026-10 y").unwrap(); + for rep in ["$1", "${1}", "$1a", "${1}a", "$y", "${y}!", "$$", "$$1", "a$", "$}", "${", "${}", "${y", "$0|$2|$9", "$-1", "€$1€", ""] { + let mut want = String::new(); + cap.expand(rep, &mut want); + let mut got = String::new(); + assert!(super::expand_capped(&cap, &super::template(rep), &mut got, usize::MAX).is_some()); + assert_eq!(got, want, "{rep:?}"); + } + } + use crate::util::testutil::*; #[test]