Skip to content
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

Ensure that saveDocument works if there's no /ID-entry in the PDF document (issue 13279) #13280

Merged
merged 1 commit into from Apr 22, 2021

Conversation

Snuffleupagus
Copy link
Collaborator

First of all, while it should be very unlikely that the /ID-entry is an indirect object, note how we're using Dict.get when parsing it e.g. in PDFDocument.fingerprint. Hence we definitely should be consistent here, since if the /ID-entry is an indirect object the existing code in src/core/writer.js would already fail.
Secondly, to fix the referenced issue, we also need to check that the /ID-entry actually is an Array before attempting to access its contents in src/core/writer.js.

Drive-by change: In the xrefInfo object passed to the incrementalUpdate function, re-name the encrypt property to encryptRef since its data is fetched using Dict.getRaw (given the names of the other properties fetched similarly).

Fixes #13279

@Snuffleupagus
Copy link
Collaborator Author

/botio unittest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_unittest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/465cc8eedaa7168/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_unittest from @Snuffleupagus received. Current queue size: 0

Live output at: http://3.101.106.178:8877/f2f3197a777e4e4/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/465cc8eedaa7168/output.txt

Total script time: 3.62 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://3.101.106.178:8877/f2f3197a777e4e4/output.txt

Total script time: 5.65 mins

  • Unit Tests: Passed

@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/593b794c2d6eccf/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://3.101.106.178:8877/5cfeb2e261cfbf3/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/593b794c2d6eccf/output.txt

Total script time: 3.69 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://3.101.106.178:8877/5cfeb2e261cfbf3/output.txt

Total script time: 5.66 mins

  • Integration Tests: Passed

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Apr 21, 2021

While PDF documents without /ID is quite rare, as the issue shows they unfortunately do exist.
Given that the patch is small, and in my opinion quite safe, this might be the sort of thing that we should consider uplifting (once it's landed of course)?

…ocument (issue 13279)

First of all, while it should be very unlikely that the /ID-entry is an *indirect* object, note how we're using `Dict.get` when parsing it e.g. in `PDFDocument.fingerprint`. Hence we definitely should be consistent here, since if the /ID-entry is an *indirect* object the existing code in `src/core/writer.js` would already fail.
Secondly, to fix the referenced issue, we also need to check that the /ID-entry actually is an Array before attempting to access its contents in `src/core/writer.js`.

*Drive-by change:* In the `xrefInfo` object passed to the `incrementalUpdate` function, re-name the `encrypt` property to `encryptRef` since its data is fetched using `Dict.getRaw` (given the names of the other properties fetched similarly).
Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for doing this.

@calixteman calixteman merged commit 57a1ea8 into mozilla:master Apr 22, 2021
@Snuffleupagus Snuffleupagus deleted the saveDocument-missing-ID branch April 22, 2021 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash on form saving if at least one form field is filled
4 participants