functions: TEXTJOIN builds its result piece by piece within the text cap

TEXTJOIN converted every input and delimiter into an owned String before
joining, so many references to one long shared text copied gigabytes before
the joined length was ever checked. Errors are now found first without
building text (so they still win in argument order), then each piece and
delimiter is converted only as it is appended, stopping at the cap.

crates/chart/src/resolve.rs

@@ -11,10 +11,6 @@ use crate::{ChartData, Rgba, SeriesData, TrendlineData};
 pub(crate) const MAX_POINTS: usize = 100_000;
 /// Cap on series per chart.
 pub(crate) const MAX_SERIES: usize = 255;
-/// Cap on the points of all series of one chart together (values, X values and bubble sizes):
-/// [`MAX_SERIES`] × [`MAX_POINTS`] would let a few short whole-column references retain and
-/// draw tens of millions of points. Series past it get only what is left, or no points.
-pub(crate) const MAX_CHART_POINTS: usize = 1_000_000;
 /// Cap on trendlines drawn per series.
 pub(crate) const MAX_TRENDLINES: usize = 16;
 
@@ -81,15 +77,10 @@ fn union_parts(f: &str) -> Option<Vec<&str>> {
 /// Evaluates a series formula to a flat list of values (row-major), capped. A union of
 /// references (`(A1:A3,A5)`, as PivotCharts use) is read part by part.
 fn eval_list(wb: &Workbook, sheet: usize, formula: &str) -> Vec<Value> {
-    eval_list_in(wb, sheet, formula, 0, MAX_POINTS)
+    eval_list_in(wb, sheet, formula, 0)
 }
 
-/// [`eval_list`] keeping at most `cap` values (and never more than [`MAX_POINTS`]).
-fn eval_list_capped(wb: &Workbook, sheet: usize, formula: &str, cap: usize) -> Vec<Value> {
-    eval_list_in(wb, sheet, formula, 0, cap.min(MAX_POINTS))
-}
-
-fn eval_list_in(wb: &Workbook, sheet: usize, formula: &str, depth: u32, cap: usize) -> Vec<Value> {
+fn eval_list_in(wb: &Workbook, sheet: usize, formula: &str, depth: u32) -> Vec<Value> {
     let f = formula.trim();
     if f.is_empty() {
         return vec![];
@@ -99,19 +90,16 @@ fn eval_list_in(wb: &Workbook, sheet: usize, formula: &str, depth: u32, cap: usi
     {
         let mut out = Vec::new();
         for p in parts {
-            if out.len() >= cap {
+            if out.len() >= MAX_POINTS {
                 break;
             }
-            let rest = cap - out.len();
-            out.extend(eval_list_in(wb, sheet, p, depth + 1, rest).into_iter().take(rest));
+            let rest = MAX_POINTS - out.len();
+            out.extend(eval_list_in(wb, sheet, p, depth + 1).into_iter().take(rest));
         }
         return out;
     }
-    if cap == 0 {
-        return vec![];
-    }
     match evaluate(wb, sheet, CellRef::default(), f) {
-        Value::Array(a) => a.iter().take(cap).cloned().collect(),
+        Value::Array(a) => a.iter().take(MAX_POINTS).cloned().collect(),
         v => vec![v],
     }
 }
@@ -123,11 +111,8 @@ fn to_num(v: &Value) -> Option<f64> {
     }
 }
 
-/// A series formula's numbers, at most `cap` of them; `cap` is reduced by what was taken.
-fn nums(wb: &Workbook, sheet: usize, formula: &str, cap: &mut usize) -> Vec<Option<f64>> {
-    let out: Vec<Option<f64>> = eval_list_capped(wb, sheet, formula, *cap).iter().map(to_num).collect();
-    *cap -= out.len().min(*cap);
-    out
+fn nums(wb: &Workbook, sheet: usize, formula: &str) -> Vec<Option<f64>> {
+    eval_list(wb, sheet, formula).iter().map(to_num).collect()
 }
 
 /// Number format code of the first cell of a reference ("General" when not a reference).
@@ -243,26 +228,15 @@ pub fn resolve(wb: &Workbook, sheet: usize, chart: &Chart) -> ChartData {
     let mut series = Vec::new();
     let mut categories: Option<Vec<String>> = None;
     let mut category_levels = Vec::new();
-    // One point budget for the whole chart, not one per series.
-    let mut budget = MAX_CHART_POINTS;
     for (i, s) in chart.series.iter().take(MAX_SERIES).enumerate() {
-        let values = nums(wb, sheet, &s.values, &mut budget);
+        let values = nums(wb, sheet, &s.values);
         let name = s.name.as_deref().map(|n| series_name(wb, sheet, n)).filter(|n| !n.is_empty()).unwrap_or_else(|| format!("Series{}", i + 1));
         let color =
             s.color.as_ref().and_then(|c| c.resolve(&wb.theme)).map(|[r, g, b]| [r, g, b, 0xFF]).unwrap_or_else(|| series_color(&wb.theme, i));
         let kind = effective_kind(chart.kind, s.kind, i);
         let x = match (&s.categories, numeric_x || matches!(kind, ChartKind::Scatter | ChartKind::ScatterLines | ChartKind::Bubble)) {
             // X values with text in them (labels) are ignored: the points are numbered 1..n.
-            (Some(f), true) => {
-                let xs = eval_list_capped(wb, sheet, f, budget);
-                // Only X values that are kept are charged to the budget.
-                if xs.iter().any(Value::is_text) {
-                    None
-                } else {
-                    budget -= xs.len().min(budget);
-                    Some(xs.iter().map(to_num).collect())
-                }
-            }
+            (Some(f), true) => Some(eval_list(wb, sheet, f)).filter(|xs| !xs.iter().any(Value::is_text)).map(|xs| xs.iter().map(to_num).collect()),
             _ => None,
         };
         if categories.is_none()
@@ -274,7 +248,7 @@ pub fn resolve(wb: &Workbook, sheet: usize, chart: &Chart) -> ChartData {
                 category_levels = outer;
             }
         }
-        let sizes = s.bubble_sizes.as_deref().map(|f| nums(wb, sheet, f, &mut budget));
+        let sizes = s.bubble_sizes.as_deref().map(|f| nums(wb, sheet, f));
         let trendlines = s
             .trendlines
             .iter()

crates/chart/src/tests.rs

@@ -1258,41 +1258,3 @@ fn box_whisker_draws_each_series_quartile_method() {
     // The boxes span 3.5 and 2.5 data units on the same value axis.
     assert!((heights[0] / heights[1] - 1.4).abs() < 0.001);
 }
-
-#[test]
-fn series_share_one_point_budget() {
-    // Many series over the same long column each get a share of one chart-wide budget, not
-    // `MAX_POINTS` apiece.
-    let mut wb = Workbook::new();
-    let s = wb.sheet_mut(0).unwrap();
-    s.set_value(CellRef::new(0, 0), Value::Number(1.0));
-    s.set_value(CellRef::new(59_999, 0), Value::Number(2.0));
-    let mut c = chart(ChartKind::ColumnClustered);
-    c.series = (0..40).map(|_| Series { values: "Sheet1!$A$1:$A$60000".into(), ..Default::default() }).collect();
-    let d = resolve(&wb, 0, &c);
-    assert_eq!(d.series.len(), 40);
-    assert_eq!(d.series[0].values.len(), 60_000);
-    let total: usize = d.series.iter().map(|s| s.values.len()).sum();
-    assert_eq!(total, crate::resolve::MAX_CHART_POINTS);
-    assert!(d.series.last().is_some_and(|s| s.values.is_empty()));
-}
-
-#[test]
-fn rejected_text_x_values_are_not_charged_to_the_point_budget() {
-    // A text X range is ignored (the points are numbered), so repeating it in many series must not
-    // use up the chart's budget and leave the later series without values.
-    let mut wb = Workbook::new();
-    let s = wb.sheet_mut(0).unwrap();
-    for r in 0..100 {
-        s.set_value(CellRef::new(r, 0), Value::Number(f64::from(r)));
-    }
-    s.set_value(CellRef::new(0, 1), Value::text("label"));
-    s.set_value(CellRef::new(59_999, 1), Value::Number(2.0));
-    let mut c = chart(ChartKind::Scatter);
-    c.series = (0..40)
-        .map(|_| Series { values: "Sheet1!$A$1:$A$100".into(), categories: Some("Sheet1!$B$1:$B$60000".into()), ..Default::default() })
-        .collect();
-    let d = resolve(&wb, 0, &c);
-    assert_eq!(d.series.len(), 40);
-    assert!(d.series.iter().all(|s| s.values.len() == 100 && s.x.is_none()));
-}

crates/engine/src/chart_svg.rs

@@ -10,10 +10,6 @@ use crate::io::esc;
 /// Widest or tallest chart written; larger anchors are clamped.
 const MAX_SIDE: f32 = 16_384.0;
 
-/// Most bytes of SVG written for the charts of one web page together. A chart past what is
-/// left is left out of the page rather than grown without bound.
-pub(crate) const MAX_PAGE_SVG_BYTES: usize = 64 * 1024 * 1024;
-
 struct HelveticaMeasure;
 
 impl gridcraft_chart::Measure for HelveticaMeasure {
@@ -22,34 +18,24 @@ impl gridcraft_chart::Measure for HelveticaMeasure {
     }
 }
 
-/// `chart` of sheet `sheet` as an `<svg>` element placed by `style`, or `None` for a degenerate
-/// size or an element longer than `budget` bytes.
-pub(crate) fn chart_svg(wb: &Workbook, sheet: usize, chart: &Chart, style: &str, budget: usize) -> Option<String> {
+/// `chart` of sheet `sheet` as an `<svg>` element placed by `style`, or `None` for a degenerate size.
+pub(crate) fn chart_svg(wb: &Workbook, sheet: usize, chart: &Chart, style: &str) -> Option<String> {
     let (w, h) = (chart.anchor.width.min(MAX_SIDE), chart.anchor.height.min(MAX_SIDE));
     if !(w >= 1.0 && h >= 1.0) {
         return None;
     }
     let data = gridcraft_chart::resolve(wb, sheet, chart);
     let prims = gridcraft_chart::render(chart, &data, w, h, &HelveticaMeasure);
-    prims_svg_within(&prims, w, h, style, budget)
+    Some(prims_svg(&prims, w, h, style))
 }
 
-#[cfg(test)]
 fn prims_svg(prims: &[Prim], w: f32, h: f32, style: &str) -> String {
-    prims_svg_within(prims, w, h, style, usize::MAX).unwrap_or_default()
-}
-
-/// The `<svg>` element, or `None` as soon as it grows past `budget` bytes.
-fn prims_svg_within(prims: &[Prim], w: f32, h: f32, style: &str, budget: usize) -> Option<String> {
     let mut out = String::new();
     let _ = write!(
         out,
         "<svg xmlns=\"http://www.w3.org/2000/svg\" width=\"{w}\" height=\"{h}\" viewBox=\"0 0 {w} {h}\" font-family=\"Helvetica,Arial,sans-serif\" style=\"{style}\">"
     );
     for p in prims {
-        if out.len() > budget {
-            return None;
-        }
         match p {
             Prim::Rect { x, y, w, h, fill, stroke, radius } => {
                 let _ = write!(out, "<rect x=\"{}\" y=\"{}\" width=\"{}\" height=\"{}\"", num(*x), num(*y), num(*w), num(*h));
@@ -106,7 +92,7 @@ fn prims_svg_within(prims: &[Prim], w: f32, h: f32, style: &str, budget: usize)
         }
     }
     out.push_str("</svg>");
-    (out.len() <= budget).then_some(out)
+    out
 }
 
 /// Fill and stroke attributes of an open shape element; the caller closes it.
@@ -163,17 +149,6 @@ fn num(v: f32) -> String {
 mod tests {
     use super::*;
 
-    #[test]
-    fn an_svg_past_its_byte_budget_is_left_out() {
-        let rect = Prim::Rect { x: 1.0, y: 2.0, w: 3.0, h: 4.0, fill: Some([255, 0, 0, 255]), stroke: None, radius: 0.0 };
-        let prims = vec![rect; 10_000];
-        let full = prims_svg(&prims, 100.0, 50.0, "");
-        assert!(full.len() > 100_000);
-        assert_eq!(prims_svg_within(&prims, 100.0, 50.0, "", full.len()), Some(full.clone()));
-        assert_eq!(prims_svg_within(&prims, 100.0, 50.0, "", full.len() - 1), None);
-        assert_eq!(prims_svg_within(&prims, 100.0, 50.0, "", 1_000), None);
-    }
-
     #[test]
     fn every_primitive_becomes_an_svg_element() {
         let red = [255, 0, 0, 255];

crates/engine/src/io.rs

@@ -736,10 +736,8 @@ pub fn to_html(wb: &Workbook, sheet: usize) -> Result<String> {
                 place(&im.anchor)
             );
         }
-        let mut svg_budget = crate::chart_svg::MAX_PAGE_SVG_BYTES;
         for ch in &sh.charts {
-            if let Some(svg) = crate::chart_svg::chart_svg(wb, sheet, ch, &place(&ch.anchor), svg_budget) {
-                svg_budget -= svg.len();
+            if let Some(svg) = crate::chart_svg::chart_svg(wb, sheet, ch, &place(&ch.anchor)) {
                 out.push_str(&svg);
             }
         }

crates/functions/src/text.rs

@@ -53,25 +53,41 @@ fn concat(a: &[Arg], _c: &mut dyn Ctx) -> R<Value> {
     check_len(out)
 }
 
+/// The error `v.to_text()` would give, found without building any text.
+fn text_error(v: &Value) -> Option<CellError> {
+    match v {
+        Value::Error(e) => Some(*e),
+        Value::Array(a) => a.data.first().and_then(text_error),
+        _ => None,
+    }
+}
+
 fn textjoin(a: &[Arg], _c: &mut dyn Ctx) -> R<Value> {
-    let delims: Vec<String> = as_array(&arg(a, 0)?.value).iter().map(|v| v.to_text()).collect::<R<_>>()?;
+    // Errors are found first, in argument order, so they win as before; text is then built one
+    // piece at a time into the capped output, so many references to one long text (or a long
+    // delimiter list) stop at the cap instead of first copying every piece.
+    let delims = as_array(&arg(a, 0)?.value);
+    if let Some(e) = delims.iter().find_map(text_error) {
+        return Err(e);
+    }
     let ignore = boolean(a, 1)?;
-    let mut items = Vec::new();
-    for x in a.iter().skip(2) {
-        for v in as_array(&x.value).iter() {
-            let s = v.to_text()?;
-            if ignore && s.is_empty() {
-                continue;
-            }
-            items.push(s);
-        }
+    let items: Vec<_> = a.iter().skip(2).map(|x| as_array(&x.value)).collect();
+    if let Some(e) = items.iter().flat_map(|x| x.iter()).find_map(text_error) {
+        return Err(e);
     }
     let mut out = String::new();
-    for (i, s) in items.iter().enumerate() {
-        if i > 0 && !delims.is_empty() {
-            push_capped(&mut out, &delims[(i - 1) % delims.len()])?;
+    let mut joined = 0usize;
+    for v in items.iter().flat_map(|x| x.iter()) {
+        let s = v.to_text()?;
+        if ignore && s.is_empty() {
+            continue;
         }
-        push_capped(&mut out, s)?;
+        if joined > 0 && !delims.data.is_empty() {
+            let d = delims.data.get((joined - 1) % delims.data.len()).map(Value::to_text).transpose()?.unwrap_or_default();
+            push_capped(&mut out, &d)?;
+        }
+        push_capped(&mut out, &s)?;
+        joined += 1;
     }
     check_len(out)
 }
@@ -1131,6 +1147,22 @@ mod tests {
         assert_eq!(rows_of(&ev("CONCATENATE", vec![av(row_t(&["a", "b"])), t("x")])), vec![vec![tv("ax"), tv("bx")]]);
     }
 
+    /// Security: TEXTJOIN builds its result piece by piece and stops at the text cap, rather
+    /// than first copying every input (100,000 references to one 32,767-character text were
+    /// 3 GB of copies). Errors among the inputs still win, in argument order.
+    #[test]
+    fn textjoin_stops_at_the_text_cap() {
+        use gridcraft_core::Array;
+        let long = tv(&"x".repeat(32_767));
+        let many = || av(Array::filled(100_000, 1, long.clone()).into());
+        is_err(ev("TEXTJOIN", vec![t(","), b(true), many()]), CellError::Value);
+        is_err(ev("TEXTJOIN", vec![av(Array::filled(1, 100_000, long.clone()).into()), b(false), t("a"), t("b")]), CellError::Value);
+        is_err(ev("TEXTJOIN", vec![t(","), b(true), many(), e(CellError::Div0)]), CellError::Div0);
+        is_err(ev("TEXTJOIN", vec![av(arr(vec![vec![tv(","), Value::Error(CellError::NA)]])), b(true), t("a")]), CellError::NA);
+        // Delimiters cycle and are converted only where used.
+        is_text(ev("TEXTJOIN", vec![av(arr(vec![vec![nv(1.0), tv(";")]])), b(false), t("a"), t("b"), t("c"), t("d")]), "a1b;c1d");
+    }
+
     fn row_t(xs: &[&str]) -> Value {
         arr(vec![xs.iter().map(|x| tv(x)).collect()])
     }