Prevent crash in ICO caused by bad/truncated PNG

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

 /*
  * Copyright (c) 2008, 2009, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 200 matching lines @@
   if (!m_pngDecoders[index]) {
     AlphaOption alphaOption =
         m_premultiplyAlpha ? AlphaPremultiplied : AlphaNotPremultiplied;
     m_pngDecoders[index] = WTF::wrapUnique(
         new PNGImageDecoder(alphaOption, m_colorBehavior, m_maxDecodedBytes,
                             dirEntry.m_imageOffset));
     setDataForPNGDecoderAtIndex(index);
   }
   // Fail if the size the PNGImageDecoder calculated does not match the size
   // in the directory.
-  if (m_pngDecoders[index]->isSizeAvailable() &&
-      (m_pngDecoders[index]->size() != dirEntry.m_size))
+  auto& pngDecoder = m_pngDecoders[index];

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

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.

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

Done.

+  if (pngDecoder->isSizeAvailable() && pngDecoder->size() != dirEntry.m_size)

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

If isSizeAvailable() is false, should we really fail?  Or should we just return
false and wait for more data?

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

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?

[Peter Kasting, 2017/03/20 20:42:15]

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;

[scroggo_chromium, 2017/03/21 16:03:30]

Sgtm. I'd prefer to make this a separate change.

[scroggo_chromium, 2017/03/21 16:16:52]

Uploaded https://codereview.chromium.org/2761303003

     return setFailed();
-  m_frameBufferCache[index] = *m_pngDecoders[index]->frameBufferAtIndex(0);
-  m_frameBufferCache[index].setPremultiplyAlpha(m_premultiplyAlpha);
-  return !m_pngDecoders[index]->failed() || setFailed();
+  if (const auto* frame = pngDecoder->frameBufferAtIndex(0)) {
+    m_frameBufferCache[index] = *frame;
+    m_frameBufferCache[index].setPremultiplyAlpha(m_premultiplyAlpha);

[scroggo_chromium, 2017/03/17 17:50:34]

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.

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

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.

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

Done.

+  }
+  return !pngDecoder->failed() || setFailed();
 }
 
 bool ICOImageDecoder::processDirectory() {
   // Read directory.
   DCHECK(!m_decodedOffset);
   if (m_data->size() < sizeOfDirectory)
     return false;
   const uint16_t fileType = readUint16(2);
   m_dirEntriesCount = readUint16(4);
   m_decodedOffset = sizeOfDirectory;
@@ skipping 88 matching lines @@
   SECURITY_DCHECK(index < m_dirEntries.size());
   const uint32_t imageOffset = m_dirEntries[index].m_imageOffset;
   if ((imageOffset > m_data->size()) || ((m_data->size() - imageOffset) < 4))
     return Unknown;
   char buffer[4];
   const char* data = m_fastReader.getConsecutiveData(imageOffset, 4, buffer);
   return strncmp(data, "\x89PNG", 4) ? BMP : PNG;
 }
 
 }  // namespace blink