From 0ef62376ffd17a6ea4f5f6540afa1abc2a989681 Mon Sep 17 00:00:00 2001
From: Andreas Stefl <stefl.andreas@gmail.com>
Date: Fri, 14 Aug 2026 21:32:08 +0200
Subject: [PATCH 1/2] fix: walk a style's parent chain instead of recursing it,
 and read an xml part once

Two crashes reported from Play, both on documents rather than on anything
the caller does.

A `w:basedOn` / `style:parent-style-name` chain was resolved one stack frame
per link, so a document declaring enough styles in one chain overflowed the
stack of the thread that opened it - an http worker's, which is small. Both
registries now walk the chain onto a stack and build it from the root down;
the cycle guard stays what it was, an entry present but still null.

Reading a part through pugixml's stream loader holds it twice - the chunk
list it reads into, then the contiguous buffer it parses in place. The file
knows its size, so it is read once into a buffer pugixml is handed and frees.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WACM9eXR7F5aZ22tyq2jwU
---
 CHANGELOG.md                                  |   5 +
 src/odr/internal/odf/odf_style.cpp            |  60 ++++++---
 .../internal/ooxml/text/ooxml_text_style.cpp  |  51 +++++---
 src/odr/internal/util/xml_util.cpp            |  29 ++++-
 test/CMakeLists.txt                           |   1 +
 .../internal/ooxml/ooxml_text_style_test.cpp  | 116 ++++++++++++++++++
 6 files changed, 225 insertions(+), 37 deletions(-)
 create mode 100644 test/src/internal/ooxml/ooxml_text_style_test.cpp

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d3a744bcb..9a2403786 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -45,6 +45,11 @@ The release run heads these entries with the version and opens a fresh
   instead of in the running text.
 - A text document is laid out on the master page it names, so a letter template
   keeps the margins that leave room for its letterhead.
+- A docx or odt that chains its styles deeply opens instead of taking the
+  process down with it: the `w:basedOn` / `style:parent-style-name` chain is
+  walked onto a stack rather than recursed, so its length costs no stack.
+- A document's xml parts are read once instead of buffered twice on the way into
+  the parser, which lowers the memory opening a large one takes.
 
 ## v6.5.0 - 2026-08-10
 
diff --git a/src/odr/internal/odf/odf_style.cpp b/src/odr/internal/odf/odf_style.cpp
index c5a0370b0..1e77108ec 100644
--- a/src/odr/internal/odf/odf_style.cpp
+++ b/src/odr/internal/odf/odf_style.cpp
@@ -5,8 +5,10 @@
 
 #include <cstdlib>
 #include <cstring>
+#include <ranges>
 #include <unordered_map>
 #include <utility>
