fix: bound object-graph expansion and detect reference cycles - #52
Merged
Merged
Conversation
Binary plists address objects by index, so one stored object can be
referenced many times. maxObjectCount bounds how many objects are
*stored*, not how many are *expanded*, so a tiny file can describe an
enormous tree: if each level references the level below twice, n objects
expand to 2^n nodes.
A 155-byte file built this way (29 objects, 2^28 expansions) exhausts the
heap and terminates the process with
FATAL ERROR: Ineffective mark-compacts near heap limit -
JavaScript heap out of memory
which a caller cannot catch, since V8 aborts rather than throwing. A
43-byte file whose object references itself recurses until the stack
overflows.
Add an expansion budget (maxExpandedObjectCount, default 1,000,000, with
a setter alongside the existing knobs) and a per-path cycle check, so both
cases raise an ordinary catchable Error instead of killing the process.
The change is additive: no valid document parses differently. All 8 real
plist fixtures in the test suite parse identically and the existing tests
pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
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.
Summary
Binary plists address objects by index, so one stored object can be referenced
many times.
maxObjectCountbounds how many objects are stored:…but nothing bounds how many are expanded.
parseArray()/parseDictionary()call
parseObject(objRef)for each reference with no visited set and no budget, soa tiny file can describe an enormous tree: if each level references the level below
twice, n objects expand to 2ⁿ nodes.
A 155-byte file (29 objects, 2²⁸ expansions) kills the process:
That is not catchable — V8 aborts rather than throwing, so wrapping
parseFileSync()intry/catchdoes not save the caller. Measured on thismachine before the change:
Separately, a 43-byte file whose object references itself recurses until
RangeError: Maximum call stack size exceeded.Because the expansion is what blows up,
maxObjectCountnever fires — the PoC usesonly 29 objects, far under the 32768 limit.
Fix
Two additive guards inside
parseObject:maxExpandedObjectCount(default 1,000,000, with asetMaxExpandedObjectCountsetter alongside the existing knobs) — caps total expansions, so the amplification
case raises an ordinary
Error.reports
Circular reference detected at object #ninstead of overflowing the stack.Both raise catchable errors, so callers can handle a malicious file the same way they
already handle a malformed one.
Verification
test/parse identically — the change is additiveand no valid document is affected.
maxExpandedObjectCount exceeded(was: fatal OOM).
Circular reference detected at object #0(was: stack overflow).Notes
I picked the budget rather than memoising resolved objects deliberately: memoisation
would also collapse the DAG, but it makes repeated references return the same
instance, which is an observable change for anyone mutating the parsed result. The
budget changes nothing for valid input. Happy to switch to memoisation (or add it as
well, as a performance win) if you'd prefer.
The default of 1,000,000 is a guess at "comfortably above any real file" — say the
word if you'd like it higher, lower, or derived from
maxObjectCount.Found and fixed with AI assistance (Claude). Happy to add a regression test with the
two PoC files if useful.