Prevent crash in ICO caused by bad/truncated PNG

third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "platform/image-decoders/ico/ICOImageDecoder.h"
 
 #include "platform/image-decoders/ImageDecoderTestHelpers.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "wtf/PtrUtil.h"
 #include <memory>
 
 namespace blink {
 
 namespace {
 
 std::unique_ptr<ImageDecoder> createDecoder() {
   return WTF::wrapUnique(
       new ICOImageDecoder(ImageDecoder::AlphaNotPremultiplied,
                           ColorBehavior::transformToTargetForTesting(),
                           ImageDecoder::noDecodedImageByteLimit));
 }
 }
 
+TEST(ICOImageDecoderTests, errorInPngInIco) {
+  auto data = readFile("/LayoutTests/images/resources/png-in-ico.ico");

[Peter Kasting, 2017/03/17 21:37:05]

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.

[scroggo_chromium, 2017/03/20 13:26:42]

Done.

+  ASSERT_FALSE(data->isEmpty());
+
+  // Modify the file to have a broken CRC in IHDR.
+  constexpr size_t crcOffset = 22u + 29u;

[Peter Kasting, 2017/03/17 21:37:05]

Nit: 'u' unnecessary on next two lines (only needed in ASSERT/EXPECT due to
template matching confusion)

[scroggo_chromium, 2017/03/20 13:26:42]

Done.

+  constexpr size_t crcSize = 4u;
+  auto modifiedData = SharedBuffer::create(data->data(), crcOffset);

[Peter Kasting, 2017/03/17 21:37:05]

Similar concern with auto.

[scroggo_chromium, 2017/03/20 13:26:42]

Done.

+  char badCrc[crcSize];
+  memset(badCrc, 0, crcSize);

[Peter Kasting, 2017/03/17 21:37:05]

Nit: Or just replace these two lines with a Vector<char> and pass that

[scroggo_chromium, 2017/03/20 13:26:42]

Done.

+  modifiedData->append(badCrc, crcSize);
+  modifiedData->append(data->data() + crcOffset + crcSize,
+                       data->size() - crcOffset - crcSize);
+  ASSERT_EQ(data->size(), modifiedData->size());

[Peter Kasting, 2017/03/17 21:37:05]

Nit: Seems like all these ASSERTs can be EXPECTs

[scroggo_chromium, 2017/03/20 13:26:43]

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.

+
+  auto decoder = createDecoder();
+  decoder->setData(modifiedData.get(), true);
+
+  // ICOImageDecoder reports the frame count based on whether enough data has
+  // been received according to the icon directory. So even though the
+  // embedded PNG is broken, there is enough data to include it in the frame
+  // count.
+  const size_t frameCount = decoder->frameCount();

[Peter Kasting, 2017/03/17 21:37:06]

Nit: Can just inline this

[scroggo_chromium, 2017/03/20 13:26:42]

Done.

+  ASSERT_EQ(1u, frameCount);
+
+  decoder->frameBufferAtIndex(0);
+  ASSERT_TRUE(decoder->failed());
+}
+
 TEST(ICOImageDecoderTests, parseAndDecodeByteByByte) {
   testByteByByteDecode(&createDecoder,
+                       "/LayoutTests/images/resources/png-in-ico.ico", 1u,
+                       cAnimationNone);
+  testByteByByteDecode(&createDecoder,
                        "/LayoutTests/images/resources/2entries.ico", 2u,
                        cAnimationNone);
   testByteByByteDecode(&createDecoder,
                        "/LayoutTests/images/resources/greenbox-3frames.cur", 3u,
                        cAnimationNone);
   testByteByByteDecode(
       &createDecoder,
       "/LayoutTests/images/resources/icon-without-and-bitmap.ico", 1u,
       cAnimationNone);
   testByteByByteDecode(&createDecoder, "/LayoutTests/images/resources/1bit.ico",
                        1u, cAnimationNone);
   testByteByByteDecode(&createDecoder,
                        "/LayoutTests/images/resources/bug653075.ico", 2u,
                        cAnimationNone);
 }
 
 }  // namespace blink