From 8a3ccd46ba204ae5fcd0778f20a0314fc0ce794c Mon Sep 17 00:00:00 2001
From: "Dylan M. Taylor" <dylan@dylanmtaylor.com>
Date: Tue, 3 Nov 2026 23:22:21 +0000
Subject: [PATCH] address review: entity_bytes takes the largest declaration of
 each name

---
 crates/svg/src/lib.rs   | 16 +++++++++++-----
 crates/svg/src/tests.rs |  8 ++++++++
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/crates/svg/src/lib.rs b/crates/svg/src/lib.rs
index c4d5aa74c..a6b73f787 100644
--- a/crates/svg/src/lib.rs
+++ b/crates/svg/src/lib.rs
@@ -1318,7 +1318,9 @@ const MAX_ENTITY_BYTES: usize = 64 << 20;
 /// references to nested entities could otherwise expand attribute values to gigabytes.
 fn entity_bytes(text: &str) -> usize {
     use std::collections::HashMap;
-    let mut decls: HashMap<&str, &str> = HashMap::new();
+    // Every declaration of a name is kept, comments and quoted text included: the parser uses
+    // only one of them, and the largest bounds whichever it is.
+    let mut decls: HashMap<&str, Vec<&str>> = HashMap::new();
     let mut rest = text;
     while let Some(p) = rest.find("<!ENTITY") {
         rest = &rest[p + 8..];
@@ -1332,7 +1334,7 @@ fn entity_bytes(text: &str) -> usize {
         let after = after.trim_start();
         let Some(q) = after.chars().next().filter(|c| *c == '"' || *c == '\'') else { continue };
         if let Some(end) = after[1..].find(q) {
-            decls.entry(name).or_insert(&after[1..1 + end]);
+            decls.entry(name).or_default().push(&after[1..1 + end]);
         }
     }
     if decls.is_empty() {
@@ -1341,13 +1343,17 @@ fn entity_bytes(text: &str) -> usize {
     fn refs(s: &str) -> impl Iterator<Item = &str> {
         s.split('&').skip(1).filter_map(|r| r.find(';').map(|e| &r[..e]))
     }
-    fn size<'t>(name: &'t str, decls: &HashMap<&'t str, &'t str>, memo: &mut HashMap<&'t str, usize>, depth: usize) -> usize {
+    fn size<'t>(name: &'t str, decls: &HashMap<&'t str, Vec<&'t str>>, memo: &mut HashMap<&'t str, usize>, depth: usize) -> usize {
         if let Some(&n) = memo.get(name) {
             return n;
         }
-        let Some(value) = decls.get(name) else { return 0 };
+        let Some(values) = decls.get(name) else { return 0 };
         // Nested past any real use (or a cycle, which the parser refuses): too large.
-        let n = if depth > 32 { usize::MAX } else { refs(value).fold(value.len(), |n, r| n.saturating_add(size(r, decls, memo, depth + 1))) };
+        let n = if depth > 32 {
+            usize::MAX
+        } else {
+            values.iter().map(|value| refs(value).fold(value.len(), |n, r| n.saturating_add(size(r, decls, memo, depth + 1)))).max().unwrap_or(0)
+        };
         memo.insert(name, n);
         n
     }
diff --git a/crates/svg/src/tests.rs b/crates/svg/src/tests.rs
index 93a4ffc6e..f10057e6f 100644
--- a/crates/svg/src/tests.rs
+++ b/crates/svg/src/tests.rs
@@ -476,6 +476,14 @@ fn entity_references_that_expand_past_the_limit_are_refused() {
     assert!(parse(ok.as_bytes()).is_ok());
 }
 
+#[test]
+fn a_decoy_entity_declaration_does_not_hide_the_real_one() {
+    // A commented-out `b` comes first; the parser uses the later `b = &e;` (1 MiB each, 100 times).
+    let dtd = format!("<!-- <!ENTITY b \"x\"> --><!ENTITY e \"{}\"><!ENTITY b \"&e;\">", "x".repeat(1 << 20));
+    let svg = format!("<!DOCTYPE svg [{dtd}]><svg xmlns=\"http://www.w3.org/2000/svg\"><g id=\"{}\"/></svg>", "&b;".repeat(100));
+    assert!(matches!(parse(svg.as_bytes()), Err(Error::Xml(e)) if e.contains("entity")));
+}
+
 #[test]
 fn gradient_stops_copied_past_the_budget_are_refused() {
     // 4,000 stops referenced by 2,100 shapes would keep 8.4 million stops.