Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1180013006:
Do not consume input of corrupt progressive JPEG (Closed)
|
Created:
5 years, 6 months ago by Noel Gordon Modified:
5 years, 6 months ago CC:
blink-reviews, urvang Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionDo not consume input of corrupt progressive JPEG
Add an error emitter override, and use it to count "Corrupt JPEG" warning
messages from libjpeg during progressive JPEG decoding to prevent reading
ahead with jpeg_consume_input() if the JPEG is corrupt: the input data is
bogus and can result in different renderings of the same image otherwise.
Also, during initialization, set the m_info fields for a progressive JPEG
(aka multi-scan or buffered image) decode to default values. Add FIXME.
BUG=500567
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197353
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address review comments. #Messages
Total messages: 20 (7 generated)
noel@chromium.org changed reviewers: + pkasting@google.com, scroggo@google.com
Not possible to auto test this as we found out in http://crbug.com/398235 but with this patch, the test case on the bug produces one rendering only (good).
scroggo@chromium.org changed reviewers: + scroggo@chromium.org
lgtm https://codereview.chromium.org/1180013006/diff/1/Source/platform/image-decod... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/1180013006/diff/1/Source/platform/image-decod... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:319: m_err.pub.emit_message = emit_message; Besides error reporting, it looks like the behavior change is only for a progressive image. Should we use the default emit_message for sequential jpegs? (Or do you want consistent error reporting for both?) On that note, should we also look for corrupt data in a sequential jpeg? (Or should we wait until we see problems resulting from a corrupt sequential jpeg to try to handle it?) https://codereview.chromium.org/1180013006/diff/1/Source/platform/image-decod... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:498: m_info.enable_external_quant = false; Are these (setting to false and 0) necessary or just here for completeness? (It seems like the code above to memset m_info to 0 already accomplished this. IIUC, we should only reach here once, unless jpeg_start_decompress returned false; can these be changed by jpeg_start_decompress?) https://codereview.chromium.org/1180013006/diff/1/Source/platform/image-decod... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:674: err->num_corrupt_warnings++; This appears to be used as a boolean. Do we need to count each one? (Or is that for debugging purposes?)
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
Leon's questions are the same ones I would ask, so I'm going to LGTM this as owner and let you work with him on the actual code review.
https://codereview.chromium.org/1180013006/diff/1/Source/platform/image-decod... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/1180013006/diff/1/Source/platform/image-decod... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:319: m_err.pub.emit_message = emit_message; On 2015/06/15 15:57:06, scroggo_chromium wrote: > Besides error reporting, it looks like the behavior change is only for a > progressive image. Should we use the default emit_message for sequential jpegs? > (Or do you want consistent error reporting for both?) > > On that note, should we also look for corrupt data in a sequential jpeg? (Or > should we wait until we see problems resulting from a corrupt sequential jpeg to > try to handle it?) Good points. Since we have corruption for sequential JPEG handled already per https://crbug.com/398235, we don't really need an emit_error for them. We do for the progressive (aka multi-scan / buffered image) JPEG case, so in the next patch set, I restricted adding the emit_message handler in that case only, so the patch is a change in behavior for progressive JPEG only. With that change, the test-case http://lcamtuf.coredump.cx/ffjpeg still shows one rendering (good: that's the goal here). https://codereview.chromium.org/1180013006/diff/1/Source/platform/image-decod... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:498: m_info.enable_external_quant = false; On 2015/06/15 15:57:06, scroggo_chromium wrote: > Are these (setting to false and 0) necessary or just here for completeness? (It > seems like the code above to memset m_info to 0 already accomplished this. For completeness. Two variables related to buffered image mode decompression were set (see the diff left). I had to find all the others and wrote them down here for completeness. It's a reminder to me of what they are, but also that these variables interact with one another in somewhat tricky ways (see libjpeg.txt). > IIUC, we should only reach here once, unless jpeg_start_decompress returned false; Correct. Aside: and if we did come here twice, anyone notice a memory leak? > can these be changed by jpeg_start_decompress?) In general, they need to set before jpeg_start_decompress since it reads them and updates internal libjpeg state, and resets some them in some circumstances. You may change some of them after jpeg_start_decompress, but not all of them. About the only interesting one to change after is do_block_smoothing. For example, you could disable it after some number of progressive frames had been decoded, the idea being that the image has sufficient detail at that time to not need block smoothing at all (block smoothing is a relatively costly operation). A good example of the effect of block smoothing on the initial image frames is http://pooyak.com/p/progjpeg, the initial frames are blurry (block smoothed). An earlier version of this change did that, but I could not decide on which frame number to use to disable block smoothing, so I left it out. Anyho, TL,DR; they are here for completeness. We could also just ASSERT them. Would you prefer that? https://codereview.chromium.org/1180013006/diff/1/Source/platform/image-decod... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:674: err->num_corrupt_warnings++; On 2015/06/15 15:57:05, scroggo_chromium wrote: > This appears to be used as a boolean. Do we need to count each one? (Or is that > for debugging purposes?) For debug but also compat with the way libjpeg internally counts warnings: err->num_warnings++; /me not wanting to fall too far from the tree.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1180013006/#ps20001 (title: "Address review comments.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180013006/20001
lgtm https://codereview.chromium.org/1180013006/diff/1/Source/platform/image-decod... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/1180013006/diff/1/Source/platform/image-decod... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:498: m_info.enable_external_quant = false; On 2015/06/18 12:10:07, noel gordon wrote: > On 2015/06/15 15:57:06, scroggo_chromium wrote: > > Are these (setting to false and 0) necessary or just here for completeness? > (It > > seems like the code above to memset m_info to 0 already accomplished this. > > For completeness. Two variables related to buffered image mode decompression > were set (see the diff left). I had to find all the others and wrote them down > here for completeness. It's a reminder to me of what they are, but also that > these variables interact with one another in somewhat tricky ways (see > libjpeg.txt). > > > IIUC, we should only reach here once, unless jpeg_start_decompress returned > false; > > Correct. Aside: and if we did come here twice, anyone notice a memory leak? > > > can these be changed by jpeg_start_decompress?) > > In general, they need to set before jpeg_start_decompress since it reads them > and updates internal libjpeg state, and resets some them in some circumstances. > > You may change some of them after jpeg_start_decompress, but not all of them. > > About the only interesting one to change after is do_block_smoothing. For > example, you could disable it after some number of progressive frames had been > decoded, the idea being that the image has sufficient detail at that time to not > need block smoothing at all (block smoothing is a relatively costly operation). > > A good example of the effect of block smoothing on the initial image frames is > http://pooyak.com/p/progjpeg, the initial frames are blurry (block smoothed). > > An earlier version of this change did that, but I could not decide on which > frame number to use to disable block smoothing, so I left it out. > > Anyho, TL,DR; they are here for completeness. We could also just ASSERT them. > Would you prefer that? Either way. I just wanted to make sure I understand why they were there. I think the comment in patch set 2 makes it clear they should already be set. https://codereview.chromium.org/1180013006/diff/1/Source/platform/image-decod... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:674: err->num_corrupt_warnings++; On 2015/06/18 12:10:07, noel gordon wrote: > On 2015/06/15 15:57:05, scroggo_chromium wrote: > > This appears to be used as a boolean. Do we need to count each one? (Or is > that > > for debugging purposes?) > > For debug but also compat with the way libjpeg internally counts warnings: > err->num_warnings++; /me not wanting to fall too far from the tree. sgtm
On 2015/06/18 13:19:24, scroggo_chromium wrote: > lgtm > > https://codereview.chromium.org/1180013006/diff/1/Source/platform/image-decod... > File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): > > https://codereview.chromium.org/1180013006/diff/1/Source/platform/image-decod... > Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:498: > m_info.enable_external_quant = false; > On 2015/06/18 12:10:07, noel gordon wrote: > > On 2015/06/15 15:57:06, scroggo_chromium wrote: > > > Are these (setting to false and 0) necessary or just here for completeness? > > (It > > > seems like the code above to memset m_info to 0 already accomplished this. > > > > For completeness. Two variables related to buffered image mode decompression > > were set (see the diff left). I had to find all the others and wrote them > down > > here for completeness. It's a reminder to me of what they are, but also that > > these variables interact with one another in somewhat tricky ways (see > > libjpeg.txt). > > > > > IIUC, we should only reach here once, unless jpeg_start_decompress returned > > false; > > > > Correct. Aside: and if we did come here twice, anyone notice a memory leak? > > > > > can these be changed by jpeg_start_decompress?) > > > > In general, they need to set before jpeg_start_decompress since it reads them > > and updates internal libjpeg state, and resets some them in some > circumstances. > > > > You may change some of them after jpeg_start_decompress, but not all of them. > > > > About the only interesting one to change after is do_block_smoothing. For > > example, you could disable it after some number of progressive frames had been > > decoded, the idea being that the image has sufficient detail at that time to > not > > need block smoothing at all (block smoothing is a relatively costly > operation). > > > > A good example of the effect of block smoothing on the initial image frames is > > http://pooyak.com/p/progjpeg, the initial frames are blurry (block smoothed). > > > > An earlier version of this change did that, but I could not decide on which > > frame number to use to disable block smoothing, so I left it out. ^^^ Hope you noticed this part, might have uses for the Android use-case?
On 2015/06/18 13:19:24, scroggo_chromium wrote: > > Anyho, TL,DR; they are here for completeness. We could also just ASSERT them. > > > Would you prefer that? > > Either way. I just wanted to make sure I understand why they were there. I think > the comment in patch set 2 makes it clear they should already be set. Ok thanks, and we have a FIXME.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180013006/20001
Updated changelog.
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197353 |
