Parse each package part once, and bound the total

src/formats/docx/content.rs

@@ -633,7 +633,7 @@ impl<'a, 'b, 'e> InlineWalker<'a, 'b, 'e> {
         let Some((part, bytes)) = self.ctx.rel_part(rel_id)? else {
             return Ok(Vec::new());
         };
-        match crate::package::xml::parse_xml(&bytes) {
+        match self.ctx.pkg.borrow_mut().parse_part(&part, &bytes) {
             Ok(root) => Ok(crate::shared::drawingml::chart_blocks(&root)),
             Err(e) if e.is_fatal() => Err(e),
             Err(e) => {
@@ -648,7 +648,7 @@ impl<'a, 'b, 'e> InlineWalker<'a, 'b, 'e> {
         let Some((part, bytes)) = self.ctx.rel_part(rel_id)? else {
             return Ok(Vec::new());
         };
-        match crate::package::xml::parse_xml(&bytes) {
+        match self.ctx.pkg.borrow_mut().parse_part(&part, &bytes) {
             Ok(root) => Ok(crate::shared::drawingml::diagram_blocks(&root)),
             Err(e) if e.is_fatal() => Err(e),
             Err(e) => {

src/formats/pptx/mod.rs

@@ -13,7 +13,7 @@ use crate::model::{
 use crate::package::relationships::{
     RelTarget, Relationships, TargetMode, read_rels, rel_target_bytes, rel_type, rels_part_for,
 };
-use crate::package::xml::{Element, ns, parse_xml};
+use crate::package::xml::{Element, ns};
 use crate::package::{Package, archive::probe_ole, path};
 use crate::shared::assets::AssetSink;
 use crate::shared::delta::rebase_emphasis;
@@ -580,7 +580,7 @@ fn parse_graphic_frame(
         && let Some(rid) = chart_ref.attr_qualified(ns::R, "id")
         && let Some((part, bytes)) = ctx.rel_part(rid)?
     {
-        match parse_xml(&bytes) {
+        match ctx.pkg.borrow_mut().parse_part(&part, &bytes) {
             Ok(root) => blocks.extend(crate::shared::drawingml::chart_blocks(&root)),
             Err(e) if e.is_fatal() => return Err(e),
             Err(e) => log::warn!("skipping corrupt chart part {part}: {e}"),
@@ -591,7 +591,7 @@ fn parse_graphic_frame(
         && let Some(rid) = rel_ids.attr_qualified(ns::R, "dm")
         && let Some((part, bytes)) = ctx.rel_part(rid)?
     {
-        match parse_xml(&bytes) {
+        match ctx.pkg.borrow_mut().parse_part(&part, &bytes) {
             Ok(root) => blocks.extend(crate::shared::drawingml::diagram_blocks(&root)),
             Err(e) if e.is_fatal() => return Err(e),
             Err(e) => log::warn!("skipping corrupt diagram part {part}: {e}"),

src/package/archive.rs

@@ -2,7 +2,7 @@
 
 use crate::error::ConvertError;
 use crate::package::limits;
-use crate::package::xml::{Element, parse_xml};
+use crate::package::xml::{Element, parse_xml_counted};
 use std::collections::HashMap;
 use std::io::{Cursor, Read};
 use std::rc::Rc;
@@ -17,6 +17,19 @@ pub struct Package<'a> {
     /// reference one part many times). Bounded by `MAX_TOTAL_BYTES`. Buffers
     /// are shared (`Rc`), so a cache hit never copies the bytes.
     cache: HashMap<String, Rc<[u8]>>,
+    /// Parsed part trees by normalized name, shared (`Rc`) with callers: a
+    /// part referenced many times parses once. Holds up to
+    /// `MAX_CACHED_XML_NODES`; past that a part is returned uncached and
+    /// re-parses on its next reference.
+    trees: HashMap<String, Rc<Element>>,
+    /// Nodes currently held in `trees`, against `MAX_CACHED_XML_NODES`.
+    cached_nodes: u64,
+    /// XML nodes parsed out of this package so far, counting each parse
+    /// separately. The tree cache removes the repeats a document actually
+    /// has; this bounds the ones it cannot hold, so a package pointing at an
+    /// uncacheable part many times still terminates. Bounded by
+    /// `MAX_DOCUMENT_XML_NODES`.
+    parsed_nodes: u64,
 }
 
 impl<'a> Package<'a> {
@@ -29,7 +42,14 @@ impl<'a> Package<'a> {
                 detail: format!("archive contains {} entries", zip.len()),
             });
         }
-        Ok(Package { zip, total_read: 0, cache: HashMap::new() })
+        Ok(Package {
+            zip,
+            total_read: 0,
+            cache: HashMap::new(),
+            trees: HashMap::new(),
+            cached_nodes: 0,
+            parsed_nodes: 0,
+        })
     }
 
     /// Read a part's bytes. `Ok(None)` means the part is absent (a valid
@@ -116,11 +136,11 @@ impl<'a> Package<'a> {
     /// Read and parse an optional XML part under the unified recovery policy:
     /// absent -> `Ok(None)`; unreadable or corrupt -> skipped with a log;
     /// fatal resource-limit errors always propagate.
-    pub fn optional_xml_part(&mut self, name: &str) -> Result<Option<Element>, ConvertError> {
+    pub fn optional_xml_part(&mut self, name: &str) -> Result<Option<Rc<Element>>, ConvertError> {
         let Some(bytes) = self.optional_part(name)? else {
             return Ok(None);
         };
-        match parse_xml(&bytes) {
+        match self.parse_part(name, &bytes) {
             Ok(tree) => Ok(Some(tree)),
             Err(e) if e.is_fatal() => Err(e),
             Err(e) => {
@@ -132,9 +152,28 @@ impl<'a> Package<'a> {
 
     /// Read and parse an XML part that must exist and parse for any
     /// meaningful output.
-    pub fn required_xml_part(&mut self, name: &str) -> Result<Element, ConvertError> {
+    pub fn required_xml_part(&mut self, name: &str) -> Result<Rc<Element>, ConvertError> {
         let bytes = self.required_part(name)?;
-        parse_xml(&bytes)
+        self.parse_part(name, &bytes)
+    }
+
+    /// Parse one of this package's parts, serving a part already parsed from
+    /// the tree cache. Parts reached by relationship (charts, diagrams) come
+    /// through here with the bytes the relationship resolved to, so that they
+    /// share the cache and the node total with the rest of the package.
+    pub fn parse_part(&mut self, name: &str, bytes: &[u8]) -> Result<Rc<Element>, ConvertError> {
+        let name = name.trim_start_matches('/');
+        if let Some(tree) = self.trees.get(name) {
+            return Ok(Rc::clone(tree));
+        }
+        let before = self.parsed_nodes;
+        let tree = Rc::new(parse_xml_counted(bytes, &mut self.parsed_nodes)?);
+        let nodes = self.parsed_nodes - before;
+        if self.cached_nodes + nodes <= limits::MAX_CACHED_XML_NODES {
+            self.cached_nodes += nodes;
+            self.trees.insert(name.to_string(), Rc::clone(&tree));
+        }
+        Ok(tree)
     }
 }
 
@@ -179,6 +218,45 @@ mod tests {
         assert_eq!(pkg.total_read, 4096, "repeated reads must not re-charge the budget");
     }
 
+    #[test]
+    fn a_part_referenced_repeatedly_is_parsed_once() {
+        let data = one_part_zip("a.xml", b"<a><b/><b/><b/></a>");
+        let mut pkg = Package::open(&data).unwrap();
+        let first = pkg.required_xml_part("a.xml").unwrap();
+        let per_parse = pkg.parsed_nodes;
+        assert!(per_parse > 0);
+        for _ in 0..64 {
+            let again = pkg.required_xml_part("a.xml").unwrap();
+            assert!(Rc::ptr_eq(&first, &again), "a repeat reference must share the parsed tree");
+        }
+        assert_eq!(pkg.parsed_nodes, per_parse, "a cached tree must not re-parse");
+        // The leading slash OPC part URIs carry must not miss the cache.
+        assert!(Rc::ptr_eq(&first, &pkg.required_xml_part("/a.xml").unwrap()));
+    }
+
+    #[test]
+    fn a_part_too_large_to_cache_re_parses_and_hits_the_document_total() {
+        let data = one_part_zip("a.xml", b"<a><b/><b/><b/></a>");
+        let mut pkg = Package::open(&data).unwrap();
+        // A package whose cache is full of earlier parts: this one is served
+        // uncached, so every reference to it parses again. The per-part cap
+        // resets on each of those parses and never fires; the running total
+        // is what stops it.
+        pkg.cached_nodes = limits::MAX_CACHED_XML_NODES;
+        pkg.required_xml_part("a.xml").unwrap();
+        assert!(pkg.trees.is_empty(), "a full cache must not grow past its budget");
+        let per_parse = pkg.parsed_nodes;
+        pkg.required_xml_part("a.xml").unwrap();
+        assert_eq!(pkg.parsed_nodes, per_parse * 2, "an uncached repeat must be charged again");
+
+        pkg.parsed_nodes = limits::MAX_DOCUMENT_XML_NODES;
+        let err = pkg.required_xml_part("a.xml").unwrap_err();
+        assert!(
+            matches!(err, ConvertError::ResourceLimit { limit: "max_document_xml_nodes", .. }),
+            "expected max_document_xml_nodes, got: {err}"
+        );
+    }
+
     #[test]
     fn total_budget_exhaustion_reports_max_total_bytes() {
         let mut w = zip::ZipWriter::new(Cursor::new(Vec::new()));

src/package/limits.rs

@@ -23,6 +23,25 @@ pub const MAX_XML_DEPTH: usize = 256;
 /// memory test) so a saturating part stays around the archive budget.
 pub const MAX_XML_NODES: usize = 2_000_000;
 
+/// Maximum XML nodes parsed across one document, summed over every part and
+/// every time a part is parsed. The per-part cap above resets on each parse,
+/// so a package that references one part many times (an EPUB reading order,
+/// a presentation's slide list) can multiply it without bound; this is the
+/// ceiling on that product. Sized against `MAX_TOTAL_BYTES`: XML runs about
+/// 30-60 bytes per node, so a document that reads its whole decompression
+/// budget as markup lands here, and one that parses more than that is
+/// re-reading parts rather than covering new ones.
+pub const MAX_DOCUMENT_XML_NODES: u64 = 16_000_000;
+
+/// Maximum XML nodes held in a package's parsed-tree cache, which is what
+/// keeps a part referenced many times from being parsed many times. At the
+/// measured DOM cost (~430 bytes/node) this is ~86 MiB, and it is far above
+/// the parts documents actually repeat: style tables, slide layouts and
+/// masters, and chapter files all sit in the thousands of nodes. A part too
+/// large to cache re-parses per reference and is bounded by the document
+/// total above.
+pub const MAX_CACHED_XML_NODES: u64 = 200_000;
+
 /// Maximum content-bearing cells a repeat expansion may produce per table.
 pub const MAX_EXPANSION: u64 = 4_000_000;
 

src/package/xml.rs

@@ -192,8 +192,10 @@ impl<'a> Iterator for DescendantNodes<'a> {
 /// Parse an XML part into a synthetic root element containing the top-level
 /// nodes. Encoding comes from the BOM or XML declaration; the part is
 /// transcoded to UTF-8 before parsing so namespace resolution sees one
-/// consistent encoding.
-pub fn parse_xml(bytes: &[u8]) -> Result<Element, ConvertError> {
+/// consistent encoding. Nodes are charged against `document_nodes` as well
+/// as the per-part cap, so that a package re-parsing one part for every
+/// reference to it stays bounded.
+pub fn parse_xml_counted(bytes: &[u8], document_nodes: &mut u64) -> Result<Element, ConvertError> {
     let utf8 = to_utf8(bytes);
     let mut reader = NsReader::from_reader(utf8.as_ref());
     reader.config_mut().check_end_names = false;
@@ -202,7 +204,7 @@ pub fn parse_xml(bytes: &[u8]) -> Result<Element, ConvertError> {
     let mut root =
         Element { ns: None, local: String::new(), attrs: Vec::new(), children: Vec::new() };
     let mut stack: Vec<Element> = Vec::new();
-    let mut nodes: usize = 0;
+    let mut nodes = NodeCount { part: 0, document: document_nodes };
     let mut recovered = false;
 
     loop {
@@ -310,14 +312,34 @@ fn declared_encoding(head: &[u8]) -> Option<String> {
     Some(rest[..end].to_string())
 }
 
-fn bump_nodes(nodes: &mut usize) -> Result<(), ConvertError> {
-    *nodes += 1;
-    if *nodes > limits::MAX_XML_NODES {
+/// [`parse_xml_counted`] against a throwaway document total.
+#[cfg(test)]
+pub fn parse_xml(bytes: &[u8]) -> Result<Element, ConvertError> {
+    parse_xml_counted(bytes, &mut 0)
+}
+
+/// Node counters for one parse: the part's own count, and the running total
+/// for the document the part belongs to.
+struct NodeCount<'a> {
+    part: usize,
+    document: &'a mut u64,
+}
+
+fn bump_nodes(nodes: &mut NodeCount<'_>) -> Result<(), ConvertError> {
+    nodes.part += 1;
+    if nodes.part > limits::MAX_XML_NODES {
         return Err(ConvertError::ResourceLimit {
             limit: "max_xml_nodes",
             detail: format!("part exceeds {} xml nodes", limits::MAX_XML_NODES),
         });
     }
+    *nodes.document += 1;
+    if *nodes.document > limits::MAX_DOCUMENT_XML_NODES {
+        return Err(ConvertError::ResourceLimit {
+            limit: "max_document_xml_nodes",
+            detail: format!("document exceeds {} parsed xml nodes", limits::MAX_DOCUMENT_XML_NODES),
+        });
+    }
     Ok(())
 }
 

tests/gen_fixtures.py

@@ -1739,6 +1739,27 @@ def malformed():
         # structurally unusable -> the labelled raw-recovery path must run.
         raw = ppt.read_bytes().replace(b"\xf5\x0f", b"\xf5\xff")
         (m / "brokenpersist--recovers.ppt").write_bytes(raw)
+    # Every reading-order entry resolves to the package document itself, so
+    # the part read once per entry is the one that grows with the entry
+    # count. Parsing it per entry is quadratic; the package's parsed-tree
+    # cache is what keeps this linear. No entry yields a chapter, so the book
+    # itself is unreadable.
+    n = 6400
+    items = "".join(
+        f'<item id="i{i}" href="c.opf" media-type="application/xhtml+xml"/>'
+        for i in range(n))
+    refs = "".join(f'<itemref idref="i{i}"/>' for i in range(n))
+    write_zip(m / "selfref--errors.epub", [
+        ("META-INF/container.xml",
+         '<?xml version="1.0"?>'
+         '<container version="1.0" xmlns="urn:oasis:names:tc:opendocument:xmlns:container">'
+         '<rootfiles><rootfile full-path="c.opf"'
+         ' media-type="application/oebps-package+xml"/></rootfiles></container>'),
+        ("c.opf",
+         '<?xml version="1.0"?>'
+         '<package xmlns="http://www.idpf.org/2007/opf" version="3.0">'
+         f'<metadata/><manifest>{items}</manifest><spine>{refs}</spine></package>'),
+    ], mimetype_first="application/epub+zip")
 
 
 def abuse():

tests/snapshots/snapshots__malformed__selfref--errors.epub.snap

@@ -0,0 +1,5 @@
+---
+source: tests/snapshots.rs
+expression: output
+---
+ERROR: malformed document: no chapter in the book could be read