Don't read past the EOI marker for JPEG images with non-default restart interval (issue 7828) #8164
Conversation
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/59dd0f5968b874a/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/59dd0f5968b874a/output.txt Total script time: 2.24 mins Published |
|
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/d0e4907dedf8580/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/39cf2712dc89d41/output.txt |
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/39cf2712dc89d41/output.txt Total script time: 22.21 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/d0e4907dedf8580/output.txt Total script time: 29.38 mins
|
src/core/jpg.js
Outdated
| @@ -332,6 +332,7 @@ var JpegImage = (function JpegImageClosure() { | |||
| if (!resetInterval) { | |||
| resetInterval = mcuExpected; | |||
| } | |||
| var nonDefaultResetInterval = resetInterval !== mcuExpected; | |||
There was a problem hiding this comment.
Can this logic here be expressed by replacing new code and the if above to resetInterval = resetInterval ? Math.min(mcuExpected, resetInterval) : mcuExpected;?
There was a problem hiding this comment.
I don't think so, and quick testing seem to confirm it, since provided that I'm reading the specification correctly (see the quotes above) it should only be the last mcu where the resetInterval could potentially be an issue and not all of them.
There was a problem hiding this comment.
duh. how about var toRead = resetInterval ? Math.min(mcuExpected - mcu, resetInterval) : mcuExpected; inside the loop?
There was a problem hiding this comment.
That works better, and it's less code as well :-)
…rt interval (issue 7828) *After browsing through (a version of) the JPEG specification, see https://www.w3.org/Graphics/JPEG/itu-t81.pdf, I hope that this patch makes sense.* Note that while issue 7828 became a problem after PR 7661, it isn't really a regression from than PR. The explanation is rather that we're now relying on `core/jpg.js` instead of the Native Image decoder in more situations than before, which thus exposed an *existing* issue in our JPEG decoder. Another factor also seems to be that in many JPEG images, the DRI (Define Restart Interval) marker isn't present, in which case this bug won't manifest either. According to https://www.w3.org/Graphics/JPEG/itu-t81.pdf#page=89 (at the bottom of the page): "NOTE – The final restart interval may be smaller than the size specified by the DRI marker segment, as it includes only the number of MCUs remaining in the scan." Furthermore, according to https://www.w3.org/Graphics/JPEG/itu-t81.pdf#page=39 (in the middle of the page): "[...] If restart is enabled and the restart interval is defined to be Ri, each entropy-coded segment except the last one shall contain Ri MCUs. The last one shall contain whatever number of MCUs completes the scan." Based on the above, it thus seem to me that we should simply ensure that we're not attempting to continue to parse Scan data once we've found all MCUs (Minimum Coded Unit) of the image. Fixes 7828.
|
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/1c4b55ebc5515b3/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/ddde584883cda23/output.txt |
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/ddde584883cda23/output.txt Total script time: 22.33 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/1c4b55ebc5515b3/output.txt Total script time: 30.58 mins
|
|
/botio makeref |
From: Bot.io (Linux)ReceivedCommand cmd_makeref from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/fd3e118fefb1de6/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/63668c36e5b2556/output.txt |
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/63668c36e5b2556/output.txt Total script time: 21.83 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/fd3e118fefb1de6/output.txt Total script time: 29.45 mins
|
Don't read past the EOI marker for JPEG images with non-default restart interval (issue 7828)
After browsing through (a version of) the JPEG specification, see https://www.w3.org/Graphics/JPEG/itu-t81.pdf, I hope that this patch makes sense.
Note that while issue #7828 became a problem after PR #7661, it isn't really a regression from that PR. The explanation is rather that we're now relying on
core/jpg.jsinstead of the Native Image decoder in more situations than before, which thus exposed an existing issue in our JPEG decoder.Another factor also seems to be that in many JPEG images, the DRI (Define Restart Interval) marker isn't present, in which case this bug won't manifest either.
According to https://www.w3.org/Graphics/JPEG/itu-t81.pdf#page=89 (at the bottom of the page):
Furthermore, according to https://www.w3.org/Graphics/JPEG/itu-t81.pdf#page=39 (in the middle of the page):
Based on the above, it thus seem to me that we should simply ensure that we're not attempting to continue to parse Scan data once we've found all MCUs (Minimum Coded Unit) of the image.
Fixes #7828.