Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 2754003008: Prevent crash in ICO caused by bad/truncated PNG (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by scroggo_chromium
Modified:
2 months ago
Reviewers:
Peter Kasting
CC:
chromium-reviews, blink-reviews, kinuko+watch
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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 #

Messages

Total messages: 22 (14 generated)
scroggo_chromium
https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right): https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp#newcode226 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. ...
2 months, 1 week ago (2017-03-17 17:50:34 UTC) #2
Peter Kasting
Only the isSizeAvailable() question is blocking. https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right): https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp#newcode221 third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:221: auto& pngDecoder = ...
2 months, 1 week ago (2017-03-17 21:37:06 UTC) #7
scroggo_chromium
https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right): https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp#newcode221 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 ...
2 months ago (2017-03-20 13:26:43 UTC) #9
Peter Kasting
LGTM I think the changes below are correct, but you're welcome to make them in ...
2 months ago (2017-03-20 20:42:15 UTC) #14
scroggo_chromium
https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right): https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp#newcode222 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, ...
2 months ago (2017-03-21 16:03:31 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2754003008/60001
2 months ago (2017-03-21 16:03:55 UTC) #18
scroggo_chromium
https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right): https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp#newcode222 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, ...
2 months ago (2017-03-21 16:16:52 UTC) #19
commit-bot: I haz the power
2 months ago (2017-03-21 18:00:29 UTC) #22
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/682ebd314250a8b8597feb5b4219...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06