PNG: Verify CRC of IDAT chunk is correct #1898
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1898 +/- ##
======================================
- Coverage 87% 87% -1%
======================================
Files 960 960
Lines 50972 50975 +3
Branches 6310 6311 +1
======================================
+ Hits 44731 44733 +2
Misses 5202 5202
- Partials 1039 1040 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| // Restore the stream position for IDAT chunks, because it will be decoded later and | ||
| // was only read to verifying the CRC is correct. | ||
| if (type == PngChunkType.Data) | ||
| { | ||
| this.currentStream.Position = pos; | ||
| } |
There was a problem hiding this comment.
Why do we need to read the IDAT chunk twice? Upon rewinding the stream, this means creating & filling PngChunk.Data twice. Can't we avoid this?
There was a problem hiding this comment.
I dont see how we can avoid reading the stream twice. We need to read the stream completely once to be able to calculate the CRC. Then we are sure the byte stream is not corrupted and can start decoding it in ZlibInflateStream. Otherwise we would need to introduce PNG specific logic into ZlibInflateStream, which i dont think would be good.
Also the compressed stream can span over multiple IDAT chunks, which would make it even more complicated.
Note also the PngChunk.Data is not filled twice, just once. Before this PR the PngChunk.Data was left empty.
So we have now with this PR an additional allocation of the size of IDAT chunk.
There was a problem hiding this comment.
Does libpng check it? I know it’s optional but haven’t had the chance to dig through the code
There was a problem hiding this comment.
yes, libpng does check it
There was a problem hiding this comment.
One way to possible avoid a large allocation here for the IDAT chunk just to calculate the CRC could be to read the IDAT data in smaller portions from the stream (lets say 8192 bytes) and calculate the CRC on those smaller chunk's of data.
Is it worth introducing such special handling for calculating the CRC of IDAT?
There was a problem hiding this comment.
ZlibInflateStream operates over BufferedReadStream, so we can consider making CRC32 calculation as an optional part of BufferedReadStream buffer fills. Although it's definitely not a 2.0 optimization, I would open a separate issue to investigate this.
read the
IDATdata in smaller portions from the stream (lets say 8192 bytes) and calculate the CRC on those smaller chunk's of data.
If my understanding is correct, PNG-s with single very large IDAT chunks are rare, so it's probably not worth it, I would better investigate the BufferedReadStream approach, since it also avoids stream rewinding.
Co-authored-by: Anton Firszov <antonfir@gmail.com>
antonfirsov
left a comment
antonfirsov
left a comment
There was a problem hiding this comment.
Looks good for 2.0, but might worth to investigate on-the-fly CRC32 calculation in the future.
Prerequisites
Description
This PR changes the PNG decoder to verify that the CRC of IDAT chunk is correct.