Add previewBuffer type as a workaround for Sony preview images #2008
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2008 +/- ##
==========================================
+ Coverage 61.16% 61.17% +0.01%
==========================================
Files 96 96
Lines 19255 19285 +30
Branches 9862 9862
==========================================
+ Hits 11777 11798 +21
- Misses 5135 5146 +11
+ Partials 2343 2341 -2
Continue to review full report at Codecov.
|
kevinbackhouse
force-pushed
the
FixIssue2001
branch
2 times, most recently
from
84ecb6f to
5eab1d1
Compare
kevinbackhouse
force-pushed
the
FixIssue2001
branch
from
5eab1d1 to
bd35cb8
Compare
There was a problem hiding this comment.
@kevinbackhouse This looks ingenious.
I will confess to being nervous about introducing new classes and typeIds. There could be unexpected "ripple". The simple fix of warning when a preview is > 4mb seems much less brutal and risky.
However, you've put in a solid effort and this deserves detailed inspection with the debugger and I will do that in the next couple of days.
|
This is very good work, @kevinbackhouse. I don't want to accept this into 0.27-maintenance. However, I will approve this for 'main' if you wish. As the saying goes "when you are in a hole, stop digging". The hole here is that the "external" preview is a Sony violation of the Exif spec. By adding a typeId to deal with this, you are saying it's OK. A simpler solution is to warn that Sony tag 0x2001 is illegal and take no further action (no allocation, do nothing at all). You are right that an arbitrary preview limit of 1mb, or 4mb can we swamped by adding sufficient previews. The safe limit is to warn and ignore all "external" previews. I am bothered by the use of 0xffff. Why can't you use About 12 months ago there was a discussion about SubImages which have an arbitrary restriction of 10. I am suspicious of the Exiv2 Architecture concerning SubImages and the multitude of groups such as BTW, I don't know why there are typeIds for XMP data. I suspect that's another architectural issue that requires investigation and redesign. My suspicion is that it was added in 2005 or 2006 as a "quick fix" to support the XMPsdk and has never been properly addressed. |
|
I thought of something to say in great praise of your design of assigning a new typeId for "external" previews. I recall Andreas talking about those previews and saying that it's probably impossible to rewrite one of those files without damaging the preview. Your design addresses that issue beautifully as you have allocated and stored the preview. We should modify |
|
@clanmills: I had an idea for a hacky workaround that doesn't involve adding a new type: #2013. The idea is to put a string in the metadata rather than a big buffer full of zeros. Maybe something like that could be a pragmatic solution for the 0.27-maintenance branch? |
|
I have converted this to draft so that we can revisit it in the future. The Meanwhile, #2015 is probably a better short-term fix. |
|
@kevinbackhouse I think you're right AND you're incorrect. You're right: When you floated the concept of previewBuffer, I thought "That's inginenous. We will allocate and track previews and that should make the rewrite easy". You're incorrect: Now that I've tracked down and found a test file, I thinking that we don't need to store offset/length. These previews are JPEG artefacts and should not be cross referenced in the Exif data. However, let's hold this. If David @dhoulder works on this during "The Holidays" the truth will emerge. We don't have an spec for 0x2001. If it's an offset/length to the preview, we might need to write the whole file, and then use a fixup to edit the offset/length in 0x2001. However, I suspect the reason the 0x2001 count is zero in some files is because you can find the preview by parsing the JPEG and don't need the offset/length at all! For sure, my Nikon D5300 stores previews in this way without the need for an equivalent to Sony/0x2001. Together, we're making good progress with this. |
|
You're always welcome to contribute @piponazo. Good to have you here. You did very good work this week on #2018. I think this PR can be closed as it was superceded by #2015. However, let's leave it to @kevinbackhouse to close this and delete the branch. |
Fixes: #2001
This definitely needs review from somebody who knows Exiv2's architecture better than I do.
This is attempting to fix the problem caused by Sony preview images, which are stored in the main body of the image file, but referenced in the metadata. Previously, we were allocating a buffer of the correct size, but filling it with zeros because we don't have access to the result of the image file while we're parsing the metadata. A malicious file could use that to trigger an out-of-memory crash (see #1881). I have fixed it here by adding a new type called
previewBuffer, which is a placeholder that stores the size without actually allocating the memory.I'm not sure if it's ok to add a fake type in types.hpp like I have done here. If it is ok, then I think this should be a good solution to the problem.