-
Notifications
You must be signed in to change notification settings - Fork 1.6k
ROB: guard empty link destination arrays during page transfer #3896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3036,6 +3036,8 @@ def _insert_filtered_annotations( | |
| outlist.append(ano.clone(self).indirect_reference) | ||
| else: | ||
| d = cast("ArrayObject", d) | ||
| if not isinstance(d, list) or not d: | ||
| continue | ||
| p = self._get_cloned_page(d[0], pages, reader) | ||
| if p is not None: | ||
| anc = ano.clone(self, ignore_fields=("/Dest",)) | ||
|
|
@@ -3051,6 +3053,8 @@ def _insert_filtered_annotations( | |
| outlist.append(ano.clone(self).indirect_reference) | ||
| else: | ||
| d = cast("ArrayObject", d) | ||
| if not isinstance(d, list) or not d: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same change here, checks for |
||
| continue | ||
| p = self._get_cloned_page(d[0], pages, reader) | ||
| if p is not None: | ||
| anc = ano.clone(self, ignore_fields=("/D",)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -899,6 +899,56 @@ def test_link_annotation(pdf_file_path): | |
| writer.write(output_stream) | ||
|
|
||
|
|
||
| def _reader_with_link_annotation(annotation: DictionaryObject) -> PdfReader: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need an external helper if it is only used in one other function?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair, it was only used once. Inlined it into the test. |
||
| """A one-page PDF whose single page carries the given link annotation.""" | ||
| writer = PdfWriter() | ||
| writer.add_blank_page(width=72, height=72) | ||
| writer.pages[0][NameObject("/Annots")] = ArrayObject([writer._add_object(annotation)]) | ||
| stream = BytesIO() | ||
| writer.write(stream) | ||
| stream.seek(0) | ||
| return PdfReader(stream) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "annotation", | ||
| [ | ||
| # Empty /Dest array. | ||
| DictionaryObject({ | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please provide meaningful IDs.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added ids: empty-dest-array, empty-goto-action-dest, non-array-dest. |
||
| NameObject("/Subtype"): NameObject("/Link"), | ||
| NameObject("/Dest"): ArrayObject([]), | ||
| }), | ||
| # Empty /GoTo action destination. | ||
| DictionaryObject({ | ||
| NameObject("/Subtype"): NameObject("/Link"), | ||
| NameObject("/A"): DictionaryObject({ | ||
| NameObject("/S"): NameObject("/GoTo"), | ||
| NameObject("/D"): ArrayObject([]), | ||
| }), | ||
| }), | ||
| # /Dest that is not an array at all. | ||
| DictionaryObject({ | ||
| NameObject("/Subtype"): NameObject("/Link"), | ||
| NameObject("/Dest"): NumberObject(5), | ||
| }), | ||
| ], | ||
| ) | ||
| def test_malformed_link_destination_does_not_crash_transfer(annotation): | ||
| """A link with an empty or non-array destination must not abort page transfer.""" | ||
| reader = _reader_with_link_annotation(annotation) | ||
|
|
||
| # append() routes the annotation through _insert_filtered_annotations. | ||
| appended = PdfWriter() | ||
| appended.append(reader) | ||
| appended.write(BytesIO()) | ||
|
|
||
| # add_page() + write() routes it through extract_links()/_resolve_links(). | ||
| added = PdfWriter() | ||
| for page in reader.pages: | ||
| added.add_page(page) | ||
| added.write(BytesIO()) | ||
|
|
||
|
|
||
| def test_io_streams(): | ||
| """This is the example from the docs ("Streaming data").""" | ||
| filepath = RESOURCE_ROOT / "pdflatex-outline.pdf" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we check for an
ArrayObjecthere, could we avoid the explicit casting?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Switched both branches to check
isinstance(d, ArrayObject)directly, so the explicit cast is gone and mypy still narrows fine.