Skip to content

ROB: guard empty link destination arrays during page transfer#3896

Open
metsw24-max wants to merge 2 commits into
py-pdf:mainfrom
metsw24-max:link-dest-bounds
Open

ROB: guard empty link destination arrays during page transfer#3896
metsw24-max wants to merge 2 commits into
py-pdf:mainfrom
metsw24-max:link-dest-bounds

Conversation

@metsw24-max

Copy link
Copy Markdown
Contributor

Empty link destination arrays crash page transfer

A /Link annotation whose /Dest (or /GoTo action /D) is an empty array makes page transfer read the target page from d[0] with no length check, so append()/merge() raise IndexError in _insert_filtered_annotations, and add_page() followed by write() raises it from DirectReferenceLink.find_referenced_page via _resolve_links; a non-array /Dest trips TypeError likewise. Both sites now skip a destination that is not a non-empty array, matching the existing drop-when-unresolvable handling and the None path already present in _resolve_links.

Repro:

from io import BytesIO
from pypdf import PdfReader, PdfWriter
from pypdf.generic import ArrayObject, DictionaryObject, NameObject

src = PdfWriter()
src.add_blank_page(width=72, height=72)
link = DictionaryObject({
    NameObject("/Subtype"): NameObject("/Link"),
    NameObject("/Dest"): ArrayObject([]),  # empty destination
})
src.pages[0][NameObject("/Annots")] = ArrayObject([src._add_object(link)])
buf = BytesIO(); src.write(buf); buf.seek(0)

writer = PdfWriter()
writer.append(PdfReader(buf))   # IndexError before this change
writer.write(BytesIO())

@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.86%. Comparing base (5286c38) to head (5ffea49).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3896      +/-   ##
==========================================
+ Coverage   97.83%   97.86%   +0.03%     
==========================================
  Files          55       57       +2     
  Lines       10609    10742     +133     
  Branches     1985     2009      +24     
==========================================
+ Hits        10379    10513     +134     
  Misses        127      127              
+ Partials      103      102       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread pypdf/_writer.py Outdated
outlist.append(ano.clone(self).indirect_reference)
else:
d = cast("ArrayObject", d)
if not isinstance(d, list) or not d:

Copy link
Copy Markdown
Collaborator

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 ArrayObject here, could we avoid the explicit casting?

Copy link
Copy Markdown
Contributor Author

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.

Comment thread pypdf/_writer.py Outdated
outlist.append(ano.clone(self).indirect_reference)
else:
d = cast("ArrayObject", d)
if not isinstance(d, list) or not d:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same change here, checks for ArrayObject directly now.

Comment thread tests/test_writer.py Outdated
writer.write(output_stream)


def _reader_with_link_annotation(annotation: DictionaryObject) -> PdfReader:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, it was only used once. Inlined it into the test.

Comment thread tests/test_writer.py
"annotation",
[
# Empty /Dest array.
DictionaryObject({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide meaningful IDs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added ids: empty-dest-array, empty-goto-action-dest, non-array-dest.

Signed-off-by: Sayed Kaif <metsw24@gmail.com>
@metsw24-max

Copy link
Copy Markdown
Contributor Author

Pushed the review changes: both branches now check isinstance(d, ArrayObject) directly (cast dropped), the test helper is inlined, and the parametrised cases have meaningful ids.

@metsw24-max

Copy link
Copy Markdown
Contributor Author

any update?

@stefan6419846

Copy link
Copy Markdown
Collaborator

As already mentioned in your three other PRs, I recommend you to have a look at #3900. Until then, pinging for updates might have the effect of moving it to the end of my queue and even further delaying it.

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