fix: incorrectly triggered enforce check during preview extraction #1835
Conversation
hassec
force-pushed
the
issue_1829
branch
2 times, most recently
from
6afd15e
to
ffe394c
Compare
Codecov Report
@@ Coverage Diff @@
## main #1835 +/- ##
==========================================
- Coverage 60.59% 60.58% -0.01%
==========================================
Files 96 96
Lines 18860 18864 +4
Branches 9479 9483 +4
==========================================
+ Hits 11428 11429 +1
- Misses 5142 5146 +4
+ Partials 2290 2289 -1
Continue to review full report at Codecov.
|
|
Should this be fixed on 0.27-maintenance? |
|
I don't think so. It's a feature fix, but not a security issue. In fact, the problem here is that the security was a bit too strict :D |
|
But it's a bug, isn't it? On 0.27-maintenance, we could do the simpler fix of just replacing |
… LoaderTiff::getData(), closes #1829
|
Yep, but my understanding was that we won't do anything on 0.27-maintenance aside from security fixes. This bug has been dormant in exiv2 for many years. The reporting user was using That being said, it's a tiny fix so sure we can easily port this to |
|
I think we should fix this one on 0.27-maintenance, since it's such a simple change. Also, it looks like the bug was caused by an over-enthusiastic security fix, so it's slightly related to security! |
|
I don't have a strong opinion about this, so happy to go either way 👍 |
|
@Mergifyio backport 0.27-maintenance |
|
Command
|
This should close #1829.
The problem was an
enforcestatement insideLoaderTiff::getData()I'm not an expert on the entire preview extraction stuff so take the below with a grain of salt.
But it seems that there are previews inside a file which are defined by "stripes" or "tiles"
If that is the case, they are inside the exifdata like below:
Where the first one tells you at what offset into the file the preview starts and the second tells you how large the preview is in bytes.
But sometimes you get many offsets and counts, (who knows why, didn't really look into that yet, but the resulting image is in fact a preview 🤷♂️)
And that's what would trigger the bug.
You can see that the function
LoaderTiff::getData()behaves differently if you need to copy many chunks.It checks each loop iteration to make sure that the value of
IdxBuf + size of current chunkdoesn't overflow theDataBufwe are copying the entire image into.It just ignored that by construction this check is going to be false on the last iteration where the two should be equal.
I'm not sure how to make a proper test case for this without introducing a large file :/
Let's add this as yet another +1 to the long list of reasons why it would really be great to soon somehow start testing more one real files.