check: charge relationship parts to the package's decompression budget

apps/gridcraft-cli/src/check/package.rs

@@ -6,6 +6,10 @@ const MAX_XML_PART: u64 = 64 * 1024 * 1024;
 /// Most zip entries looked at while validating a written package.
 const MAX_ENTRIES: usize = 200_000;
 
+/// Decompressed bytes that comparing one package's relationship kinds may read in total: the
+/// relationship parts themselves and the targets whose content is checked.
+const RELATIONSHIP_BUDGET: u64 = 512 * 1024 * 1024;
+
 /// Relationship kinds (lower case) whose absence after a save isn't a loss: the calculation chain is
 /// a cache Excel rebuilds, and GridCraft never writes it.
 pub(super) const REBUILT_KINDS: [&str; 1] = ["calcchain"];
@@ -117,6 +121,28 @@ fn read_zip_part(zip: &mut Zip<'_>, name: &str) -> Result<Vec<u8>, String> {
     Ok(buf)
 }
 
+enum PartError {
+    /// Reading the part would spend more than the remaining budget.
+    OverBudget,
+    Unreadable,
+}
+
+/// Reads a part like [`read_zip_part`], charging the decompressed bytes to `budget`.
+fn read_zip_part_within(zip: &mut Zip<'_>, name: &str, budget: &mut u64) -> Result<Vec<u8>, PartError> {
+    let f = zip.by_name(name).map_err(|_| PartError::Unreadable)?;
+    // One byte past what is left shows that the part doesn't fit.
+    let limit = MAX_XML_PART.min(budget.saturating_add(1));
+    let mut buf = Vec::new();
+    f.take(limit).read_to_end(&mut buf).map_err(|_| PartError::Unreadable)?;
+    let read = buf.len() as u64;
+    if read > *budget {
+        *budget = 0;
+        return Err(PartError::OverBudget);
+    }
+    *budget -= read;
+    Ok(buf)
+}
+
 /// Whether the producer flagged its cached results as stale with `<calcPr fullCalcOnLoad="1">`:
 /// libraries that don't calculate write it, and store 0 or nothing as every formula's value.
 pub(super) fn full_calc_on_load(bytes: &[u8]) -> bool {
@@ -232,19 +258,30 @@ pub fn validate_package(bytes: &[u8]) -> Result<Vec<String>, String> {
 /// nothing are left out, as a save may rightly drop them: a link to a place in the workbook
 /// (`#Sheet1!A1`, written as a cell's `location`) and a target part with nothing in it.
 pub(super) fn relationship_kinds(bytes: &[u8]) -> Result<BTreeMap<String, String>, String> {
+    relationship_kinds_within(bytes, RELATIONSHIP_BUDGET)
+}
+
+/// [`relationship_kinds`] reading at most `budget` decompressed bytes. `Err` once the relationship
+/// parts alone need more: their kinds can't be listed completely.
+fn relationship_kinds_within(bytes: &[u8], mut budget: u64) -> Result<BTreeMap<String, String>, String> {
     let mut zip = zip::ZipArchive::new(Cursor::new(bytes)).map_err(|e| format!("not a zip package: {e}"))?;
     let names = zip_names(&mut zip);
     let by_key: HashMap<String, &String> = names.iter().map(|n| (part_key(n), n)).collect();
     // Several relationships name the same part; each fresh check decompressed it again, so
     // repeated references to one highly compressible part multiplied the work. The verdict is
     // remembered per (part, named content) and the decompression the package can cause is
-    // budgeted as a whole (finding: repeated decompression).
+    // budgeted as a whole, the relationship parts included: many large, unreferenced rels parts
+    // would otherwise each spend a full per-part allowance.
     let mut verdicts: HashMap<(String, &'static str), bool> = HashMap::new();
-    let mut budget: u64 = 512 * 1024 * 1024;
     let mut kinds = BTreeMap::new();
     for n in names.iter().filter(|n| part_key(n).ends_with(".rels")) {
         let Some(src_dir) = rels_source_dir(&part_key(n)) else { continue };
-        let Ok(els) = read_zip_part(&mut zip, n).and_then(|xml| xml_elements(&xml)) else { continue };
+        let xml = match read_zip_part_within(&mut zip, n, &mut budget) {
+            Ok(xml) => xml,
+            Err(PartError::OverBudget) => return Err(format!("{n}: relationship check stopped at its decompression limit")),
+            Err(PartError::Unreadable) => continue,
+        };
+        let Ok(els) = xml_elements(&xml) else { continue };
         for (_, attrs) in els.iter().filter(|(el, _)| el == "Relationship") {
             let Some(kind) = attr(attrs, "Type").and_then(|t| t.rsplit('/').next()) else { continue };
             let target = attr(attrs, "Target").unwrap_or("");
@@ -329,3 +366,36 @@ fn holds_something_uncached(zip: &mut Zip<'_>, name: &str, marker: &'static str,
         buf.clear();
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::io::Write as _;
+
+    use super::*;
+
+    /// A package with `count` relationship parts of `size` bytes each, none of them describing an
+    /// existing part.
+    fn many_rels(count: usize, size: usize) -> Vec<u8> {
+        let mut z = zip::ZipWriter::new(Cursor::new(Vec::new()));
+        let opts = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated);
+        let head = r#"<?xml version="1.0"?><Relationships xmlns="http://schemas.openxmlformats.org/package/2006/relationships"><Relationship Id="rId1" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/custom" Target="https://example.com" TargetMode="External"/>"#;
+        let tail = "</Relationships>";
+        let filler = " ".repeat(size.saturating_sub(head.len() + tail.len()));
+        for i in 0..count {
+            z.start_file(format!("xl/_rels/part{i}.xml.rels"), opts).unwrap();
+            z.write_all(head.as_bytes()).unwrap();
+            z.write_all(filler.as_bytes()).unwrap();
+            z.write_all(tail.as_bytes()).unwrap();
+        }
+        z.finish().unwrap().into_inner()
+    }
+
+    #[test]
+    fn relationship_parts_count_against_the_decompression_budget() {
+        // Each part fits the per-part allowance, but together they need more than the budget.
+        let bytes = many_rels(8, 64 * 1024);
+        assert!(relationship_kinds_within(&bytes, 256 * 1024).is_err());
+        let kinds = relationship_kinds_within(&bytes, 1024 * 1024).unwrap();
+        assert_eq!(kinds.get("custom").map(String::as_str), Some("custom"));
+    }
+}

crates/engine/src/cmd/edit/find.rs

@@ -104,13 +104,6 @@ pub(super) fn find(s: &mut Session, p: &Json) -> Result<Json> {
     Ok(json!({"cell": c.a1(), "sheet": si, "value": t, "count": hits.len()}))
 }
 
-/// Longest text one replacement may give a cell, in bytes: Excel's 32,767 characters, each up to
-/// four bytes. Every occurrence copies the replacement text, so a short cell full of matches and a
-/// long replacement would otherwise expand to gigabytes.
-const REPLACE_CELL_MAX_BYTES: usize = 32_767 * 4;
-/// Most bytes of replaced text one Replace All builds across all its cells.
-const REPLACE_TOTAL_MAX_BYTES: usize = 256 * 1024 * 1024;
-
 pub(super) fn replace(s: &mut Session, p: &Json) -> Result<Json> {
     let what = str_param(p, "what").unwrap_or("").to_string();
     let with = str_param(p, "with").unwrap_or("").to_string();
@@ -126,7 +119,6 @@ pub(super) fn replace(s: &mut Session, p: &Json) -> Result<Json> {
     let n = edit(s, |cx| {
         let sheets: Vec<usize> = if workbook { (0..cx.wb.sheets.len()).collect() } else { vec![cx.wb.active_sheet()] };
         let mut n = 0;
-        let mut total = 0usize;
         for si in sheets {
             let cells: Vec<(CellRef, String)> =
                 cx.wb.sheet(si).map(|sh| sh.cells.iter().map(|(c, _)| (c, sh.input_text(c))).collect()).unwrap_or_default();
@@ -138,19 +130,12 @@ pub(super) fn replace(s: &mut Session, p: &Json) -> Result<Json> {
                     continue;
                 }
                 let new = if whole {
-                    Some(with.clone()).filter(|w| w.len() <= REPLACE_CELL_MAX_BYTES)
+                    with.clone()
                 } else if case {
-                    replace_cs_within(&text, &what, &with, REPLACE_CELL_MAX_BYTES)
+                    text.replace(&what, &with)
                 } else {
-                    replace_ci_within(&text, &what, &with, REPLACE_CELL_MAX_BYTES)
+                    replace_ci(&text, &what, &with)
                 };
-                let Some(new) = new else {
-                    return Err(EngineError::Other(format!("The replacement makes the text of {} too long.", c.a1())));
-                };
-                total = total.saturating_add(new.len());
-                if total > REPLACE_TOTAL_MAX_BYTES {
-                    return Err(EngineError::Other("The replacements make too much text. Replace in a smaller range.".into()));
-                }
                 // A Python cell's text is its code: the replacement edits the code and the cell
                 // stays a Python cell. Code that would no longer fit in a PY formula is left as it was.
                 let old_py = cx.wb.sheet(si).and_then(|sh| sh.cell(c)).and_then(|x| x.py.clone());
@@ -205,37 +190,7 @@ fn folds_in_place(s: &str) -> bool {
     })
 }
 
-#[cfg(test)]
 pub(super) fn replace_ci(text: &str, what: &str, with: &str) -> String {
-    replace_ci_within(text, what, with, usize::MAX).unwrap_or_default()
-}
-
-/// `text` with every `what` replaced by `with`, or `None` once the result passes `limit` bytes
-/// (checked before each copy, so nothing larger is allocated).
-pub(super) fn replace_cs_within(text: &str, what: &str, with: &str, limit: usize) -> Option<String> {
-    if what.is_empty() {
-        return (text.len() <= limit).then(|| text.to_string());
-    }
-    let mut out = String::new();
-    let mut i = 0;
-    for (pos, m) in text.match_indices(what) {
-        if out.len() + (pos - i) + with.len() > limit {
-            return None;
-        }
-        out.push_str(text.get(i..pos).unwrap_or(""));
-        out.push_str(with);
-        i = pos + m.len();
-    }
-    let rest = text.get(i..).unwrap_or("");
-    if out.len() + rest.len() > limit {
-        return None;
-    }
-    out.push_str(rest);
-    Some(out)
-}
-
-/// Case-insensitive [`replace_cs_within`].
-pub(super) fn replace_ci_within(text: &str, what: &str, with: &str, limit: usize) -> Option<String> {
     let lower = text.to_lowercase();
     let w = what.to_lowercase();
     // Slicing `text` at positions found in `lower` is only sound when folding kept every byte
@@ -243,22 +198,15 @@ pub(super) fn replace_ci_within(text: &str, what: &str, with: &str, limit: usize
     // boundaries (U+212A KELVIN SIGN shrinks 3→1 bytes, U+0130 grows 2→3), where the tail used
     // to be dropped. Those fall back to the case-sensitive replace, as length changes already do.
     if lower.len() != text.len() || w.is_empty() || !folds_in_place(text) || !folds_in_place(what) {
-        return replace_cs_within(text, what, with, limit);
+        return text.replace(what, with);
     }
     let mut out = String::new();
     let mut i = 0;
     while let Some(pos) = lower.get(i..).and_then(|s| s.find(&w)) {
-        if out.len() + pos + with.len() > limit {
-            return None;
-        }
         out.push_str(text.get(i..i + pos).unwrap_or(""));
         out.push_str(with);
         i += pos + w.len();
     }
-    let rest = text.get(i..).unwrap_or("");
-    if out.len() + rest.len() > limit {
-        return None;
-    }
-    out.push_str(rest);
-    Some(out)
+    out.push_str(text.get(i..).unwrap_or(""));
+    out
 }

crates/engine/src/cmd/edit/tests.rs

@@ -353,36 +353,6 @@ fn cut_paste_that_overflows_the_grid_moves_nothing() {
     assert_eq!(get(&mut s, "XFD2")["value"], json!(2.0));
 }
 
-#[test]
-fn replace_refuses_text_that_would_grow_past_a_cell() {
-    // 10,000 matches × a 1,000-character replacement would make a 10 MB cell (and a few such
-    // cells gigabytes); the replacement is refused before that text is built.
-    let mut s = Session::new();
-    s.new_workbook();
-    let long = "a".repeat(10_000);
-    s.execute("range.setValues", json!({"range": "A1", "values": [[long.clone()], ["abc"]]})).unwrap();
-    for case in [false, true] {
-        let err = s.execute("edit.replace", json!({"what": "a", "with": "x".repeat(1_000), "matchCase": case})).expect_err("too long");
-        assert!(err.to_string().contains("too long"), "{err}");
-        assert_eq!(s.execute("cell.get", json!({"cell": "A1"})).unwrap()["value"], json!(long));
-        assert_eq!(s.execute("cell.get", json!({"cell": "A2"})).unwrap()["value"], json!("abc"));
-    }
-    // A replacement that stays within a cell's limit still happens.
-    s.execute("edit.replace", json!({"what": "b", "with": "x".repeat(100)})).unwrap();
-    assert_eq!(s.execute("cell.get", json!({"cell": "A2"})).unwrap()["value"], json!(format!("a{}c", "x".repeat(100))));
-}
-
-#[test]
-fn bounded_replace_stops_at_its_limit() {
-    use super::find::{replace_ci_within, replace_cs_within};
-    assert_eq!(replace_cs_within("abab", "a", "xy", 6).as_deref(), Some("xybxyb"));
-    assert_eq!(replace_cs_within("abab", "a", "xy", 5), None);
-    assert_eq!(replace_ci_within("AbAb", "a", "xy", 6).as_deref(), Some("xybxyb"));
-    assert_eq!(replace_ci_within("AbAb", "a", "xy", 5), None);
-    assert_eq!(replace_cs_within("abc", "z", "", 3).as_deref(), Some("abc"));
-    assert_eq!(replace_cs_within("abc", "z", "", 2), None);
-}
-
 #[test]
 fn replace_case_insensitive() {
     assert_eq!(replace_ci("Apple apple APPLE", "apple", "pear"), "pear pear pear");