Account for broken outlines/annotations, where the destination dictionary contains an invalid /Dest entry
#8825
Conversation
|
/botio unittest |
|
/botio unittest |
|
/botio preview |
src/core/obj.js
Outdated
| dest = destDict.get('Dest'); | ||
| } else if (destDict.has('Dest')) { // Simple destination. | ||
| let simpleDest = destDict.get('Dest'); | ||
| if (isDict(simpleDest)) { |
There was a problem hiding this comment.
How about directly assigning to dest instead of a temporary variable? In case simpleDest is invalid, you are already returning early, so not using a temporary variable should be fine.
There was a problem hiding this comment.
I updated the patch to do the proper checks above, so that we'll no longer reach this branch in the isDict(destDict.get('Dest')) === true case. Hence these changes were simply reverted.
| @@ -689,6 +689,34 @@ describe('annotation', function() { | |||
| expect(data.dest).toEqual([{ num: 17, gen: 0, }, { name: 'XYZ', }, | |||
| 0, 841.89, null]); | |||
| }); | |||
|
|
|||
| it('should correctly parse a Dest, which violates the specification ' + | |||
| 'by containing a dictionary', function() { | |||
There was a problem hiding this comment.
Can you add a comment to the line where the invalid dictionary is constructed, for those who don't know the PDF spec by heart?
There was a problem hiding this comment.
Sorry, I don't completely follow what sort of comment you're asking for here. Are you simply asking for a reference to the particular section of the PDF spec, or something else?
[...] where the invalid dictionary is constructed,
I might be reading too much into this, but just to clarify:
Please note that there's no way for a Dest entry to contain a "valid" dictionary, the mere fact that one is even present makes it invalid according to the spec.
There was a problem hiding this comment.
I was thinking of a comment before line 703, something like this (edit as needed):
// Dest must be a Name or Array (PDF27000 section x.y.z), but there are PDFs that store a dictionary.
|
Looks good to me, can be merged after feedback on the above nits. |
|
@Rob--W Thanks for the review! diff --git a/src/core/obj.js b/src/core/obj.js
index 2e1228de..95924a6f 100644
--- a/src/core/obj.js
+++ b/src/core/obj.js
@@ -651,7 +651,7 @@ var Catalog = (function CatalogClosure() {
var docBaseUrl = params.docBaseUrl || null;
var action = destDict.get('A'), url, dest;
- if (!action && destDict.has('Dest')) {
+ if (!isDict(action) && destDict.has('Dest')) {
// A /Dest entry should *only* contain a Name or an Array, but some bad
// PDF generators ignore that and treat it as an /A entry.
action = destDict.get('Dest');
@@ -764,12 +764,7 @@ var Catalog = (function CatalogClosure() {
break;
}
} else if (destDict.has('Dest')) { // Simple destination.
- let simpleDest = destDict.get('Dest');
- if (isDict(simpleDest)) {
- warn('parseDestDictionary: The Dest entry should not be a dictionary.');
- return;
- }
- dest = simpleDest;
+ dest = destDict.get('Dest');
}
if (isString(url)) {
diff --git a/test/unit/annotation_spec.js b/test/unit/annotation_spec.js
index 024355a8..faf3293d 100644
--- a/test/unit/annotation_spec.js
+++ b/test/unit/annotation_spec.js
@@ -700,6 +700,8 @@ describe('annotation', function() {
let annotationDict = new Dict();
annotationDict.set('Type', Name.get('Annot'));
annotationDict.set('Subtype', Name.get('Link'));
+ // The /Dest must be a Name or an Array, refer to ISO 32000-1:2008
+ // section 12.3.3, but there are PDF files where it's a dictionary.
annotationDict.set('Dest', destDict);
let annotationRef = new Ref(798, 0); |
…nary contains an invalid `/Dest` entry According to the specification, see http://www.adobe.com/content/dam/Adobe/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#page=377, a `Dest` entry in an outline item should *not* contain a dictionary. Unsurprisingly there's PDF generators that completely ignore this, treating is an `A` entry instead. The patch also adds a little bit more validation code in `Catalog.parseDestDictionary`.
|
/botio unittest |
From: Bot.io (Linux m4)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/7da0bb68d4083eb/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/0b76ee4abc14969/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/7da0bb68d4083eb/output.txt Total script time: 2.60 mins
|
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/17a533e255dfbe2/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/17a533e255dfbe2/output.txt Total script time: 2.36 mins Published |
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/0b76ee4abc14969/output.txt Total script time: 12.62 mins
|
|
Thanks for the patch and review response! |
Account for broken outlines/annotations, where the destination dictionary contains an invalid `/Dest` entry
According to the specification, see http://www.adobe.com/content/dam/Adobe/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#page=377, a
Destentry in an outline item should not contain a dictionary.Unsurprisingly there's PDF generators that completely ignore this, treating is an
Aentry instead.The patch also adds a little bit more validation code in
Catalog.parseDestDictionary.Fixes #5089 (comment).