Skip to content

fix: walk the group chain iteratively so deep nesting can't overflow the stack - #42

Open
BrianWillows wants to merge 1 commit into
iarna:masterfrom
BrianWillows:fix/iterative-group-chain
Open

fix: walk the group chain iteratively so deep nesting can't overflow the stack#42
BrianWillows wants to merge 1 commit into
iarna:masterfrom
BrianWillows:fix/iterative-group-chain

Conversation

@BrianWillows

Copy link
Copy Markdown

Summary

RTF groups nest via {, and get / getFont / getColor / getStyle each recurse
up the parent chain on every lookup:

getStyle (name) {
  if (!name) return Object.assign({}, this.parent.getStyle(), this.style)
  return this.style[name] != null ? this.style[name] : this.parent.getStyle(name)
}

A document that nests deeply enough overflows the stack:

RangeError: Maximum call stack size exceeded
    at RTFGroup.getStyle (rtf-group.js:21:12)
    at RTFGroup.getStyle (rtf-group.js:22:53)
    at RTFGroup.getStyle (rtf-group.js:22:53)  ...

The important part: that error is raised inside a stream callback, not passed to
the rtf.string(str, cb) error argument — so it can't be caught through the
documented API and takes the process down. A ~39 KB file (20,000 nested groups) is
enough. For anything parsing untrusted .rtf — mail/attachment processing, document
converters, indexing pipelines — that's a remote crash.

Fix

Walk the chain with a loop instead of recursing. The walk still terminates at
RTFDocument, which overrides each of these to return directly, so resolution order
is unchanged.

getStyle() with no name now also collects the chain and merges it root-first in a
single pass, instead of allocating a fresh object at every level of every lookup.

Results

The crash is gone — 20,000 nested groups now parses normally instead of killing the
process — and deep documents get meaningfully faster:

nested groups before after
1,000 123 ms 34 ms 3.6×
2,000 472 ms 84 ms 5.6×
4,000 1,666 ms 290 ms 5.7×
8,000 6,590 ms 1,164 ms 5.7×

Being straight about what this doesn't fix: the cost is still quadratic in nesting
depth, just with a much smaller constant. Each addContent resolves the effective
style by walking the whole chain, so N nested groups still do O(N) work N times.
Making that O(1) means caching a group's effective style, which needs invalidating
wherever the interpreter does group.style.x = ... — that's a broader change and I
didn't want to guess at it inside someone else's parser. Happy to follow up with it
if you'd like, or to add a nesting-depth limit instead.

Realistic documents are unaffected either way: 2,000 × 3-deep formatting groups
(29 KB) parses in ~52 ms. This only bites on deliberately-nested input.

Verification

The repo has no test suite (test/ isn't present, so npm test can't run), so I
checked correctness with a differential harness against the current implementation:
34 documents serialise byte-for-byte identically, covering font tables, colour
tables, bold/italic/underline, alignment, super/subscript, \plain, escaped (\'e9)
and unicode (\u233?) characters, paragraph handling, repeated sibling groups, and
nesting from 1 to 250 deep.

Found and fixed with AI assistance (Claude). Happy to add the harness as a test file
if that would be useful — it would give the repo a starting suite.

…the stack

RTF groups nest via `{`, and get/getFont/getColor/getStyle each recursed
up the parent chain on every lookup. A document that nests deeply enough
overflows the stack:

    RangeError: Maximum call stack size exceeded
        at RTFGroup.getStyle (rtf-group.js:21:12)
        at RTFGroup.getStyle (rtf-group.js:22:53)  ...

That error is raised inside a stream callback rather than passed to the
rtf.string(str, cb) error argument, so it is not catchable through the
documented API and takes the process down. A ~39KB file was enough.

Walk the chain with a loop instead. The walk still terminates at
RTFDocument, which overrides each of these to return directly, so
resolution order is unchanged.

getStyle() with no name also collected the chain and merged it root-first
in one pass, rather than allocating a fresh object at every level of every
lookup - about 5.7x faster on deeply nested input.

The repo has no test suite, so correctness was checked with a differential
harness: 34 documents (fonts, colour tables, bold/italic/underline,
alignment, super/subscript, \plain, escaped and unicode characters,
paragraphs, sibling groups, and nesting from 1 to 250 deep) serialise
byte-for-byte identically before and after.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants