From 4d3fe2aa1ac041df7fa4d785beb8429d66678ed8 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 15:59:59 +0000 Subject: [PATCH] svg import: cap attributes per element, map span offsets to characters once --- crates/svg/src/import.rs | 35 ++++++++++++++++++++++---- crates/svg/src/import/budget.rs | 2 +- crates/svg/src/import/hostile_tests.rs | 12 +++++++++ crates/svg/src/import/text.rs | 12 ++++++--- crates/svg/src/import/text_tests.rs | 8 ++++++ 5 files changed, 60 insertions(+), 9 deletions(-) diff --git a/crates/svg/src/import.rs b/crates/svg/src/import.rs index 1ac8a78bf..f77b10bfa 100644 --- a/crates/svg/src/import.rs +++ b/crates/svg/src/import.rs @@ -704,13 +704,28 @@ fn simple_hash(b: &[u8]) -> String { /// Deepest element nesting the parser is given: deeper documents would exhaust the stack. const MAX_NESTING: usize = 512; +/// The most attributes one element may have. The XML parser checks each attribute against the +/// earlier ones, so a huge count is quadratic work; real elements have a few dozen at most. +const MAX_ATTRIBUTES: usize = 4096; + /// How deep elements nest in `data`: a scan of its tags (comments, CDATA, declarations and quoted /// attribute values skipped) that stops once past `limit`. pub(crate) fn nesting(data: &[u8], limit: usize) -> usize { + scan(data, limit).0 +} + +/// Whether an element of `data` has more than [`MAX_ATTRIBUTES`] attributes (counted as quoted +/// values, before the parser sees them). +pub(crate) fn too_wide(data: &[u8]) -> bool { + scan(data, usize::MAX).1 > MAX_ATTRIBUTES +} + +/// [`nesting`] and the most attribute values on one element; stops once either is past its limit. +fn scan(data: &[u8], limit: usize) -> (usize, usize) { let find = |from: usize, pat: &[u8]| -> usize { data.get(from..).and_then(|r| r.windows(pat.len()).position(|w| w == pat)).map_or(data.len(), |p| from + p + pat.len()) }; - let (mut depth, mut max, mut i) = (0usize, 0usize, 0usize); + let (mut depth, mut max, mut i, mut widest) = (0usize, 0usize, 0usize, 0usize); while let Some(off) = data.get(i..).and_then(|r| r.iter().position(|b| *b == b'<')) { i += off; let rest = data.get(i..).unwrap_or(&[]); @@ -725,12 +740,15 @@ pub(crate) fn nesting(data: &[u8], limit: usize) -> usize { i = find(i + 2, b">"); } else { // A start tag: to its `>` outside quotes; `/>` closes it. - let (mut quote, mut j, mut closed) = (None, i + 1, false); + let (mut quote, mut j, mut closed, mut attrs) = (None, i + 1, false, 0usize); while let Some(&c) = data.get(j) { match quote { Some(q) if c == q => quote = None, Some(_) => {} - None if c == b'"' || c == b'\'' => quote = Some(c), + None if c == b'"' || c == b'\'' => { + quote = Some(c); + attrs += 1; + } None if c == b'>' => { closed = data.get(j.wrapping_sub(1)) == Some(&b'/'); break; @@ -739,17 +757,21 @@ pub(crate) fn nesting(data: &[u8], limit: usize) -> usize { } j += 1; } + widest = widest.max(attrs); + if widest > MAX_ATTRIBUTES { + return (max, widest); + } if !closed { depth += 1; max = max.max(depth); if max > limit { - return max; + return (max, widest); } } i = j + 1; } } - max + (max, widest) } /// Whether `data` nests elements more than [`MAX_NESTING`] deep: the byte scan first, then the @@ -883,6 +905,9 @@ fn import_within( ) -> Result { // SVGZ is unpacked here, within a budget, so every check below sees the real document. let data = budget::document(data, &limits).map_err(ImportError)?; + if too_wide(&data) { + return Err(ImportError(format!("an element has more than {MAX_ATTRIBUTES} attributes"))); + } if too_deep(&data) { return Err(ImportError(format!("elements nested more than {MAX_NESTING} deep"))); } diff --git a/crates/svg/src/import/budget.rs b/crates/svg/src/import/budget.rs index 7de080af4..c50266dc0 100644 --- a/crates/svg/src/import/budget.rs +++ b/crates/svg/src/import/budget.rs @@ -87,7 +87,7 @@ fn embedded(used: &Used, limits: &Limits, data: &[u8], opts: &Options) -> Option } }; used.bytes.fetch_add(plain.len(), Ordering::Relaxed); - if super::too_deep(&plain) { + if super::too_wide(&plain) || super::too_deep(&plain) { return None; } used.depth.fetch_add(1, Ordering::Relaxed); diff --git a/crates/svg/src/import/hostile_tests.rs b/crates/svg/src/import/hostile_tests.rs index 0af1297d0..16731407c 100644 --- a/crates/svg/src/import/hostile_tests.rs +++ b/crates/svg/src/import/hostile_tests.rs @@ -181,3 +181,15 @@ fn entity_expansion_cannot_bypass_the_nesting_limit() { let err = import_svg(&mut d, data.as_bytes(), p, 0.0, 0.0, "x").err().map(|e| e.to_string()).unwrap_or_default(); assert!(err.contains("nested more than 512 deep"), "{err:?}"); } + +#[test] +fn an_element_with_thousands_of_attributes_is_refused_before_parsing() { + use super::too_wide; + let many = |n: usize| (0..n).map(|i| format!(" a{i}=\"1\"")).collect::(); + assert!(!too_wide(format!("", many(4096)).as_bytes())); + assert!(too_wide(format!("", many(4097)).as_bytes())); + assert!(!too_wide(format!("{}", "".repeat(10_000)).as_bytes()), "per element, not per file"); + let mut d = Document::new("t"); + let p = d.first_page().unwrap(); + assert!(import_svg(&mut d, &svg(&format!("", many(100_000))), p, 0.0, 0.0, "x").is_err()); +} diff --git a/crates/svg/src/import/text.rs b/crates/svg/src/import/text.rs index cee54bbd8..7ed8d018f 100644 --- a/crates/svg/src/import/text.rs +++ b/crates/svg/src/import/text.rs @@ -195,7 +195,7 @@ pub(crate) fn text(cx: &mut Ctx, t: &usvg::Text, parent: NodeId, base: Affine, d let x = chunk.x().map_or(pen_x, f64::from) + d(t.dx()); let y = chunk.y().map_or(pen_y, f64::from) + d(t.dy()); char_at += count; - let ci = |b: usize| text.get(..b).map_or(count, |p| p.chars().count()); + let ci = char_index(text); let spans: Vec<_> = chunk.spans().iter().map(|sp| (ci(sp.start()), ci(sp.end()), sp, span_style(sp, s, fonts))).collect(); if spans.is_empty() { continue; @@ -229,6 +229,13 @@ pub(crate) fn text(cx: &mut Ctx, t: &usvg::Text, parent: NodeId, base: Affine, d } } +/// The characters of `text` before a byte offset (all of them for an offset that is past the end +/// or inside a character), by a search in a table built once: many spans don't rescan the prefix. +fn char_index(text: &str) -> impl Fn(usize) -> usize + '_ { + let starts: Vec = text.char_indices().map(|(i, _)| i).collect(); + move |b| if text.is_char_boundary(b) { starts.partition_point(|s| *s < b) } else { starts.len() } +} + /// The text of `lines` (one per line, stacked at their pitch; `s`: their scale): characters, the /// first span's style and a run per span, fills still to come. fn text_data(lines: &[Line], s: f64) -> TextData { @@ -262,8 +269,7 @@ fn on_path(cx: &mut Ctx, t: &usvg::Text, world: Affine, parent: NodeId) -> bool let Some(len) = figcraft_geom::measure::polylines(&path).and_then(|l| l.first().map(|l| l.length())).filter(|l| *l > 1e-9) else { return false }; let fonts = cx.fonts.get_or_insert_with(Fonts::load); let text = chunk.text(); - let count = text.chars().count(); - let ci = |b: usize| text.get(..b).map_or(count, |p| p.chars().count()); + let ci = char_index(text); let spans: Vec<_> = chunk.spans().iter().map(|sp| (ci(sp.start()), ci(sp.end()), sp, span_style(sp, 1.0, fonts))).collect(); let line = Line { x: 0.0, y: 0.0, anchor: chunk.anchor(), text: text.to_string(), spans }; let mut td = text_data(std::slice::from_ref(&line), 1.0); diff --git a/crates/svg/src/import/text_tests.rs b/crates/svg/src/import/text_tests.rs index f267b2947..94c6ae5b2 100644 --- a/crates/svg/src/import/text_tests.rs +++ b/crates/svg/src/import/text_tests.rs @@ -30,3 +30,11 @@ fn css_system_font_names_keep_their_name_where_the_system_font_is() { assert_eq!(family(&named(&["-apple-system", "BlinkMacSystemFont", "system-ui", "sans-serif"]), &elsewhere), "Inter"); assert_eq!(family(&named(&["-apple-system"]), &elsewhere), "-apple-system", "nothing installed: the first name it gives"); } + +#[test] +fn char_index_counts_the_characters_before_a_byte_offset() { + let t = "aé😀b"; + let ci = super::char_index(t); + assert_eq!([ci(0), ci(1), ci(3), ci(7), ci(8)], [0, 1, 2, 3, 4]); + assert_eq!([ci(2), ci(4), ci(99)], [4, 4, 4], "inside a character or past the end: all of them"); +}