+#include <vector>
 
 namespace odr::internal::odf {
 
@@ -603,34 +605,54 @@ Style *StyleRegistry::generate_default_style_(const std::string &name,
   return style.get();
 }
 
+/// Walks the `style:parent-style-name` chain onto a stack and builds it from
+/// the root down; recursing it costs a stack frame per link.
 Style *StyleRegistry::generate_style_(const std::string &name,
                                       const pugi::xml_node node) {
-  // a null entry means the style is still resolving, i.e. the parent chain is
-  // cyclic; break it rather than recurse forever
-  const auto [style_it, inserted] = m_styles.try_emplace(name);
-  std::unique_ptr<Style> &style = style_it->second;
-  if (!inserted) {
-    return style.get();
-  }
+  // the names are the map's own, which keep their addresses across a rehash
+  std::vector<std::pair<const std::string *, pugi::xml_node>> chain;
 
+  const std::string *current_name = &name;
+  pugi::xml_node current_node = node;
   Style *parent{nullptr};
-  if (const pugi::xml_attribute parent_attr =
-          node.attribute("style:parent-style-name");
-      parent_attr) {
-    if (const auto parent_it = m_index_style.find(parent_attr.value());
-        parent_it != std::end(m_index_style)) {
-      parent = generate_style_(parent_attr.value(), parent_it->second);
+
+  while (true) {
+    // an entry present but still null means the link is already on this chain
+    const auto [style_it, inserted] = m_styles.try_emplace(*current_name);
+    if (!inserted) {
+      parent = style_it->second.get();
+      break;
+    }
+    chain.emplace_back(&style_it->first, current_node);
+
+    const pugi::xml_attribute parent_attr =
+        current_node.attribute("style:parent-style-name");
+    if (!parent_attr) {
+      break;
+    }
+    const auto parent_it = m_index_style.find(parent_attr.value());
+    if (parent_it == std::end(m_index_style)) {
+      break;
     }
+    current_name = &parent_it->first;
+    current_node = parent_it->second;
   }
 
-  Style *family{nullptr};
-  if (const pugi::xml_attribute family_attr = node.attribute("style:family");
-      family_attr) {
-    family = generate_default_style_(family_attr.value(), {});
+  for (const auto &[chain_name, chain_node] : chain | std::views::reverse) {
+    Style *family{nullptr};
+    if (const pugi::xml_attribute family_attr =
+            chain_node.attribute("style:family");
+        family_attr) {
+      family = generate_default_style_(family_attr.value(), {});
+    }
+
+    std::unique_ptr<Style> &style = m_styles[*chain_name];
+    style =
+        std::make_unique<Style>(this, *chain_name, chain_node, parent, family);
+    parent = style.get();
   }
 
-  style = std::make_unique<Style>(this, name, node, parent, family);
-  return style.get();
+  return parent;
 }
 
 void StyleRegistry::generate_master_pages_(Document &document) {
diff --git a/src/odr/internal/ooxml/text/ooxml_text_style.cpp b/src/odr/internal/ooxml/text/ooxml_text_style.cpp
index 123c1d88e..f1eac2224 100644
--- a/src/odr/internal/ooxml/text/ooxml_text_style.cpp
+++ b/src/odr/internal/ooxml/text/ooxml_text_style.cpp
@@ -2,6 +2,10 @@
 
 #include <odr/internal/ooxml/ooxml_util.hpp>
 
+#include <ranges>
+#include <utility>
+#include <vector>
+
 namespace odr::internal::ooxml::text {
 
 namespace {
@@ -250,31 +254,48 @@ void StyleRegistry::generate_styles_(const pugi::xml_node styles_root) {
   }
 }
 
+/// Walks the `w:basedOn` chain onto a stack and builds it from the root down;
+/// recursing it costs a stack frame per link.
 Style *StyleRegistry::generate_style_(const std::string &name,
                                       const pugi::xml_node node) {
-  // an entry present but still null means we are inside its own resolution;
-  // returning it breaks a cyclic `w:basedOn` chain
-  if (const auto styles_it = m_styles.find(name);
-      styles_it != std::end(m_styles)) {
-    return styles_it->second.get();
-  }
-  std::unique_ptr<Style> &style = m_styles[name];
+  // the names are the map's own, which keep their addresses across a rehash
+  std::vector<std::pair<const std::string *, pugi::xml_node>> chain;
 
+  const std::string *current_name = &name;
+  pugi::xml_node current_node = node;
   Style *parent{nullptr};
 
-  if (const pugi::xml_attribute parent_attr =
-          node.child("w:basedOn").attribute("w:val");
-      parent_attr) {
+  while (true) {
+    // an entry present but still null means the link is already on this chain
+    const auto [styles_it, inserted] = m_styles.try_emplace(*current_name);
+    if (!inserted) {
+      parent = styles_it->second.get();
+      break;
+    }
+    chain.emplace_back(&styles_it->first, current_node);
+
+    const pugi::xml_attribute parent_attr =
+        current_node.child("w:basedOn").attribute("w:val");
+    if (!parent_attr) {
+      break;
+    }
     // `find`, not `operator[]`: an unknown parent id must not grow m_index
     // while generate_styles_ iterates it
-    if (const auto index_it = m_index.find(parent_attr.value());
-        index_it != std::end(m_index)) {
-      parent = generate_style_(index_it->first, index_it->second);
+    const auto index_it = m_index.find(parent_attr.value());
+    if (index_it == std::end(m_index)) {
+      break;
     }
+    current_name = &index_it->first;
+    current_node = index_it->second;
+  }
+
+  for (const auto &[chain_name, chain_node] : chain | std::views::reverse) {
+    std::unique_ptr<Style> &style = m_styles[*chain_name];
+    style = std::make_unique<Style>(*chain_name, chain_node, parent);
+    parent = style.get();
   }
 
-  style = std::make_unique<Style>(name, node, parent);
-  return style.get();
+  return parent;
 }
 
 } // namespace odr::internal::ooxml::text
diff --git a/src/odr/internal/util/xml_util.cpp b/src/odr/internal/util/xml_util.cpp
index 466be5308..9a4d7f05e 100644
--- a/src/odr/internal/util/xml_util.cpp
+++ b/src/odr/internal/util/xml_util.cpp
@@ -9,6 +9,9 @@
 #include <pugixml.hpp>
 
 #include <cstddef>
+#include <cstdlib>
+#include <memory>
+#include <new>
 #include <string_view>
 #include <tuple>
 
@@ -76,14 +79,34 @@ std::string xml::read_declared_encoding(std::istream &in) {
   return std::string(head.substr(at, value_end - at));
 }
 
+/// Reads the part once; pugixml's stream loader buffers it twice. The buffer is
+/// `malloc`ed because pugixml takes it over and frees it, parse or no parse.
 pugi::xml_document xml::parse(const abstract::ReadableFilesystem &filesystem,
                               const AbsPath &path) {
-  pugi::xml_document result;
-  auto file = filesystem.open(path);
+  const std::shared_ptr<abstract::File> file = filesystem.open(path);
   if (!file) {
     throw FileNotFound();
   }
-  if (const auto success = result.load(*file->stream()); !success) {
+
+  const std::size_t size = file->size();
+  if (size == 0) {
+    throw NoXmlFile();
+  }
+  auto *buffer = static_cast<char *>(std::malloc(size));
+  if (buffer == nullptr) {
+    throw std::bad_alloc();
+  }
+
+  const std::unique_ptr<std::istream> stream = file->stream();
+  stream->read(buffer, static_cast<std::streamsize>(size));
+  if (stream->gcount() != static_cast<std::streamsize>(size)) {
+    std::free(buffer);
+    throw NoXmlFile();
+  }
+
+  pugi::xml_document result;
+  if (const auto success = result.load_buffer_inplace_own(buffer, size);
+      !success) {
     throw NoXmlFile();
   }
   return result;
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index e7b1056c5..55456c9ab 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -63,6 +63,7 @@ add_executable(odr_test
         "src/internal/oldms/xls_test.cpp"
 
         "src/internal/ooxml/ooxml_crypto_test.cpp"
+        "src/internal/ooxml/ooxml_text_style_test.cpp"
 
         "src/internal/pdf/pdf_cid.cpp"
         "src/internal/pdf/pdf_cmap.cpp"
diff --git a/test/src/internal/ooxml/ooxml_text_style_test.cpp b/test/src/internal/ooxml/ooxml_text_style_test.cpp
new file mode 100644
index 000000000..7cdb826e7
--- /dev/null
+++ b/test/src/internal/ooxml/ooxml_text_style_test.cpp
@@ -0,0 +1,116 @@
+#include <odr/internal/ooxml/text/ooxml_text_style.hpp>
+
+#include <cstddef>
+#include <optional>
+#include <string>
+#include <thread>
+
+#include <gtest/gtest.h>
+
+#include <pugixml.hpp>
+
+using namespace odr;
+using namespace odr::internal::ooxml::text;
+
+namespace {
+
+/// `count` styles, `s0` based on `s1` and so on; only the last names a font
+/// size, so reading it off `s0` proves the whole chain resolved.
+std::string based_on_chain(const std::size_t count) {
+  std::string result = "<w:styles>";
+  for (std::size_t i = 0; i < count; ++i) {
+    result += R"(<w:style w:styleId="s)" + std::to_string(i) + R"(">)";
+    if (i + 1 < count) {
+      result += R"(<w:basedOn w:val="s)" + std::to_string(i + 1) + R"("/>)";
+    } else {
+      result += R"(<w:rPr><w:sz w:val="48"/></w:rPr>)";
+    }
+    result += "</w:style>";
+  }
+  result += "</w:styles>";
+  return result;
+}
+
+StyleRegistry registry_of(const std::string &xml,
+                          pugi::xml_document &document) {
+  EXPECT_TRUE(document.load_string(xml.c_str()));
+  return StyleRegistry(document.child("w:styles"));
+}
+
+} // namespace
+
+TEST(ooxml_text_style, based_on_chain_inherits) {
+  pugi::xml_document document;
+  const StyleRegistry registry = registry_of(based_on_chain(3), document);
+
+  const Style *style = registry.style("s0");
+  ASSERT_NE(nullptr, style);
+  ASSERT_NE(nullptr, style->parent());
+  EXPECT_EQ("s1", style->parent()->name());
+  EXPECT_EQ("s2", style->parent()->parent()->name());
+  EXPECT_EQ(nullptr, style->parent()->parent()->parent());
+
+  ASSERT_TRUE(style->resolved().text_style.font_size.has_value());
+  EXPECT_EQ(Measure(24, DynamicUnit("pt")),
+            *style->resolved().text_style.font_size);
+}
+
+/// On a thread, whose stack is the small one an http worker gets.
+TEST(ooxml_text_style, deep_based_on_chain_resolves) {
+  constexpr std::size_t count = 100000;
+
+  std::optional<Measure> font_size;
+  bool last_is_root = false;
+
+  std::thread worker([&font_size, &last_is_root] {
+    pugi::xml_document document;
+    const StyleRegistry registry = registry_of(based_on_chain(count), document);
+
+    if (const Style *style = registry.style("s0"); style != nullptr) {
+      font_size = style->resolved().text_style.font_size;
+    }
+    if (const Style *last = registry.style("s" + std::to_string(count - 1));
+        last != nullptr) {
+      last_is_root = last->parent() == nullptr;
+    }
+  });
+  worker.join();
+
+  ASSERT_TRUE(font_size.has_value());
+  EXPECT_EQ(Measure(24, DynamicUnit("pt")), *font_size);
+  EXPECT_TRUE(last_is_root);
+}
+
+/// A `w:basedOn` cycle resolves to styles that exist and end somewhere.
+TEST(ooxml_text_style, cyclic_based_on_chain_terminates) {
+  pugi::xml_document document;
+  const StyleRegistry registry =
+      registry_of(R"(<w:styles>)"
+                  R"(<w:style w:styleId="a"><w:basedOn w:val="b"/></w:style>)"
+                  R"(<w:style w:styleId="b"><w:basedOn w:val="c"/></w:style>)"
+                  R"(<w:style w:styleId="c"><w:basedOn w:val="a"/></w:style>)"
+                  R"(</w:styles>)",
+                  document);
+
+  for (const char *name : {"a", "b", "c"}) {
+    const Style *style = registry.style(name);
+    ASSERT_NE(nullptr, style) << name;
+    for (std::size_t depth = 0; style != nullptr; ++depth) {
+      ASSERT_LT(depth, 3u) << name;
+      style = style->parent();
+    }
+  }
+}
+
+/// An unknown `w:basedOn` target leaves the style parentless.
+TEST(ooxml_text_style, unknown_based_on_target) {
+  pugi::xml_document document;
+  const StyleRegistry registry = registry_of(
+      R"(<w:styles><w:style w:styleId="a"><w:basedOn w:val="gone"/></w:style></w:styles>)",
+      document);
+
+  const Style *style = registry.style("a");
+  ASSERT_NE(nullptr, style);
+  EXPECT_EQ(nullptr, style->parent());
+  EXPECT_EQ(nullptr, registry.style("gone"));
+}

From 66c6bf83786481a0b4c98d15749056d84344e7ae Mon Sep 17 00:00:00 2001
From: Andreas Stefl <stefl.andreas@gmail.com>
Date: Fri, 14 Aug 2026 21:45:12 +0200
Subject: [PATCH 2/2] fix(xml): take a file, and open its stream before its
 buffer

`parse` splits so the buffering lives on an `abstract::File` overload and the
filesystem one opens and delegates.

Opening a zip entry throws when it is encrypted or compressed by a method we
do not have, and the entry's size is its own claim until then - so the stream
is acquired first, and the buffer is held under `unique_ptr` until
`load_buffer_inplace_own` takes it over.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WACM9eXR7F5aZ22tyq2jwU
---
 src/odr/internal/util/xml_util.cpp | 35 ++++++++++++++++++------------
 src/odr/internal/util/xml_util.hpp |  6 ++++-
 2 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/src/odr/internal/util/xml_util.cpp b/src/odr/internal/util/xml_util.cpp
index 9a4d7f05e..a931e4ede 100644
--- a/src/odr/internal/util/xml_util.cpp
+++ b/src/odr/internal/util/xml_util.cpp
@@ -79,39 +79,46 @@ std::string xml::read_declared_encoding(std::istream &in) {
   return std::string(head.substr(at, value_end - at));
 }
 
-/// Reads the part once; pugixml's stream loader buffers it twice. The buffer is
+/// Reads @p file once; pugixml's stream loader buffers it twice. The buffer is
 /// `malloc`ed because pugixml takes it over and frees it, parse or no parse.
-pugi::xml_document xml::parse(const abstract::ReadableFilesystem &filesystem,
-                              const AbsPath &path) {
-  const std::shared_ptr<abstract::File> file = filesystem.open(path);
-  if (!file) {
-    throw FileNotFound();
-  }
-
-  const std::size_t size = file->size();
+pugi::xml_document xml::parse(const abstract::File &file) {
+  const std::size_t size = file.size();
   if (size == 0) {
     throw NoXmlFile();
   }
-  auto *buffer = static_cast<char *>(std::malloc(size));
+  // before the buffer: opening an entry that is encrypted or compressed by a
+  // method we do not have throws, and the size is the file's claim until then
+  const std::unique_ptr<std::istream> stream = file.stream();
+
+  std::unique_ptr<char, decltype(&std::free)> buffer(
+      static_cast<char *>(std::malloc(size)), &std::free);
   if (buffer == nullptr) {
     throw std::bad_alloc();
   }
 
-  const std::unique_ptr<std::istream> stream = file->stream();
-  stream->read(buffer, static_cast<std::streamsize>(size));
+  stream->read(buffer.get(), static_cast<std::streamsize>(size));
   if (stream->gcount() != static_cast<std::streamsize>(size)) {
-    std::free(buffer);
     throw NoXmlFile();
   }
 
   pugi::xml_document result;
-  if (const auto success = result.load_buffer_inplace_own(buffer, size);
+  if (const auto success =
+          result.load_buffer_inplace_own(buffer.release(), size);
       !success) {
     throw NoXmlFile();
   }
   return result;
 }
 
+pugi::xml_document xml::parse(const abstract::ReadableFilesystem &filesystem,
+                              const AbsPath &path) {
+  const std::shared_ptr<abstract::File> file = filesystem.open(path);
+  if (!file) {
+    throw FileNotFound();
+  }
+  return parse(*file);
+}
+
 xml::StringToken::StringToken(const Type type, std::string string)
     : type{type}, string{std::move(string)} {}
 
diff --git a/src/odr/internal/util/xml_util.hpp b/src/odr/internal/util/xml_util.hpp
index 5b6624b33..7554e5fd8 100644
--- a/src/odr/internal/util/xml_util.hpp
+++ b/src/odr/internal/util/xml_util.hpp
@@ -9,8 +9,9 @@ class xml_document;
 } // namespace pugi
 
 namespace odr::internal::abstract {
+class File;
 class ReadableFilesystem;
-}
+} // namespace odr::internal::abstract
 
 namespace odr::internal {
 class AbsPath;
@@ -19,7 +20,10 @@ class AbsPath;
 namespace odr::internal::util::xml {
 
 pugi::xml_document parse(const std::string &);
+/// Buffers @p in twice on the way in; prefer the @ref abstract::File overload,
+/// which reads once against the size the file knows.
 pugi::xml_document parse(std::istream &);
+pugi::xml_document parse(const abstract::File &);
 pugi::xml_document parse(const abstract::ReadableFilesystem &, const AbsPath &);
 
 /// Throws unless @p in holds a well formed xml document.