Fixes null pointer exception in PNG extension module (issue #148)

[karenhanson]

This relates to issue #148. Some background:

• The issue was coming from this line in this class: com.macgath.jhove.module.png.ItxtChunk.
• The example image supplied in issue PNG throws java.lang.NullPointerException #148 had the following chunk that was resulting in the NPE:
  
• I used this PNG documentation to check the logic.

The NPE was happening for several reasons. First, this piece of code that adds keywords to the properties list was inside the loop that was processing the chunk one byte at a time, rather than outside of the loop where all bytes in the chunks had been processed. This meant most of the values were still null on the first loop when it was run, triggering the NPE.

There were also several logic issues within the loop where string bytes were not being fully processed before moving on to the next part of the chunk.

Notes for consideration before merging

1. The file provided in issue PNG throws java.lang.NullPointerException #148 now comes out as well formed and valid, but if you look at the chunk that caused the problem, it has several nulls where there should be values (for compression/compression type). The logic in the code defaults these values to 0, so the null shows up as 0. The documentation doesn't say that it's OK to put a null there, so I'm wondering if this is actually a minor error.
2. I submitted this change across two commits. The first commit fixes the issue. The second commit adds a test using the image from PNG throws java.lang.NullPointerException #148. I'm not sure if it's OK to use that image - if not I can roll it back easily. If there is another file that I can use for the test, I can switch it out.

[codecov]

Codecov Report

Merging #580 into integration will not change coverage.
The diff coverage is n/a.

@@              Coverage Diff               @@
##             integration     #580   +/-   ##
==============================================
  Coverage          45.63%   45.63%           
  Complexity          1046     1046           
==============================================
  Files                 57       57           
  Lines               9149     9149           
  Branches            1687     1687           
==============================================
  Hits                4175     4175           
  Misses              4424     4424           
  Partials             550      550           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5015b06...dd6b9b3. Read the comment docs.

[carlwilson]

The only issue remaining is that of nulls vs. zeros and I'll discuss in the conversation.

[carlwilson]

Nice :)

[carlwilson]

Notes for consideration before merging

1. The file provided in issue PNG throws java.lang.NullPointerException #148 now comes out as well formed and valid, but if you look at the chunk that caused the problem, it has several nulls where there should be values (for compression/compression type). The logic in the code defaults these values to 0, so the null shows up as 0. The documentation doesn't say that it's OK to put a null there, so I'm wondering if this is actually a minor error.

The null catch is good detective work. I've had a good scan of https://www.w3.org/TR/PNG/ paying attention to what it says about nulls. What I've learned is:

• nulls are used as field separators which infers that it's undesirable to use them elsewhere;
• for some fields, nulls are specifically disbarred, e.g. the Keyword and Text string fields described here https://www.w3.org/TR/PNG/#11tEXt, and others. The common thread is that these are all variable length text fields where this would bring obvious problems; and
• there's no stated policy for nulls.

The key text for the compression values reads:

The compression flag is 0 for uncompressed text, 1 for compressed text. Only the text field may be compressed. The compression method entry defines the compression method used. The only compression method defined in this International Standard is 0 (zlib datastream with deflate compression, see 10.3: Other uses of compression). For uncompressed text, encoders shall set the compression method to 0, and decoders shall ignore it.

So the only field that matters is the flag as the type should read 0 regardless, it's only a "future proof" marker and is currently redundant. So this leaves the flag with is set 0 for uncompressed text and 1 for compressed text. This situation is fairly typical specification stuff, and it's hard to make everything explicit when writing these documents. I'd be reluctant to make these validation errors without having a feel as to how many files it will cause to fail. I'm not sure JHOVE users should be our method of establishing that 50% of files use null values and image viewers care not. We're waiting briefly to find out about the file (below) so can think some more. If we decide to detect and report I feel that we should introduce a warning and see how may, if any, of the test samples cause the warning.

2. I submitted this change across two commits. The first commit fixes the issue. The second commit adds a test using the image from PNG throws java.lang.NullPointerException #148. I'm not sure if it's OK to use that image - if not I can roll it back easily. If there is another file that I can use for the test, I can switch it out.

I've asked @ross-spencer at #148 if this would be OK. I'll keep the PR on hold for now if that's OK?

[MartinSpeller]

PNG throws java.lang.NullPointerException #148 - Assigned to Karen Hanson

[karenhanson]

@carlwilson Your reasoning for not creating an error for the nulls in those positions makes perfect sense from my perspective.

[karenhanson]

We have not yet received permission to include the test image, so I've looked at alternatives. I wasn't able to add the iTxt fields using a hex editor, so I found another source for an example. This site has a lot of good test files for PNG, and the license supports distribution and reuse: http://www.schaik.com/pngsuite/. I've confirmed I can replicate the error using their test file, and I'm working on updating the test to point to this example instead. I'll push an update shortly. This seems like a good source if more files are needed for the PNG test copora.

[carlwilson]

We have not yet received permission to include the test image, so I've looked at alternatives. I wasn't able to add the iTxt fields using a hex editor, so I found another source for an example. This site has a lot of good test files for PNG, and the license supports distribution and reuse: http://www.schaik.com/pngsuite/. I've confirmed I can replicate the error using their test file, and I'm working on updating the test to point to this example instead. I'll push an update shortly. This seems like a good source if more files are needed for the PNG test copora.

Outstanding stuff @karenhanson and thanks for the test corpus tips, this will be merged shortly.