detect truncated pict streams

Bug: skia:7565
Change-Id: I203797fb8d4ced8d3fcb13de71feb5f4487c9515
Reviewed-on: https://skia-review.googlesource.com/102661
Commit-Queue: Mike Reed <reed@google.com>
Reviewed-by: Mike Klein <mtklein@chromium.org>

3 files changed, 43 insertions, 39 deletions

diff --git a/src/core/SkPictureData.cpp b/src/core/SkPictureData.cpp
index 3024e5cfa3..2ca88ae841 100644
--- a/src/core/SkPictureData.cpp
+++ b/src/core/SkPictureData.cpp
@@ -433,9 +433,7 @@ bool SkPictureData::parseStreamTag(SkStream* stream,
             while (!buffer.eof() && buffer.isValid()) {
                 tag = buffer.readUInt();
                 size = buffer.readUInt();
-                if (!this->parseBufferTag(buffer, tag, size)) {
-                    return false;
-                }
+                this->parseBufferTag(buffer, tag, size);
             }
             if (!buffer.isValid()) {
                 return false;
@@ -491,30 +489,30 @@ bool new_array_from_buffer(SkReadBuffer& buffer, uint32_t inCount,
         delete[] * array;
         *array = nullptr;
         *outCount = 0;
-        return false;
+        return buffer.validate(false);
     }
     return true;
 }
 
-bool SkPictureData::parseBufferTag(SkReadBuffer& buffer, uint32_t tag, uint32_t size) {
+void SkPictureData::parseBufferTag(SkReadBuffer& buffer, uint32_t tag, uint32_t size) {
     switch (tag) {
         case SK_PICT_PAINT_BUFFER_TAG: {
             if (!buffer.validate(SkTFitsIn<int>(size))) {
-                return false;
+                return;
             }
             const int count = SkToInt(size);
             fPaints.reset(count);
             for (int i = 0; i < count; ++i) {
                 if (!buffer.readPaint(&fPaints[i])) {
-                    return false;
+                    return;
                 }
             }
         } break;
         case SK_PICT_PATH_BUFFER_TAG:
             if (size > 0) {
                 const int count = buffer.readInt();
-                if (count < 0) {
-                    return false;
+                if (!buffer.validate(count >= 0)) {
+                    return;
                 }
                 fPaths.reset(count);
                 for (int i = 0; i < count; i++) {
@@ -522,49 +520,38 @@ bool SkPictureData::parseBufferTag(SkReadBuffer& buffer, uint32_t tag, uint32_t
                 }
             } break;
         case SK_PICT_TEXTBLOB_BUFFER_TAG:
-            if (!new_array_from_buffer(buffer, size, &fTextBlobRefs, &fTextBlobCount,
-                                       SkTextBlob::MakeFromBuffer)) {
-                return false;
-            }
+            new_array_from_buffer(buffer, size, &fTextBlobRefs, &fTextBlobCount,
+                                  SkTextBlob::MakeFromBuffer);
             break;
         case SK_PICT_VERTICES_BUFFER_TAG:
-            if (!new_array_from_buffer(buffer, size, &fVerticesRefs, &fVerticesCount,
-                                       create_vertices_from_buffer)) {
-                return false;
-            }
+            new_array_from_buffer(buffer, size, &fVerticesRefs, &fVerticesCount,
+                                  create_vertices_from_buffer);
             break;
         case SK_PICT_IMAGE_BUFFER_TAG:
-            if (!new_array_from_buffer(buffer, size, &fImageRefs, &fImageCount,
-                                       create_image_from_buffer)) {
-                return false;
-            }
+            new_array_from_buffer(buffer, size, &fImageRefs, &fImageCount,
+                                  create_image_from_buffer);
             break;
         case SK_PICT_READER_TAG: {
             auto data(SkData::MakeUninitialized(size));
             if (!buffer.readByteArray(data->writable_data(), size) ||
                 !buffer.validate(nullptr == fOpData)) {
-                return false;
+                return;
             }
             SkASSERT(nullptr == fOpData);
             fOpData = std::move(data);
         } break;
         case SK_PICT_PICTURE_TAG:
-            if (!new_array_from_buffer(buffer, size, &fPictureRefs, &fPictureCount,
-                                       SkPicture::MakeFromBuffer)) {
-                return false;
-            }
+            new_array_from_buffer(buffer, size, &fPictureRefs, &fPictureCount,
+                                  SkPicture::MakeFromBuffer);
             break;
         case SK_PICT_DRAWABLE_TAG:
-            if (!new_array_from_buffer(buffer, size, (const SkDrawable***)&fDrawableRefs,
-                                       &fDrawableCount, create_drawable_from_buffer)) {
-                return false;
-            }
+            new_array_from_buffer(buffer, size, (const SkDrawable***)&fDrawableRefs,
+                                  &fDrawableCount, create_drawable_from_buffer);
             break;
         default:
-            // The tag was invalid.
-            return false;
+            buffer.validate(false); // The tag was invalid.
+            break;
     }
-    return true;    // success
 }
 
 SkPictureData* SkPictureData::CreateFromStream(SkStream* stream,
@@ -611,16 +598,19 @@ bool SkPictureData::parseStream(SkStream* stream,
 }
 
 bool SkPictureData::parseBuffer(SkReadBuffer& buffer) {
-    for (;;) {
+    while (buffer.isValid()) {
         uint32_t tag = buffer.readUInt();
         if (SK_PICT_EOF_TAG == tag) {
             break;
         }
+        this->parseBufferTag(buffer, tag, buffer.readUInt());
+    }
 
-        uint32_t size = buffer.readUInt();
-        if (!this->parseBufferTag(buffer, tag, size)) {
-            return false; // we're invalid
-        }
+    // Check that we encountered required tags
+    if (!buffer.validate(this->opData())) {
+        // If we didn't build any opData, we are invalid. Even an EmptyPicture allocates the
+        // SkData for the ops (though its length may be zero).
+        return false;
     }
     return true;
 }
diff --git a/src/core/SkPictureData.h b/src/core/SkPictureData.h
index 0927f7727a..2be1da81de 100644
--- a/src/core/SkPictureData.h
+++ b/src/core/SkPictureData.h
@@ -140,7 +140,7 @@ private:
     // Does not affect ownership of SkStream.
     bool parseStreamTag(SkStream*, uint32_t tag, uint32_t size,
                         const SkDeserialProcs&, SkTypefacePlayback*);
-    bool parseBufferTag(SkReadBuffer&, uint32_t tag, uint32_t size);
+    void parseBufferTag(SkReadBuffer&, uint32_t tag, uint32_t size);
     void flattenToBuffer(SkWriteBuffer&) const;
 
     SkTArray<SkPaint>  fPaints;
diff --git a/tests/PictureTest.cpp b/tests/PictureTest.cpp
index 7acce49943..425b25a153 100644
--- a/tests/PictureTest.cpp
+++ b/tests/PictureTest.cpp
@@ -853,3 +853,17 @@ DEF_TEST(Placeholder, r) {
     sk_sp<SkPicture> pic = recorder.finishRecordingAsPicture();
     REPORTER_ASSERT(r, pic->approximateOpCount() == 2);
 }
+
+DEF_TEST(Picture_empty_serial, reporter) {
+    SkPictureRecorder rec;
+    (void)rec.beginRecording(10, 10);
+    auto pic = rec.finishRecordingAsPicture();
+    REPORTER_ASSERT(reporter, pic);
+
+    auto data = pic->serialize();
+    REPORTER_ASSERT(reporter, data);
+
+    auto pic2 = SkPicture::MakeFromData(data->data(), data->size());
+    REPORTER_ASSERT(reporter, pic2);
+}
+