Fix integer overflow (-INT_MIN) in blink::BMPImageReader::readInfoHeader

Fix integer overflow (-INT_MIN) in blink::BMPImageReader::readInfoHeader

BUG=638445
Committed: https://crrev.com/cbe3909f4f64e0f00e2eea6898395ece4672c913
Cr-Commit-Position: refs/heads/master@{#413124}

[aleksandar.stojiljkovic, 2016-08-17 13:06:15]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ pkasting@chromium.org

[aleksandar.stojiljkovic, 2016-08-17 13:06:15]

I didn't create unit test for this since there is test in 
https://cluster-fuzz.appspot.com/.

[Peter Kasting, 2016-08-17 22:21:24]

On 2016/08/17 13:06:15, aleksandar.stojiljkovic wrote:
> I didn't create unit test for this since there is test in 
> https://cluster-fuzz.appspot.com/.

I don't believe the fact that the fuzzer caught this means it will continue
testing this particular branch in perpetuity.  We should create a new unit test.

https://codereview.chromium.org/2248383002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp
(right):

https://codereview.chromium.org/2248383002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:310: if
(m_infoHeader.biHeight == INT32_MIN)
Nit: I think this deserves a comment, e.g.:

We can't negate INT32_MIN below to get a positive int32_t.  isInfoHeaderValid()
will reject heights of 1 << 16 or larger anyway, so just reject this bitmap now.

https://codereview.chromium.org/2248383002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:311:
return false;
You need to return m_parent->setFailed() here, not just false.

[aleksandar.stojiljkovic, 2016-08-18 21:31:00]

Patch Set 2 : test and comment #3 fix. Thanks pkasting@

https://codereview.chromium.org/2248383002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp
(right):

https://codereview.chromium.org/2248383002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:310: if
(m_infoHeader.biHeight == INT32_MIN)
On 2016/08/17 22:21:24, Peter Kasting wrote:
> Nit: I think this deserves a comment, e.g.:
> 
> We can't negate INT32_MIN below to get a positive int32_t. 
isInfoHeaderValid()
> will reject heights of 1 << 16 or larger anyway, so just reject this bitmap
now.

Done.

https://codereview.chromium.org/2248383002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:311:
return false;
On 2016/08/17 22:21:24, Peter Kasting wrote:
> You need to return m_parent->setFailed() here, not just false.

Done.
Thanks. Although the returned false gets propagated to
BMPImageDecoder::decode(bool onlySize), setFailed() is called there only for
fully received files so it should be done as you suggested.

[Peter Kasting, 2016-08-18 22:06:43]

Rather than change the test code, can you just construct a BMP with the relevant
invalid height (use a hex editor if needed, but ideally the BMP would be
completely valid and internally consistent) and check that it's marked as
invalid?

[aleksandar.stojiljkovic, 2016-08-19 08:32:37]

On 2016/08/18 22:06:43, Peter Kasting wrote:
> Rather than change the test code, can you just construct a BMP with the
relevant
> invalid height (use a hex editor if needed, but ideally the BMP would be
> completely valid and internally consistent) and check that it's marked as
> invalid?

Patch Set 3: #5 fix.
I did it now using allDataReceived == false, as having -INT32_MIN x 4 bytes
would lead to >8GB file.
Otherwise, I believe that the info parsed from header was/is valid.

[Peter Kasting, 2016-08-19 08:42:51]

https://codereview.chromium.org/2248383002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageDecoderTest.cpp
(right):

https://codereview.chromium.org/2248383002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageDecoderTest.cpp:73:
const unsigned char whiteBMP[] = {
Why not just check these in as actual files in
LayoutTests/fast/images/resources/?

Also why do we need to run the whiteBMP test at all?  I'm not worried about
testing that a BMP with a negative height decodes correctly (as that's the
normal case and we already have test files that do that), and it's easier to
read a testcase that's focused on testing just one thing: the int32_min
handling.

[aleksandar.stojiljkovic, 2016-08-19 09:33:04]

Patch Set 4: fix #7.

https://codereview.chromium.org/2248383002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageDecoderTest.cpp
(right):

https://codereview.chromium.org/2248383002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageDecoderTest.cpp:73:
const unsigned char whiteBMP[] = {
On 2016/08/19 08:42:51, Peter Kasting wrote:
> Why not just check these in as actual files in
> LayoutTests/fast/images/resources/?
> 
> Also why do we need to run the whiteBMP test at all?  I'm not worried about
> testing that a BMP with a negative height decodes correctly (as that's the
> normal case and we already have test files that do that), and it's easier to
> read a testcase that's focused on testing just one thing: the int32_min
> handling.

No problem. I saw the approach used in DeferredImageDecoderTest and assumed it
is fine.
Done.

I'm testing 0x-1 because I didn't find a bmp with negative height in
LayoutTests/fast/images/resources/ neither I saw such test contributed to code
when bmp decoder was added. Probably there is a test for it, I just didn't find
it - if you know there is one, I will just remove the test. Thanks.

[Peter Kasting, 2016-08-19 09:37:25]

Yeah, we've got some top-down BMPs in the (non-public) webkit/data/bmp_decoder
dir.  SHould be safe to remove that.

LGTM

[aleksandar.stojiljkovic, 2016-08-19 09:44:47]

On 2016/08/19 09:37:25, Peter Kasting wrote:
> Yeah, we've got some top-down BMPs in the (non-public) webkit/data/bmp_decoder
> dir.  SHould be safe to remove that.
> 
> LGTM

Thanks, removed it.

[aleksandar.stojiljkovic, 2016-08-19 09:44:57]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com

[aleksandar.stojiljkovic, 2016-08-19 09:44:58]

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/2248383002/#ps80001
(title: "remove 1x-1 bmp test")

[commit-bot: I haz the power, 2016-08-19 09:45:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 13:29:54]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-19 13:31:15]

Description was changed from

==========
Fix integer overflow (-INT_MIN) in blink::BMPImageReader::readInfoHeader

BUG=638445
==========

to

==========
Fix integer overflow (-INT_MIN) in blink::BMPImageReader::readInfoHeader

BUG=638445

Committed: https://crrev.com/cbe3909f4f64e0f00e2eea6898395ece4672c913
Cr-Commit-Position: refs/heads/master@{#413124}
==========

[commit-bot: I haz the power, 2016-08-19 13:31:18]

Patchset 5 (id:??) landed as
https://crrev.com/cbe3909f4f64e0f00e2eea6898395ece4672c913
Cr-Commit-Position: refs/heads/master@{#413124}