detect bad bitmaps during deserialization

src/core/SkPictureData.cpp

Index: src/core/SkPictureData.cpp
diff --git a/src/core/SkPictureData.cpp b/src/core/SkPictureData.cpp
index 896c2e6bf324fcd5f17b9ed9c731fb7736086b6b..bc152829189b098c2eecd0014329bb7b3ce3ad60 100644
--- a/src/core/SkPictureData.cpp
+++ b/src/core/SkPictureData.cpp
@@ -395,6 +395,7 @@ bool SkPictureData::parseStreamTag(SkStream* stream,
                 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);
@@ -403,13 +404,16 @@ bool SkPictureData::parseStreamTag(SkStream* stream,
             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;
     }
@@ -424,8 +428,11 @@ bool SkPictureData::parseBufferTag(SkReadBuffer& buffer,
             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: {