-
Notifications
You must be signed in to change notification settings - Fork 488
IPTC metadata: Fix an off-by-one error. #172
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
Conversation
Fixes an issue where a single-byte value at the end of an IPTC directory would not be sufficient to consider the number of remaining bytes sufficient, causing the extraction to be halted prematurely.
// we need at least five bytes left to read a tag | ||
if (offset + 5 >= length) { | ||
// we need at least four bytes left to read a tag | ||
if (offset + 4 >= length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think the comment is correct. You need at least 5 bytes:
- 1 for dir type
- 1 for tag type
- 2 for byte count
- 1+ for contents
I don't think it's valid to have zero bytes of content.
Do you have an image that reproduces this behaviour? I can't quite make out the problem here.
Thanks, yes - here's an example: At the end of the IPTC directory of that file is an ObjectName tag with a single-byte value. Currently, it would require the ObjectName tag in this example to have at least a 2-byte value. As best I can tell, the IPTC directory in that file is valid. |
Cheers. I'll take a look. Are you willing/able to donate that image to the project for regression testing? |
I think semantically the correct code is:
Thanks for finding this bug. Actually it occurs in two images in the test data set (Nikon D1X and Nikon D40) as well. Your sample image is quite exotic, coming from a Maginon WK1. I had to Google that! Looks like a very cool camera, and now the image makes a bit more sense too. |
Just released this in 2.9.1. |
Wow, thanks for the fast response! For the regression test library, the owner would prefer a watermarked image be included. It sounds like you have this case covered already, though I'm happy to upload an example suitable for inclusion. I did recognise the image was a bit out-there. For context, the Maginon camera (& metadata-extractor) is being used in wildlife conservation efforts in Vietnam. Thanks again. |
Unfortunately watermarking images involves putting them through software. Software tends to do strange things to the metadata, and when an image has been processed via software I generally name it as coming from the software, rather than the camera. In this case I can reproduce the scenario in a unit test if the photographer is unwilling to offer a raw image for test data. Would be a shame. There's nothing commercial going on here, though my father's a professional photographer who watermarks everything online so I understand where they're coming from. I could give them credit in the commit log though, so it would be clear who provided the image. |
Fixes an issue where a single-byte value at the end of an IPTC directory
would not be sufficient to consider the number of remaining bytes
sufficient, causing the extraction to be halted prematurely.