Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2754003008:
Prevent crash in ICO caused by bad/truncated PNG (Closed)
|
Created:
3 years, 9 months ago by scroggo_chromium Modified:
3 years, 9 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, blink-reviews, kinuko+watch Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrevent crash in ICO caused by bad/truncated PNG
Make ICOImageDecoder::decodeAtIndex() check for a null return value
from frameBufferAtIndex before attempting to dereference it. Prior to
adding APNG support, PNGImageDecoder::frameBufferAtIndex would never
return null, since the method only returns null if the index is out of
range, and PNGImageDecoder used to always report a frameCount of 1. Now
it can return null, if the start of the first frame has not been parsed
yet, so ICOImageDecoder needs to check for that.
Remove the unnecessary line to setPremultiplyAlpha on the ImageFrame. It
is already set on the PNGImageDecoder, which already set it on the
ImageFrame.
Add an ICO with an embedded PNG, constructed from border-image.png,
which is already used by LayoutTests. The image directory in the ICO
has an error: it reports that the PNG is 29 bytes, rather than the
actual 480 bytes. ICOImageDecoder ignores such an error, but as a
result it will call decodeAtIndex before the PNGImageDecoder knows that
it has any valid frames. Without the fix, this image would crash during
partial decode.
Add two tests for ways this bug could be triggered:
- byte by byte decoding
- an ICO with a broken PNG file embedded inside
BUG=702293
Review-Url: https://codereview.chromium.org/2754003008
Cr-Commit-Position: refs/heads/master@{#458468}
Committed: https://chromium.googlesource.com/chromium/src/+/682ebd314250a8b8597feb5b421958797c33d055
Patch Set 1 #
Total comments: 22
Patch Set 2 : Respond to comments #Patch Set 3 : Actually remove ASSERT #
Total comments: 2
Patch Set 4 : No conditional with side effect #
Dependent Patchsets: Messages
Total messages: 22 (14 generated)
scroggo@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right): https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:226: m_frameBufferCache[index].setPremultiplyAlpha(m_premultiplyAlpha); I don't see how this line is necessary. AFAICT, the PNGImageDecoder will get m_premultiplyAlpha set to the same as this ICOImageDecoder (when it is created, above), and it will set it on the ImageFrame inside frameCount (called by frameBufferAtIndex). But if that can be removed, that should perhaps be its own CL.
The CQ bit was checked by scroggo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Only the isSizeAvailable() question is blocking. https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right): https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:221: auto& pngDecoder = m_pngDecoders[index]; I would prefer "auto* pngDecoder = m_pngDecoders[index].get()". This is not only less surprising to read ("auto&" followed later by "->" made me double-take), it is safer: code below cannot modify the unique_ptr itself, just call non-const methods on its object. https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:222: if (pngDecoder->isSizeAvailable() && pngDecoder->size() != dirEntry.m_size) If isSizeAvailable() is false, should we really fail? Or should we just return false and wait for more data? https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:226: m_frameBufferCache[index].setPremultiplyAlpha(m_premultiplyAlpha); On 2017/03/17 17:50:34, scroggo_chromium wrote: > I don't see how this line is necessary. AFAICT, the PNGImageDecoder will get > m_premultiplyAlpha set to the same as this ICOImageDecoder (when it is created, > above), and it will set it on the ImageFrame inside frameCount (called by > frameBufferAtIndex). But if that can be removed, that should perhaps be its own > CL. I would just remove it now. I think this is an artifact of when premultiply state was not provided to the decoder at construction time. https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp (right): https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp:25: auto data = readFile("/LayoutTests/images/resources/png-in-ico.ico"); I don't think auto is right here, since readFile() returns a PassRefPtr, which we need to store in a RefPtr explicitly? Also, in this case (not the one below), "auto" is just unclear, since readFile()'s name does not clearly indicate what kind of a type it's returning. https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp:29: constexpr size_t crcOffset = 22u + 29u; Nit: 'u' unnecessary on next two lines (only needed in ASSERT/EXPECT due to template matching confusion) https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp:31: auto modifiedData = SharedBuffer::create(data->data(), crcOffset); Similar concern with auto. https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp:33: memset(badCrc, 0, crcSize); Nit: Or just replace these two lines with a Vector<char> and pass that https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp:37: ASSERT_EQ(data->size(), modifiedData->size()); Nit: Seems like all these ASSERTs can be EXPECTs https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp:46: const size_t frameCount = decoder->frameCount(); Nit: Can just inline this
Description was changed from ========== Prevent crash in ICO caused by bad/truncated PNG Make ICOImageDecoder::decodeAtIndex() check for a null return value from frameBufferAtIndex before attempting to dereference it. Prior to adding APNG support, PNGImageDecoder::frameBufferAtIndex would never return null, since the method only returns null if the index is out of range, and PNGImageDecoder used to always report a frameCount of 1. Now it can return null, if the start of the first frame has not been parsed yet, so ICOImageDecoder needs to check for that. Add an ICO with an embedded PNG, constructed from border-image.png, which is already used by LayoutTests. The image directory in the ICO has an error: it reports that the PNG is 29 bytes, rather than the actual 480 bytes. ICOImageDecoder ignores such an error, but as a result it will call decodeAtIndex before the PNGImageDecoder knows that it has any valid frames. Without the fix, this image would crash during partial decode. Add two tests for ways this bug could be triggered: - byte by byte decoding - an ICO with a broken PNG file embedded inside BUG=702293 ========== to ========== Prevent crash in ICO caused by bad/truncated PNG Make ICOImageDecoder::decodeAtIndex() check for a null return value from frameBufferAtIndex before attempting to dereference it. Prior to adding APNG support, PNGImageDecoder::frameBufferAtIndex would never return null, since the method only returns null if the index is out of range, and PNGImageDecoder used to always report a frameCount of 1. Now it can return null, if the start of the first frame has not been parsed yet, so ICOImageDecoder needs to check for that. Remove the unnecessary line to setPremultiplyAlpha on the ImageFrame. It is already set on the PNGImageDecoder, which already set it on the ImageFrame. Add an ICO with an embedded PNG, constructed from border-image.png, which is already used by LayoutTests. The image directory in the ICO has an error: it reports that the PNG is 29 bytes, rather than the actual 480 bytes. ICOImageDecoder ignores such an error, but as a result it will call decodeAtIndex before the PNGImageDecoder knows that it has any valid frames. Without the fix, this image would crash during partial decode. Add two tests for ways this bug could be triggered: - byte by byte decoding - an ICO with a broken PNG file embedded inside BUG=702293 ==========
https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right): https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:221: auto& pngDecoder = m_pngDecoders[index]; On 2017/03/17 21:37:05, Peter Kasting wrote: > I would prefer "auto* pngDecoder = m_pngDecoders[index].get()". > > This is not only less surprising to read ("auto&" followed later by "->" made me > double-take), it is safer: code below cannot modify the unique_ptr itself, just > call non-const methods on its object. Done. https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:222: if (pngDecoder->isSizeAvailable() && pngDecoder->size() != dirEntry.m_size) On 2017/03/17 21:37:05, Peter Kasting wrote: > If isSizeAvailable() is false, should we really fail? Or should we just return > false and wait for more data? We only fail here if the size is available and it does not match the size in the directory. If the size is not available yet, this condition evaluates to false, and we'll wait for more data. I did not change the behavior on this point (just used the local variable and condensed to one line), but if the size is not available yet, there's no point in copying the frame buffer. We could rewrite this as: if (pngDecoder->isSizeAvailable()) { if (pngDecoder->size() != dirEntry.m_size) return setFailed(); if (const auto* frame = pngDecoder->frameBufferAtIndex(0)) m_frameBufferCache[index] = *frame; } return !pngDecoder->failed() || setFailed(); Which might also be more clear? https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:226: m_frameBufferCache[index].setPremultiplyAlpha(m_premultiplyAlpha); On 2017/03/17 21:37:05, Peter Kasting wrote: > On 2017/03/17 17:50:34, scroggo_chromium wrote: > > I don't see how this line is necessary. AFAICT, the PNGImageDecoder will get > > m_premultiplyAlpha set to the same as this ICOImageDecoder (when it is > created, > > above), and it will set it on the ImageFrame inside frameCount (called by > > frameBufferAtIndex). But if that can be removed, that should perhaps be its > own > > CL. > > I would just remove it now. I think this is an artifact of when premultiply > state was not provided to the decoder at construction time. Done. https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp (right): https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp:25: auto data = readFile("/LayoutTests/images/resources/png-in-ico.ico"); On 2017/03/17 21:37:05, Peter Kasting wrote: > I don't think auto is right here, since readFile() returns a PassRefPtr, which > we need to store in a RefPtr explicitly? > > Also, in this case (not the one below), "auto" is just unclear, since > readFile()'s name does not clearly indicate what kind of a type it's returning. Done. https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp:29: constexpr size_t crcOffset = 22u + 29u; On 2017/03/17 21:37:05, Peter Kasting wrote: > Nit: 'u' unnecessary on next two lines (only needed in ASSERT/EXPECT due to > template matching confusion) Done. https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp:31: auto modifiedData = SharedBuffer::create(data->data(), crcOffset); On 2017/03/17 21:37:05, Peter Kasting wrote: > Similar concern with auto. Done. https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp:33: memset(badCrc, 0, crcSize); On 2017/03/17 21:37:05, Peter Kasting wrote: > Nit: Or just replace these two lines with a Vector<char> and pass that Done. https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp:37: ASSERT_EQ(data->size(), modifiedData->size()); On 2017/03/17 21:37:05, Peter Kasting wrote: > Nit: Seems like all these ASSERTs can be EXPECTs This one is just to verify that I've created a SharedBuffer of the right size. If I didn't, that would be a clue that I created it incorrectly, so no need to continue. OTOH, this shouldn't change, and maybe there's no need for this ASSERT. I have changed the others. https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp:46: const size_t frameCount = decoder->frameCount(); On 2017/03/17 21:37:06, Peter Kasting wrote: > Nit: Can just inline this Done.
The CQ bit was checked by scroggo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM I think the changes below are correct, but you're welcome to make them in a separate CL if you'd prefer. https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right): https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:222: if (pngDecoder->isSizeAvailable() && pngDecoder->size() != dirEntry.m_size) On 2017/03/20 13:26:42, scroggo_chromium wrote: > On 2017/03/17 21:37:05, Peter Kasting wrote: > > If isSizeAvailable() is false, should we really fail? Or should we just > return > > false and wait for more data? > > We only fail here if the size is available and it does not match the size in the > directory. If the size is not available yet, this condition evaluates to false, > and we'll wait for more data. > > I did not change the behavior on this point (just used the local variable and > condensed to one line), but if the size is not available yet, there's no point > in copying the frame buffer. We could rewrite this as: > > if (pngDecoder->isSizeAvailable()) { > if (pngDecoder->size() != dirEntry.m_size) > return setFailed(); > if (const auto* frame = pngDecoder->frameBufferAtIndex(0)) > m_frameBufferCache[index] = *frame; > } > return !pngDecoder->failed() || setFailed(); > > Which might also be more clear? Hmm: (a) I misread the new code, and (b) I think the old code might be incorrect. On point (a), you're right, you didn't change this behavior. Sorry! On point (b), I think the code should return false whenever the frame isn't complete at the end. This matches decodeBMP() as well as what the lone caller wants. This would look something like: if (pngDecoder->isSizeAvailable()) { // Fail if the size the PNGImageDecoder calculated does not match the size // in the directory. if (pngDecoder->size() != dirEntry.m_size) return setFailed(); // With APNG, it's possible for frameBufferAtIndex() to return null if the // PNG's frame count has not been parsed yet. const auto* frame = pngDecoder->frameBufferAtIndex(0); if (frame) m_frameBufferCache[index] = *frame; } if (pngDecoder->failed()) return setFailed(); return m_frameBufferCache[index].getStatus() == ImageFrame::FrameComplete; https://codereview.chromium.org/2754003008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right): https://codereview.chromium.org/2754003008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:224: if (const auto* frame = pngDecoder->frameBufferAtIndex(0)) Nit: Conditional-with-side-effect always kinda worries me, it would make me feel more comfortable to write as: const auto* frame = pngDecoder->frameBufferAtIndex(0); if (frame) m_frameBufferCache[index] = *frame;
https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right): https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:222: if (pngDecoder->isSizeAvailable() && pngDecoder->size() != dirEntry.m_size) On 2017/03/20 20:42:15, Peter Kasting wrote: > On 2017/03/20 13:26:42, scroggo_chromium wrote: > > On 2017/03/17 21:37:05, Peter Kasting wrote: > > > If isSizeAvailable() is false, should we really fail? Or should we just > > return > > > false and wait for more data? > > > > We only fail here if the size is available and it does not match the size in > the > > directory. If the size is not available yet, this condition evaluates to > false, > > and we'll wait for more data. > > > > I did not change the behavior on this point (just used the local variable and > > condensed to one line), but if the size is not available yet, there's no point > > in copying the frame buffer. We could rewrite this as: > > > > if (pngDecoder->isSizeAvailable()) { > > if (pngDecoder->size() != dirEntry.m_size) > > return setFailed(); > > if (const auto* frame = pngDecoder->frameBufferAtIndex(0)) > > m_frameBufferCache[index] = *frame; > > } > > return !pngDecoder->failed() || setFailed(); > > > > Which might also be more clear? > > Hmm: > (a) I misread the new code, and > (b) I think the old code might be incorrect. > > On point (a), you're right, you didn't change this behavior. Sorry! > > On point (b), I think the code should return false whenever the frame isn't > complete at the end. This matches decodeBMP() as well as what the lone caller > wants. This would look something like: > > if (pngDecoder->isSizeAvailable()) { > // Fail if the size the PNGImageDecoder calculated does not match the size > // in the directory. > if (pngDecoder->size() != dirEntry.m_size) > return setFailed(); > > // With APNG, it's possible for frameBufferAtIndex() to return null if the > // PNG's frame count has not been parsed yet. > const auto* frame = pngDecoder->frameBufferAtIndex(0); > if (frame) > m_frameBufferCache[index] = *frame; > } > > if (pngDecoder->failed()) > return setFailed(); > return m_frameBufferCache[index].getStatus() == ImageFrame::FrameComplete; Sgtm. I'd prefer to make this a separate change. https://codereview.chromium.org/2754003008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right): https://codereview.chromium.org/2754003008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:224: if (const auto* frame = pngDecoder->frameBufferAtIndex(0)) On 2017/03/20 20:42:15, Peter Kasting wrote: > Nit: Conditional-with-side-effect always kinda worries me, it would make me feel > more comfortable to write as: > > const auto* frame = pngDecoder->frameBufferAtIndex(0); > if (frame) > m_frameBufferCache[index] = *frame; I do not have a strong preference here. noel@ has been encouraging me to use these, so I did the same here. Done.
The CQ bit was checked by scroggo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2754003008/#ps60001 (title: "No conditional with side effect")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right): https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:222: if (pngDecoder->isSizeAvailable() && pngDecoder->size() != dirEntry.m_size) On 2017/03/21 16:03:30, scroggo_chromium wrote: > On 2017/03/20 20:42:15, Peter Kasting wrote: > > On 2017/03/20 13:26:42, scroggo_chromium wrote: > > > On 2017/03/17 21:37:05, Peter Kasting wrote: > > > > If isSizeAvailable() is false, should we really fail? Or should we just > > > return > > > > false and wait for more data? > > > > > > We only fail here if the size is available and it does not match the size in > > the > > > directory. If the size is not available yet, this condition evaluates to > > false, > > > and we'll wait for more data. > > > > > > I did not change the behavior on this point (just used the local variable > and > > > condensed to one line), but if the size is not available yet, there's no > point > > > in copying the frame buffer. We could rewrite this as: > > > > > > if (pngDecoder->isSizeAvailable()) { > > > if (pngDecoder->size() != dirEntry.m_size) > > > return setFailed(); > > > if (const auto* frame = pngDecoder->frameBufferAtIndex(0)) > > > m_frameBufferCache[index] = *frame; > > > } > > > return !pngDecoder->failed() || setFailed(); > > > > > > Which might also be more clear? > > > > Hmm: > > (a) I misread the new code, and > > (b) I think the old code might be incorrect. > > > > On point (a), you're right, you didn't change this behavior. Sorry! > > > > On point (b), I think the code should return false whenever the frame isn't > > complete at the end. This matches decodeBMP() as well as what the lone caller > > wants. This would look something like: > > > > if (pngDecoder->isSizeAvailable()) { > > // Fail if the size the PNGImageDecoder calculated does not match the size > > // in the directory. > > if (pngDecoder->size() != dirEntry.m_size) > > return setFailed(); > > > > // With APNG, it's possible for frameBufferAtIndex() to return null if the > > // PNG's frame count has not been parsed yet. > > const auto* frame = pngDecoder->frameBufferAtIndex(0); > > if (frame) > > m_frameBufferCache[index] = *frame; > > } > > > > if (pngDecoder->failed()) > > return setFailed(); > > return m_frameBufferCache[index].getStatus() == ImageFrame::FrameComplete; > > Sgtm. I'd prefer to make this a separate change. Uploaded https://codereview.chromium.org/2761303003
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490112216015470,
"parent_rev": "5df6441ae72bff7588641737443527cd1be44a38", "commit_rev":
"682ebd314250a8b8597feb5b421958797c33d055"}
Message was sent while issue was closed.
Description was changed from ========== Prevent crash in ICO caused by bad/truncated PNG Make ICOImageDecoder::decodeAtIndex() check for a null return value from frameBufferAtIndex before attempting to dereference it. Prior to adding APNG support, PNGImageDecoder::frameBufferAtIndex would never return null, since the method only returns null if the index is out of range, and PNGImageDecoder used to always report a frameCount of 1. Now it can return null, if the start of the first frame has not been parsed yet, so ICOImageDecoder needs to check for that. Remove the unnecessary line to setPremultiplyAlpha on the ImageFrame. It is already set on the PNGImageDecoder, which already set it on the ImageFrame. Add an ICO with an embedded PNG, constructed from border-image.png, which is already used by LayoutTests. The image directory in the ICO has an error: it reports that the PNG is 29 bytes, rather than the actual 480 bytes. ICOImageDecoder ignores such an error, but as a result it will call decodeAtIndex before the PNGImageDecoder knows that it has any valid frames. Without the fix, this image would crash during partial decode. Add two tests for ways this bug could be triggered: - byte by byte decoding - an ICO with a broken PNG file embedded inside BUG=702293 ========== to ========== Prevent crash in ICO caused by bad/truncated PNG Make ICOImageDecoder::decodeAtIndex() check for a null return value from frameBufferAtIndex before attempting to dereference it. Prior to adding APNG support, PNGImageDecoder::frameBufferAtIndex would never return null, since the method only returns null if the index is out of range, and PNGImageDecoder used to always report a frameCount of 1. Now it can return null, if the start of the first frame has not been parsed yet, so ICOImageDecoder needs to check for that. Remove the unnecessary line to setPremultiplyAlpha on the ImageFrame. It is already set on the PNGImageDecoder, which already set it on the ImageFrame. Add an ICO with an embedded PNG, constructed from border-image.png, which is already used by LayoutTests. The image directory in the ICO has an error: it reports that the PNG is 29 bytes, rather than the actual 480 bytes. ICOImageDecoder ignores such an error, but as a result it will call decodeAtIndex before the PNGImageDecoder knows that it has any valid frames. Without the fix, this image would crash during partial decode. Add two tests for ways this bug could be triggered: - byte by byte decoding - an ICO with a broken PNG file embedded inside BUG=702293 Review-Url: https://codereview.chromium.org/2754003008 Cr-Commit-Position: refs/heads/master@{#458468} Committed: https://chromium.googlesource.com/chromium/src/+/682ebd314250a8b8597feb5b4219... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/682ebd314250a8b8597feb5b4219... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
