fix: walk a style's parent chain instead of recursing it, and read an xml part once

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
 

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) {

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

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,17 +79,44 @@ std::string xml::read_declared_encoding(std::istream &in) {
   return std::string(head.substr(at, value_end - at));
 }
 
+/// 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::File &file) {
+  const std::size_t size = file.size();
+  if (size == 0) {
+    throw NoXmlFile();
+  }
+  // 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();
+  }
+
+  stream->read(buffer.get(), static_cast<std::streamsize>(size));
+  if (stream->gcount() != static_cast<std::streamsize>(size)) {
+    throw NoXmlFile();
+  }
+
+  pugi::xml_document result;
+  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) {
-  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) {
-    throw NoXmlFile();
-  }
-  return result;
+  return parse(*file);
 }
 
 xml::StringToken::StringToken(const Type type, std::string string)

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.

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"

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"));
+}