From 35e98156035abb7c610729904ec39735b97e08c9 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 16:08:50 -0500 Subject: [PATCH] Security: one work budget across every call of a lifted function (#3197) * functions: one work budget across every call of a lifted function A function lifted over a computed array runs once per element, and only the six lookups counted their repeated work against LIFTED_SCAN_BUDGET. Criteria aggregates rescanned their ranges per criterion, CUMIPMT/CUMPRINC stepped through up to 1e7 periods per element, and array-producing functions such as MUNIT built a whole result per element before keeping its first cell. The budget now covers all of it: COUNTIF/SUMIF/AVERAGEIF(S)/MAXIFS/MINIFS charge their range cells per call, the period loops of every element are added up before any runs, and array results charge their cells as they are built. Past the budget the call is #NUM!, as lifted lookups already are. * address review: charge numeric-text periods of lifted CUMIPMT/CUMPRINC to the work budget * address review: charge only calls that reach the cumulative period loop * address review: charge the numeric text a lifted period call parses * address review: SUMIF/AVERAGEIF charge only the sum-range cells they read; weight cumulative period work --- crates/functions/src/financial.rs | 1 + crates/functions/src/financial/tvm.rs | 9 +- crates/functions/src/lift.rs | 221 +++++++++++++++++++++++--- 3 files changed, 204 insertions(+), 27 deletions(-) diff --git a/crates/functions/src/financial.rs b/crates/functions/src/financial.rs index b569c1617..75b6fbc78 100644 --- a/crates/functions/src/financial.rs +++ b/crates/functions/src/financial.rs @@ -13,6 +13,7 @@ mod day_count; mod depreciation; mod securities; mod tvm; +pub(crate) use tvm::cumulative_args; use day_count::{coupdaybs, coupdays, coupdaysnc, coupncd, coupnum, couppcd}; use depreciation::{amordegrc, amorlinc, db, ddb, sln, syd, vdb}; diff --git a/crates/functions/src/financial/tvm.rs b/crates/functions/src/financial/tvm.rs index 61710f0bd..872e7cbeb 100644 --- a/crates/functions/src/financial/tvm.rs +++ b/crates/functions/src/financial/tvm.rs @@ -116,7 +116,9 @@ pub(super) fn ppmt(a: &[Arg], _c: &mut dyn Ctx) -> R { num_val(pmt_calc(rate, np, pv, fv, typ) - ipmt_calc(rate, per, np, pv, fv, typ)) } -fn cumulative(a: &[Arg], principal: bool) -> R { +/// The validated (rate, periods, pv, start, end, type) of CUMIPMT/CUMPRINC: every check that +/// refuses a call before its period loop, shared with the lifted-call work preflight. +pub(crate) fn cumulative_args(a: &[Arg]) -> R<(f64, f64, f64, f64, f64, f64)> { let (rate, np, pv) = (num(a, 0)?, num(a, 1)?, num(a, 2)?); let (start, end, typ) = (num(a, 3)?.trunc(), num(a, 4)?.trunc(), num(a, 5)?); let np_i = np.trunc(); @@ -126,6 +128,11 @@ fn cumulative(a: &[Arg], principal: bool) -> R { if end - start > 1e7 || np_i > 1e15 { return Err(CellError::Num); } + Ok((rate, np_i, pv, start, end, typ)) +} + +fn cumulative(a: &[Arg], principal: bool) -> R { + let (rate, np_i, pv, start, end, typ) = cumulative_args(a)?; let p = pmt_calc(rate, np_i, pv, 0.0, typ); let mut sum = 0.0; let mut per = start; diff --git a/crates/functions/src/lift.rs b/crates/functions/src/lift.rs index d474bf12d..81d9e353e 100644 --- a/crates/functions/src/lift.rs +++ b/crates/functions/src/lift.rs @@ -63,26 +63,86 @@ const SCANS: &[&str] = &["VLOOKUP", "HLOOKUP", "LOOKUP", "MATCH", "XLOOKUP", "XM /// `XMATCH(SEQUENCE(1048576), -SEQUENCE(1048576))` runs for hours and hangs opening the file. /// Past the budget the call is `#NUM!`, as MMULT is past 2e9 multiply-adds. 5e8 keeps the worst /// case to a few seconds and still admits 5,000 needles over a 100,000-row table. +/// +/// The same budget covers all the work of a lifted call added up across its elements: criteria +/// aggregates rescanning their ranges, period loops, and array results built per element. const LIFTED_SCAN_BUDGET: usize = 500_000_000; -/// The length of the longest vector a lifted call of `spec` searches per needle: 0 unless `spec` -/// is one of the [`SCANS`] lookups and a non-lifted argument is an array. +/// Criteria aggregates: every call tests each cell of its ranges against its criteria, so a call +/// lifted over `n` criteria does `n` x (range cells) work, like the [`SCANS`] lookups. +const CRITERIA: &[&str] = &["COUNTIF", "COUNTIFS", "SUMIF", "SUMIFS", "AVERAGEIF", "AVERAGEIFS", "MAXIFS", "MINIFS"]; + +/// Cumulative TVM functions that step through every period from `start_period` (argument 3) to +/// `end_period` (argument 4) on each call. +const PERIOD_LOOPS: &[&str] = &["CUMIPMT", "CUMPRINC"]; + +/// Budget units one period of a [`PERIOD_LOOPS`] call costs: each period computes a payment and a +/// balance with powers, many times a scanned cell. +const PERIOD_COST: usize = 16; + +/// The work every lifted call of `spec` repeats over its non-lifted arguments: the longest vector +/// a [`SCANS`] lookup searches, or every cell a [`CRITERIA`] aggregate tests. 0 for other +/// functions. fn scan_len(spec: &FnSpec, args: &[Arg]) -> usize { - if !SCANS.contains(&spec.name) { + let scans = SCANS.contains(&spec.name); + if !scans && !CRITERIA.contains(&spec.name) { return 0; } - args.iter() - .enumerate() - .filter(|(i, _)| !spec.is_scalar_arg(*i)) - .filter_map(|(_, a)| match &a.value { - Value::Array(arr) => Some(arr.rows.max(arr.cols)), - _ => None, - }) - .max() - .unwrap_or(0) + // SUMIF and AVERAGEIF read the optional third range only at the coordinates of the first. + let first = args.first().and_then(|a| if let Value::Array(arr) = &a.value { Some(arr.data.len()) } else { None }); + let lens = args.iter().enumerate().filter(|(i, _)| !spec.is_scalar_arg(*i)).filter_map(|(i, a)| match &a.value { + Value::Array(arr) if scans => Some(arr.rows.max(arr.cols)), + Value::Array(arr) if i == 2 && matches!(spec.name, "SUMIF" | "AVERAGEIF") => Some(arr.data.len().min(first.unwrap_or(usize::MAX))), + Value::Array(arr) => Some(arr.data.len()), + _ => None, + }); + if scans { lens.max().unwrap_or(0) } else { lens.fold(0, usize::saturating_add) } +} + +/// The work of one lifted call that depends on its own (lifted) scalar arguments: the number of +/// periods a [`PERIOD_LOOPS`] function steps through. 0 for other functions. +fn call_cost(spec: &FnSpec, cell_args: &[Arg]) -> usize { + if !PERIOD_LOOPS.contains(&spec.name) { + return 0; + } + // Coercing numeric text reads all of it, even in a call the function then refuses, so each + // character is charged as well: of the arguments it coerces in order, up to the first that fails. + let mut parse = 0usize; + for (i, a) in cell_args.iter().enumerate().take(6) { + if let Value::Text(t) = &a.value { + parse = parse.saturating_add(t.len()); + } + if crate::util::num(cell_args, i).is_err() { + break; + } + } + // The function's own validation and coercion (text like "10000000" counts), so a call it + // refuses before its loop costs nothing and no loop goes uncharged. + let periods = crate::financial::cumulative_args(cell_args).map_or(0, |(.., start, end, _)| (end - start + 1.0) as usize); + parse.saturating_add(periods.saturating_mul(PERIOD_COST)) } pub(crate) fn call_lifted(spec: &FnSpec, args: &[Arg], ctx: &mut dyn Ctx) -> Value { + call_lifted_within(spec, args, ctx, LIFTED_SCAN_BUDGET) +} + +/// Sets the lifted arguments of `cell_args` to their elements at (`r`, `c`). +fn element_args(spec: &FnSpec, args: &[Arg], r: usize, c: usize, cell_args: &mut [Arg]) { + for (i, a) in args.iter().enumerate() { + if spec.is_scalar_arg(i) + && let Value::Array(arr) = &a.value + && let Some(slot) = cell_args.get_mut(i) + { + let v = arr.get_broadcast(r, c); + slot.value = if let Value::Array(inner) = &v { inner.data.first().cloned().unwrap_or(Value::Empty) } else { v }; + // A blank element of a written array argument is given, as a scalar blank is. + slot.blank_value = !slot.from_ref && matches!(slot.value, Value::Empty); + } + } +} + +/// [`call_lifted`] with every call's work added up against `budget` (see [`LIFTED_SCAN_BUDGET`]). +fn call_lifted_within(spec: &FnSpec, args: &[Arg], ctx: &mut dyn Ctx, budget: usize) -> Value { let rejected = if spec.value_args.is_empty() { None } else { reject_multi_cell_refs(spec, args) }; let args = rejected.as_deref().unwrap_or(args); let mut rows = 0usize; @@ -101,27 +161,42 @@ pub(crate) fn call_lifted(spec: &FnSpec, args: &[Arg], ctx: &mut dyn Ctx) -> Val return (spec.imp)(args, ctx); } let needles = rows.saturating_mul(cols); - if needles > MAX_CELLS || needles.saturating_mul(scan_len(spec, args)) > LIFTED_SCAN_BUDGET { + // Every call is charged its repeated scan (at least 1), its own period loop, and the cells of + // an array result (built in full, though only its first cell is kept), so lifting a scalar + // argument can't multiply what one call may do into hours of work: + // `COUNTIF(big_range, SEQUENCE(1e4))`, `CUMIPMT(0.1, 1e7, 1, 1, SEQUENCE(1e4,,1e7,0), 0)`, + // `MUNIT(SEQUENCE(1e4,,3000,0))`. + let per_call = scan_len(spec, args).max(1); + let mut cell_args: Vec = args.to_vec(); + let mut spent = needles.saturating_mul(per_call); + if needles > MAX_CELLS || spent > budget { return Value::Error(CellError::Num); } - let mut cell_args: Vec = args.to_vec(); + // Period loops are known before any call runs, so a call over budget does none of them. + if PERIOD_LOOPS.contains(&spec.name) { + for r in 0..rows { + for c in 0..cols { + element_args(spec, args, r, c, &mut cell_args); + spent = spent.saturating_add(call_cost(spec, &cell_args)); + if spent > budget { + return Value::Error(CellError::Num); + } + } + } + } let mut data = Vec::with_capacity(rows * cols); for r in 0..rows { for c in 0..cols { - for (i, a) in args.iter().enumerate() { - if spec.is_scalar_arg(i) - && let Value::Array(arr) = &a.value - && let Some(slot) = cell_args.get_mut(i) - { - let v = arr.get_broadcast(r, c); - slot.value = if let Value::Array(inner) = &v { inner.data.first().cloned().unwrap_or(Value::Empty) } else { v }; - // A blank element of a written array argument is given, as a scalar blank is. - slot.blank_value = !slot.from_ref && matches!(slot.value, Value::Empty); - } - } + element_args(spec, args, r, c, &mut cell_args); let out = (spec.imp)(&cell_args, ctx); data.push(match out { - Value::Array(a) => a.data.first().cloned().unwrap_or(Value::Empty), + Value::Array(a) => { + spent = spent.saturating_add(a.data.len()); + if spent > budget { + return Value::Error(CellError::Num); + } + a.data.first().cloned().unwrap_or(Value::Empty) + } v => v, }); } @@ -206,6 +281,100 @@ mod tests { assert_eq!(rows_of(&out), vec![vec![nv(-1.0)]; 1000]); } + /// Security: lifting a criteria aggregate rescans its range for every criterion, so criteria + /// x range cells past the budget is `#NUM!` before any scan runs. + #[test] + fn lifted_criteria_aggregates_stop_past_their_work_budget() { + use gridcraft_core::Array; + let tall = |n: usize| av(Array::filled(n, 1, nv(-1.0)).into()); + is_err(ev("COUNTIF", vec![tall(1_000_000), tall(501)]), CellError::Num); + is_err(ev("SUMIF", vec![tall(1_000_000), tall(501)]), CellError::Num); + is_err(ev("COUNTIFS", vec![tall(1_000_000), tall(501)]), CellError::Num); + // Two ranges of 250,000 cells each count together. + is_err(ev("SUMIFS", vec![tall(250_000), tall(250_000), tall(1001)]), CellError::Num); + // SUMIF reads its sum range only where the criteria range has cells: a one-cell range over a + // million-cell sum range is one cell of work per criterion. + let one = av(Array::filled(1, 1, nv(-1.0)).into()); + assert_eq!(rows_of(&ev("SUMIF", vec![one, tall(501), tall(1_000_000)])).len(), 501); + let counts = ev("COUNTIF", vec![av(col(&[1.0, 2.0, 2.0])), av(col(&[2.0, 1.0, 5.0]))]); + assert_eq!(rows_of(&counts), vec![vec![nv(2.0)], vec![nv(1.0)], vec![nv(0.0)]]); + for name in super::CRITERIA.iter().chain(super::PERIOD_LOOPS) { + assert!(crate::lookup(name).is_some(), "{name} isn't registered"); + } + } + + /// Security: CUMIPMT and CUMPRINC loop over their periods once per lifted element; the + /// periods of all elements are added up first, so an over-budget call runs none of them. + #[test] + fn lifted_period_loops_stop_past_their_work_budget() { + use gridcraft_core::Array; + let ends = |len: usize, end: f64| av(Array::filled(len, 1, nv(end)).into()); + let args = |len: usize, end: f64| vec![n(0.01), n(1e7), n(1000.0), n(1.0), ends(len, end), n(0.0)]; + // 51 elements of 1e7 periods each is past 5e8. + is_err(ev("CUMIPMT", args(51, 1e7)), CellError::Num); + is_err(ev("CUMPRINC", args(51, 1e7)), CellError::Num); + // A period costs more than a scanned cell, so fewer elements are enough. + is_err(ev("CUMIPMT", args(49, 1e7)), CellError::Num); + // Numeric text is coerced by the function, so its periods are charged the same. + let text_ends = av(Array::filled(51, 1, Value::text("10000000")).into()); + is_err(ev("CUMIPMT", vec![n(0.01), n(1e7), n(1000.0), n(1.0), text_ends, n(0.0)]), CellError::Num); + let out = ev("CUMIPMT", args(2, 12.0)); + assert_eq!(rows_of(&out).len(), 2); + assert!(matches!(rows_of(&out).first().and_then(|r| r.first()), Some(Value::Number(x)) if *x < 0.0)); + } + + /// Security: numeric text is parsed once per lifted element even when the call is refused; the + /// characters read are charged, so a long text repeated down a column can't cost gigabytes of parsing. + #[test] + fn lifted_period_loops_charge_the_text_they_parse() { + use gridcraft_core::Array; + // 20,000 elements x 32,000 characters is past 5e8 though no call reaches its loop (rate is negative). + let long = Value::text(format!("{}5", "0".repeat(32_000))); + let ends = av(Array::filled(20_000, 1, long).into()); + is_err(ev("CUMIPMT", vec![n(-0.01), n(10.0), n(1000.0), n(1.0), ends, n(0.0)]), CellError::Num); + // A short text period costs next to nothing. + let ends = av(Array::filled(3, 1, Value::text("12")).into()); + assert_eq!(rows_of(&ev("CUMIPMT", vec![n(0.01), n(20.0), n(1000.0), n(1.0), ends, n(0.0)])).len(), 3); + } + + /// Text after the first argument that fails to coerce is never read, so it is not charged: the + /// elements keep their own errors instead of one budget `#NUM!`. + #[test] + fn lifted_period_loops_charge_only_the_text_they_reach() { + use gridcraft_core::Array; + let rates = av(Array::filled(20_000, 1, Value::text("x")).into()); + let long = Value::text(format!("{}5", "0".repeat(32_000))); + let out = ev("CUMIPMT", vec![rates, n(10.0), n(1000.0), n(1.0), av(long), n(0.0)]); + assert!(rows_of(&out).iter().all(|r| r.first() == Some(&Value::Error(CellError::Value)))); + } + + /// A call the function refuses before its loop (here: end period past the period count) is + /// not charged, so rejected elements can't push a valid one over the budget. + #[test] + fn lifted_period_loops_do_not_charge_rejected_calls() { + use gridcraft_core::Array; + let mut ends = vec![nv(1e7 + 1.0); 60]; + ends.push(nv(12.0)); + let ends = av(Array::new(ends.len(), 1, ends).unwrap_or_else(|| panic!("shape")).into()); + let out = ev("CUMIPMT", vec![n(0.01), n(1e7), n(1000.0), n(1.0), ends, n(0.0)]); + let rows = rows_of(&out); + assert_eq!(rows.len(), 61); + assert!(matches!(rows.last().and_then(|r| r.first()), Some(Value::Number(x)) if *x < 0.0)); + } + + /// Security: an array-producing function lifted over a computed array builds each element's + /// whole result before keeping one cell; their cells count against the budget. + #[test] + fn lifted_array_results_count_against_the_budget() { + use gridcraft_core::Array; + let spec = crate::lookup("MUNIT").unwrap_or_else(|| panic!("MUNIT isn't registered")); + let dims = |n: usize| [av(Array::filled(n, 1, nv(10.0)).into())]; + // Ten 10x10 identity matrices are 1,000 cells. + let out = super::call_lifted_within(spec, &dims(10), &mut TestCtx::default(), 1_010); + assert_eq!(rows_of(&out), vec![vec![nv(1.0)]; 10]); + is_err(super::call_lifted_within(spec, &dims(11), &mut TestCtx::default(), 1_010), CellError::Num); + } + #[test] fn value_params_reject_multi_cell_references() { let s = FnSpec { value_args: &[true, false], ..spec() };