detect bad bitmaps during deserialization

src/core/SkPictureData.cpp

 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 #include <new>
 #include "SkBBoxHierarchy.h"
 #include "SkDrawPictureCallback.h"
 #include "SkPictureData.h"
@@ skipping 377 matching lines @@
                 fPictureCount = 0;
                 return false;
             }
         } break;
         case SK_PICT_BUFFER_SIZE_TAG: {
             SkAutoMalloc storage(size);
             if (stream->read(storage.get(), size) != size) {
                 return false;
             }
 
+            /* Should we use SkValidatingReadBuffer instead? */

[scroggo, 2014/11/12 18:24:47]

If we're going to check isValid, absolutely! Otherwise the checks for isValid
are not useful.

             SkReadBuffer buffer(storage.get(), size);
             buffer.setFlags(pictInfoFlagsToReadBufferFlags(fInfo.fFlags));
             buffer.setVersion(fInfo.fVersion);
 
             fFactoryPlayback->setupBuffer(buffer);
             fTFPlayback.setupBuffer(buffer);
             buffer.setBitmapDecoder(proc);
 
-            while (!buffer.eof()) {
+            while (!buffer.eof() && buffer.isValid()) {
                 tag = buffer.readUInt();
                 size = buffer.readUInt();
                 if (!this->parseBufferTag(buffer, tag, size)) {
                     return false;
                 }
             }
+            if (!buffer.isValid()) {

[scroggo, 2014/11/12 18:24:47]

Do we need both checks?

Is it possible for isValid to be false when entering the loop (assuming we've
changed to SkValidatingReadBuffer)? If not, we can move this check to the end of
the while loop and remove the second check.

Taken a step further, can buffer.eof() be true when we first enter the loop? I'm
guessing only if size is 0?

Could we rewrite this loop as:

do {
  tag = buffer.readUInt();
  size = buffer.readUInt();
  if (!parseBufferTag(buffer, tag, size)) {
    return false;
  }
  if (!buffer.isValid()) {
    return false;
  }
} while (!buffer.eof());

(As a side note, that still doesn't fix the problem situation where buffer was
not at eof at the beginning of the loop, but it is after the first uint, or the
second, or part way into parseBufferTag. That seems like a deeper problem, which
we may have convinced ourselves we don't need to worry about.)

+                return false;
+            }
             SkDEBUGCODE(haveBuffer = true;)
         } break;
     }
     return true;    // success
 }
 
 bool SkPictureData::parseBufferTag(SkReadBuffer& buffer,
                                    uint32_t tag, uint32_t size) {
     switch (tag) {
         case SK_PICT_BITMAP_BUFFER_TAG: {
             const int count = SkToInt(size);
             fBitmaps = SkTRefArray<SkBitmap>::Create(size);
             for (int i = 0; i < count; ++i) {
                 SkBitmap* bm = &fBitmaps->writableAt(i);
-                buffer.readBitmap(bm);
-                bm->setImmutable();
+                if (buffer.readBitmap(bm)) {
+                    bm->setImmutable();
+                } else {
+                    return false;
+                }
             }
         } break;
         case SK_PICT_PAINT_BUFFER_TAG: {
             const int count = SkToInt(size);
             fPaints = SkTRefArray<SkPaint>::Create(size);
             for (int i = 0; i < count; ++i) {
                 buffer.readPaint(&fPaints->writableAt(i));
             }
         } break;
         case SK_PICT_PATH_BUFFER_TAG:
@@ skipping 143 matching lines @@
     }
 }
 
 bool SkPictureData::suitableForLayerOptimization() const {
     return fContentInfo.numLayers() > 0;
 }
 #endif
 ///////////////////////////////////////////////////////////////////////////////