diff --git a/crates/xlsx/src/controls.rs b/crates/xlsx/src/controls.rs index cf38d0908..75c8032df 100644 --- a/crates/xlsx/src/controls.rs +++ b/crates/xlsx/src/controls.rs @@ -249,7 +249,7 @@ pub const SHAPETYPE: &str = ", link: Option, @@ -420,6 +420,11 @@ pub fn read(cx: &mut Ctx<'_>, controls: Option<&El>, vml_rid: Option<&str>, rels for r in rels { rel_by_id.entry(r.id.as_str()).or_insert(r); } + // Each properties part is read and parsed once, however many controls name it: every read is + // charged to the package's budget, so a file whose controls all point at one part would + // otherwise spend it on that part alone. A part that fails to read, or isn't a + // `formControlPr`, is remembered as nothing and not read again. + let mut props_by_part: HashMap<&str, Option> = HashMap::new(); // Every shape the sheet lists, read here or not. let mut listed: HashSet = HashSet::new(); for ctl in controls.map(controls_in).unwrap_or_default() { @@ -430,9 +435,12 @@ pub fn read(cx: &mut Ctx<'_>, controls: Option<&El>, vml_rid: Option<&str>, rels } let shape = shape_of(n); let vml_props = shape.and_then(props_from_vml); - let prop_part = ctl.attr("id").and_then(|id| rel_by_id.get(id)).map(|r| r.target.clone()); + let prop_part = ctl.attr("id").and_then(|id| rel_by_id.get(id)).map(|r| r.target.as_str()); let file_props = match prop_part { - Some(t) => cx.pkg.read_xml(&t).ok().flatten().filter(|e| e.name == "formControlPr").map(|e| props_from_ctrl_prop(&e)), + Some(t) => props_by_part + .entry(t) + .or_insert_with(|| cx.pkg.read_xml(t).ok().flatten().filter(|e| e.name == "formControlPr").map(|e| props_from_ctrl_prop(&e))) + .clone(), None => None, }; let pr = ctl.child("controlPr"); diff --git a/crates/xlsx/src/tests/controls_tests.rs b/crates/xlsx/src/tests/controls_tests.rs index b58d1f200..7aedc51d3 100644 --- a/crates/xlsx/src/tests/controls_tests.rs +++ b/crates/xlsx/src/tests/controls_tests.rs @@ -333,3 +333,44 @@ fn controls_come_from_the_legacy_drawing_not_the_header_and_footer_drawing() { assert_eq!(c.len(), 3, "{c:?}"); assert_eq!(c[2].text, "Go"); } + +/// Every listed control that names the same properties part shares one read of it. Each read is +/// charged to the package's 1 GB budget, so 10,000 reads of a 200 KB part would spend it, drop the +/// controls after that, and fail the parts read after the controls (here the sheet's notes). A +/// hostile file can list that many controls against one part. +#[test] +fn controls_naming_one_properties_part_read_it_once() { + let n = 13_000; + let mut controls = String::from(r#""#); + for i in 0..n { + let _ = std::fmt::Write::write_fmt( + &mut controls, + format_args!( + r#"10{i}020{}0"#, + 2000 + i, + i + 1 + ), + ); + } + controls.push_str(""); + let rels = r#""#; + // 200 KB of attribute whitespace: read once per control, 10,000 reads would be 2 GB. + let prop_a = format!( + r#""#, + " ".repeat(200 * 1024) + ); + let notes = r#"Annread after the controls"#; + let bytes = minimal( + &format!("{controls}"), + &[("xl/worksheets/_rels/sheet1.xml.rels", rels), ("xl/ctrlProps/a.xml", &prop_a), ("xl/comments1.xml", notes)], + "", + "", + ); + let (wb, report) = read_xlsx(&bytes).unwrap(); + let sh = wb.sheet(0).unwrap(); + assert_eq!(sh.controls.len(), 10_000, "the cap of one sheet, every control read from the one part"); + assert!(sh.controls.iter().all(|c| { + c.kind == ControlKind::Slider && c.link.as_deref() == Some("$B$1") && (c.min, c.max, c.step, c.page, c.value) == (0, 200, 5, 20, 45) + })); + assert_eq!(sh.comments.len(), 1, "a part read after the controls still reads: {:?}", report.warnings); +}