Conversation
- Add a configurable maximum nesting depth for arrays and dictionaries (Config::setMaxNestingDepth, default 5000). A container nested beyond the limit is skipped, so parsing returns instead of recursing without bound. - Detect cycles in the /Pages tree so Pages::getPages() no longer recurses indefinitely on a self-referential tree. - Apply the existing decodeMemoryLimit to LZWDecode and RunLengthDecode (previously honoured only by FlateDecode). - Add regression and characterization tests, document the new option and setDecodeMemoryLimit (README, doc/CustomConfig.md), and add ISO 32000-1 section references. - Refine documentation Co-Authored-By: Claude Opus 4.8 <[email protected]>
j0k3r
left a comment
j0k3r
left a comment
There was a problem hiding this comment.
Looks good to me but there is only that ancestorRefs which might become huge for an untrusted PDF and break PHP memory.
It seems that using a SplObjectStorage is better to track the active path without copying it per level
|
Thank you very much! You were right, the per-level array copy made memory quadratic in the page-tree depth (a chain of 4000 /Pages nodes needed over 400 MB, 10000 nodes exhausted 1 GB). Switched to a single SplObjectStorage shared by all levels, released in a finally block as you suggested. One deviation though: I used offsetSet/offsetExists/offsetUnset instead of attach/contains/detach, because those three are deprecated as of PHP 8.5. Also added a regression test with a 10000-level page tree. |
2 participants
Type of pull request
About
Keeps the parser within bounded memory and time on malformed or degenerate input, without changing behaviour for well-formed PDFs.
Details
getRawObject()recurses once per nested array or dictionary level. A new configurable limit (Config::setMaxNestingDepth(), default5000) bounds this; a container nested beyond the limit is skipped so parsing returns instead of recursing without bound. Regular PDFs nest far below this, so their output is unchanged.Pages::getPages()now tracks visited nodes, so a/Pagestree that references one of its ancestors returns the pages found so far instead of recursing indefinitely.Config::setDecodeMemoryLimit()previously bounded onlyFlateDecode; it now also boundsLZWDecodeandRunLengthDecode. The default (0= unlimited) is unchanged.Additional notes
setMaxNestingDepth()/getMaxNestingDepth().RobustnessTestplus characterization tests in existing test classes that pin the behavior for well-formed input.README.mdanddoc/CustomConfig.mddescribe the new option andsetDecodeMemoryLimit; added ISO 32000-1 section references in the touched code.@j0k3r: I hope it isn't too much to review. There are a lot of comments too and I added a couple more tests to make sure nothing breaks in production. If its too much code, please let me know, so I cut it up in smaller pieces. I combined them, because they all target issues which belong to the same error class ("malformed input leads parser to eat all memory").
CC @blackrose-0xday