Repository navigation
ROB: Guard ArrayObject.read_from_stream against uncaught RecursionError - #4430
Open
damasiyachiranj wants to merge 2 commits into
Open
damasiyachiranj wants to merge 2 commits into
damasiyachiranj wants to merge 2 commits into
Conversation
A deeply nested array (e.g. "[[[[...]]]]") drives recursion depth in read_from_stream's recursive descent, not element count, so array_maximum_element_count never engages and an uncaught RecursionError escapes to the caller instead of a PdfReadError. Mirrors the existing guard in DictionaryObject.read_from_stream: catches (MemoryError, RecursionError) around the recursive read_object() call and re-raises as PdfReadError, while still letting LimitReachedError (the element-count/token budget) propagate unchanged. Fixes py-pdf#4427.
Adds test_array_object__read_from_stream__deeply_nested_array_raises_pdfreaderror, verifying a 5000-deep nested array now raises PdfReadError instead of an uncaught RecursionError.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4430 +/- ##
=======================================
Coverage 98.87% 98.87%
=======================================
Files 59 59
Lines 12299 12305 +6
Branches 2342 2342
=======================================
+ Hits 12161 12167 +6
Misses 68 68
Partials 70 70 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
damasiyachiranj
changed the title
stefan6419846
requested changes
Nov 14, 2026
| # Always propagate security issues. | ||
| raise | ||
| except (MemoryError, RecursionError) as exc: | ||
| # A deeply nested array (e.g. "[[[[...]]]]") blows the interpreter's |
Collaborator
There was a problem hiding this comment.
Why do we need this large comment? The referenced case does not have one and nesting triggering recursion errors in such a scenario should be obvious and - if required - derivable from the corresponding PR once merged.
|
|
||
|
|
||
| def test_array_object__read_from_stream__deeply_nested_array_raises_pdfreaderror() -> None: | ||
| # A single array nested thousands of levels deep drives recursion depth, |
Collaborator
There was a problem hiding this comment.
I do not see how this comment provides benefits, especially as inline comment instead of docstring.
| # not element count, so array_maximum_element_count never engages. | ||
| # Previously this raised an uncaught RecursionError instead of PdfReadError. | ||
| pdf = ReaderDummy() | ||
| depth = 5000 |
Collaborator
There was a problem hiding this comment.
Why do we need a depth of 5000 to trigger an issue which occurs with exceeding sys.getrecursionlimit(), which usually is just 1000?
This branch has not been deployed
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.
Fixes #4427.
ArrayObject.read_from_streamrecurses intoread_object()for each element. A single array nested thousands of levels deep (e.g.[[[[...]]]]) drives recursion depth rather than element count, soarray_maximum_element_countnever engages, and Python's interpreter recursion limit is hit first — raising an uncaughtRecursionErrorinstead of a cleanPdfReadError.DictionaryObject.read_from_streamalready guards against this (catchesMemoryError/RecursionErroraround its recursiveread_object()call and re-raises asPdfReadError, while still lettingLimitReachedErrorpropagate). This PR applies the same guard toArrayObject.read_from_stream.Added
test_array_object__read_from_stream__deeply_nested_array_raises_pdfreaderror, which builds a 5000-deep nested array and asserts it now raisesPdfReadErrorinstead of an uncaughtRecursionError.