fix: walk a style's parent chain instead of recursing it, and read an xml part once - #691
Merged
Merged
Conversation
… 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
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ef62376ff
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
`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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 Generated with Claude Code
Two native crash clusters from Play (14 Aug 2026), both document-driven.
Stack overflow in
StyleRegistry::generate_style_A
w:basedOn/style:parent-style-namechain 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 is unchanged in meaning: an entry present but still null.ooxml_text_style_test.cppresolves a 100 000-link chain on a thread, so it runs on the small stack rather than the process's. Against the old recursive code it kills the test binary; against this it passes in ~85 ms. Also covers inheritance, a cycle, and an unknown parent.Double buffering in
xml::parsepugixml's stream loader holds every part twice — the chunk list it reads into, then the contiguous buffer it parses in place. Zip entries know their uncompressed size, so the part is read once into a buffer pugixml is handed and frees. This lowers peak memory for the reported pugixml OOM aborts; it does not eliminate them, since Scudo aborts inside
mallocrather than returning null.Verification
The third cluster in the same triage (
DynamicUnit::to_string()SIGSEGV) needs no change here: it is an unguardedframe.width()->in v6.5.0, already fixed on main by 734b6cc and not yet released.