Bug #131147 for PDF-API2: Bug with missing properties including patch

On Mon Dec 09 09:41:13 2019, vadimr wrote: Show quoted text
Was that RT 121911 by any chance? That bug has been fixed in both API2 and Builder, so I'm wondering (if and) how this one is related. Show quoted text
Vadim, I applied your patch (File.pm) and it appears to fix the problem (both Perl one-liners you gave above). It also doesn't break any t-tests or examples in PDF::Builder. Note that I did NOT add Klaus's patch to Pages.pm -- is his patch made unnecessary by yours? As he has not provided any test code, I can't be absolutely sure that your patch alone fixes the problem. Per my earlier question about RT 130722, this patch does not seem to affect it. If Klaus gives his blessing to your patch (alone), I will release it in PDF::Builder. It's up to Steve as to what he wants to do with PDF::API2.

On Mon Dec 16 12:18:18 2019, Klaus@Ethgen.ch wrote: Show quoted text
You provided a PDF (map of Switzerland) that loads correctly. That's the one that Vadim used in his two one-line examples. You provided a patch, but I didn't see any example code showing the problem you're reporting. That's what I was asking to see. Let me play with the code you just listed, with and without Vadim's patch, and perhaps with and without your patch, and see what happens.

Show quoted text
Phil, RT 130722 was about malformed PDF with broken pages tree, and I remain of the same opinion: no correction was required on PDF::API2 side; that RT can not (and should not) be compared to others concerning valid PDF files; error message and/or source line number being the same (or close) is co-incidental and of no importance. Klaus's patch would fix the issue with "importPageIntoForm" with attached PDF, but my 2nd one-liner was to show that, following the route of applying a fix close to exposed line number, the 2nd patch would be required to fix failing "page" call, with line number somewhere close to RT 121911. That's the only reason I mentioned it. No other connection. Instead, my idea was to find more universal solution. The suggested patch should practically work OK, but I think some considerations are to be documented at least. The "null" object itself is not a scarecrow, it's very common in, e.g., explicit destination arrays. But, as value of dictionary entry, the only place in the Reference I found is quoted above. Does it mean such entries can be safely discarded? Can any entry, which is strongly "required", somehow finish having value "null"? I don't think so, but, what if, per quote above, we discard this entry, and "required" entry disappears from updated file? PDF ideology, which I agree with and which is NOT strictly adhered to by, e.g. PDF::API2, is don't touch entries you don't know what to do about. They can be from some future PDF specification :). I don't like the idea of throwing things away. But that's pure speculation. Ideally, PDF::API2::Basic::PDF::Dict should access dict entries with e.g. "key_exists", which checks if value of this key resolves to "null" and would return "false", and a getter "get_value", which similarly returns same value (e.g. undef) for non-existing keys or if value is "null". In practice, it will never happen. Given the way PDF::API2 was designed to traverse pages tree, I think the cheapest (and rather safe, regardless of what said above) way to fix the present RT would be with my patch.

Show quoted text
Correct, mine can possibly address other issues (unknown yet), and it's either of the two, but if you decide to choose more cautious way (Klaus's patch) rather than more radical, then _also_ search where to (cautiously) patch so that "page" call (2nd one-liner) won't fail. As to documentation, maybe a link to this RT in a comment in source is more than enough. Show quoted text
They are related in a sense that "PDF::API2::Basic::PDF::Pages-> find_prop" method is called on object of unsuitable class -- PDF::API2::Basic::PDF::Objind (130722) or PDF::API2::Basic::PDF::Null (131147). But objects became blessed into these classes for unrelated reasons. When PDF is broken (130722) we can't exclude possibility of similar mismatch anywhere. In fact, it's a good thing that PDF::API2 fails early and noisily. Acrobat doesn't report any issue, but when I try to import a new page in UI (it was some time ago, IIRC insert one before the last... doesn't matter) then there were bizarre results in both visual feedback ("Pages" docker) and cryptic error message. Though at first file was opened as valid! Returning to 131147, I think with my patch, PDF::API2::Basic::PDF::Null is prevented from appearing in pages tree when bubbling up from a leaf to root. As to my previous concerns (is suggested patch too radical, can it break anything)... After some thought, I can imagine only some weird steganography would be disrupted, if any is ever used in PDF :) Otherwise, should be safe.

Agreed with Vadim that dictionary entries with null values should be ignored, per the spec. I've updated the code accordingly. As Vadim noted, neither his patch nor my version will handle a dictionary with an indirect object pointing to a null value, but I'm okay with waiting for that particular case to come up, hopefully with an associated patch. Thanks for the bug report and the patch.