Don't decode AND mask for an icon that already has alpha information

Source/platform/image-decoders/ico/ICOImageDecoder.cpp

Index: Source/platform/image-decoders/ico/ICOImageDecoder.cpp
diff --git a/Source/platform/image-decoders/ico/ICOImageDecoder.cpp b/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
index 06dce8e51035e6f192292e52224884faebbab5c7..14bf784db393d90e9bc9228cdbf2883fccf4161c 100644
--- a/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
+++ b/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
@@ -216,8 +216,11 @@ bool ICOImageDecoder::decodeAtIndex(size_t index)
             // We need to have already sized m_frameBufferCache before this, and
             // we must not resize it again later (see caution in frameCount()).
             ASSERT(m_frameBufferCache.size() == m_dirEntries.size());
-            m_bmpReaders[index] = adoptPtr(new BMPImageReader(this, dirEntry.m_imageOffset, 0, true));
-            m_bmpReaders[index]->setData(m_data.get());
+            // Make sure the image data doesn't go beyond the end of the file.
+            uint32_t imageSize = std::min(dirEntry.m_imageSize, m_data->size() - dirEntry.m_imageOffset);
+            RefPtr<SharedBuffer> bmpData(SharedBuffer::create(&m_data->data()[dirEntry.m_imageOffset], imageSize));
+            m_bmpReaders[index] = adoptPtr(new BMPImageReader(this, 0, 0, true));
+            m_bmpReaders[index]->setData(bmpData.get());

[Peter Kasting, 2014/11/18 20:59:05]

I have three concerns with this code, in decreasing order.

(1) We also call setData() on the BMP reader in ICOImageDecoder::setData()
above, for when e.g. more data comes in after an initial decode attempt.  If we
want to be correct, we have to either mirror this clamp there, or we have to
instead inform the BMP decoder about the size limits so it will always avoid
running over its data limit.

(2) Is the image size in the directory entry always trustworthy?  Do other
browsers always use it, do they ignore it if it's 0, do they use some other
number to calculate this limit?  There are so many malformed images on the web
that I'm worried about this patch fixing this case and breaking others.  Perhaps
instead of trusting the image size field, we should instead clamp to "however
much data we have before the start of the next entry"?  That seems potentially
safer.

(3) There's also a setDataForPNGDecoderAtIndex() function above.  You're only
clamping for BMP.  Perhaps we should also clamp for other types of embedded
images?

             m_bmpReaders[index]->setBuffer(&m_frameBufferCache[index]);
         }
         m_frameSize = dirEntry.m_size;
@@ -314,6 +317,7 @@ ICOImageDecoder::IconDirectoryEntry ICOImageDecoder::readDirectoryEntry()
         entry.m_bitCount = readUint16(6);
         entry.m_hotSpot = IntPoint();
     }
+    entry.m_imageSize = readUint32(8);
     entry.m_imageOffset = readUint32(12);
 
     // Some icons don't have a bit depth, only a color count.  Convert the