Prevent crash in ICO caused by bad/truncated PNG

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

Index: third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp b/third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
index 75944c4ad4d4f5af0d5bdfc993cac68eff0e7ec7..3eab0b816337cf97b86b1c7546e74878a1697bac 100644
--- a/third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
@@ -218,12 +218,14 @@ bool ICOImageDecoder::decodeAtIndex(size_t 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() {