Allow creating a picture from skp to fail.

Replace the current constructor for creating an
SkPicturePlayback to a factory. In the factory,
check for incorrect data that would result in an invalid
playback. If the playback is invalid, return NULL, and
return NULL from SkPicture's factory as well.

Update SkTimedPicture(Playback) as well.

BUG=skia:1672
R=caryclark@google.com

Review URL: https://codereview.chromium.org/24826002

git-svn-id: http://skia.googlecode.com/svn/trunk@11554 2bbb7eff-a529-9590-31e7-b0007b416f81

diff --git a/debugger/QT/SkDebuggerGUI.cpp b/debugger/QT/SkDebuggerGUI.cpp
index d444152eabe9007d3849a0249422d53df886accb..4383f65771d0491caf4d3e59cdb8e3dcd430e70f 100644 (file)
--- a/debugger/QT/SkDebuggerGUI.cpp
+++ b/debugger/QT/SkDebuggerGUI.cpp
@@ -147,10 +147,20 @@ void SkDebuggerGUI::showDeletes() {
 // offsets to individual commands.
 class SkTimedPicturePlayback : public SkPicturePlayback {
 public:
-    SkTimedPicturePlayback(SkStream* stream, const SkPictInfo& info,
-                           SkPicture::InstallPixelRefProc proc,
-                           const SkTDArray<bool>& deletedCommands)
-        : INHERITED(stream, info, proc)
+    static SkTimedPicturePlayback* CreateFromStream(SkStream* stream, const SkPictInfo& info,
+                                                    SkPicture::InstallPixelRefProc proc,
+                                                    const SkTDArray<bool>& deletedCommands) {
+        // Mimics SkPicturePlayback::CreateFromStream
+        SkAutoTDelete<SkTimedPicturePlayback> playback(SkNEW_ARGS(SkTimedPicturePlayback,
+                                                                  (deletedCommands)));
+        if (!playback->parseStream(stream, info, proc)) {
+            return NULL; // we're invalid
+        }
+        return playback.detach();
+    }
+
+    SkTimedPicturePlayback(const SkTDArray<bool>& deletedCommands)
+        : INHERITED()
         , fSkipCommands(deletedCommands)
         , fTot(0.0)
         , fCurCommand(0) {
@@ -232,6 +242,14 @@ protected:
 #endif
 
 private:
+    // SkPicturePlayback::parseStream is protected, so it can be
+    // called here, but not by our static factory function. This
+    // allows the factory function to call it.
+    bool parseStream(SkStream* stream, const SkPictInfo& info,
+                     SkPicture::InstallPixelRefProc proc) {
+        return this->INHERITED::parseStream(stream, info, proc);
+    }
+
     typedef SkPicturePlayback INHERITED;
 };
 
@@ -249,8 +267,11 @@ public:
         SkTimedPicturePlayback* playback;
         // Check to see if there is a playback to recreate.
         if (stream->readBool()) {
-            playback = SkNEW_ARGS(SkTimedPicturePlayback,
-                                  (stream, info, proc, deletedCommands));
+            playback = SkTimedPicturePlayback::CreateFromStream(stream, info, proc,
+                                                                deletedCommands);
+            if (NULL == playback) {
+                return NULL;
+            }
         } else {
             playback = NULL;
         }

diff --git a/src/core/SkPicture.cpp b/src/core/SkPicture.cpp
index a1aa35e61e09f1447e65f70271d9c49cd3db46d6..1df56a52d784bbd85ca1dc58d2651b021bb7d78e 100644 (file)
--- a/src/core/SkPicture.cpp
+++ b/src/core/SkPicture.cpp
@@ -310,7 +310,10 @@ SkPicture* SkPicture::CreateFromStream(SkStream* stream, InstallPixelRefProc pro
     SkPicturePlayback* playback;
     // Check to see if there is a playback to recreate.
     if (stream->readBool()) {
-        playback = SkNEW_ARGS(SkPicturePlayback, (stream, info, proc));
+        playback = SkPicturePlayback::CreateFromStream(stream, info, proc);
+        if (NULL == playback) {
+            return NULL;
+        }
     } else {
         playback = NULL;
     }

diff --git a/src/core/SkPicturePlayback.cpp b/src/core/SkPicturePlayback.cpp
index 08097a988ee06f466809b495faf74ebec4aa34fa..13128aac74c0ba96871edb0e39b92530f537213b 100644 (file)
--- a/src/core/SkPicturePlayback.cpp
+++ b/src/core/SkPicturePlayback.cpp
@@ -473,7 +473,7 @@ static uint32_t pictInfoFlagsToReadBufferFlags(uint32_t pictInfoFlags) {
     return rbMask;
 }
 
-void SkPicturePlayback::parseStreamTag(SkStream* stream, const SkPictInfo& info, uint32_t tag,
+bool SkPicturePlayback::parseStreamTag(SkStream* stream, const SkPictInfo& info, uint32_t tag,
                                        size_t size, SkPicture::InstallPixelRefProc proc) {
     /*
      *  By the time we encounter BUFFER_SIZE_TAG, we need to have already seen
@@ -488,19 +488,23 @@ void SkPicturePlayback::parseStreamTag(SkStream* stream, const SkPictInfo& info,
 
     switch (tag) {
         case PICT_READER_TAG: {
-            void* storage = sk_malloc_throw(size);
-            stream->read(storage, size);
+            SkAutoMalloc storage(size);
+            if (stream->read(storage.get(), size) != size) {
+                return false;
+            }
             SkASSERT(NULL == fOpData);
-            fOpData = SkData::NewFromMalloc(storage, size);
+            fOpData = SkData::NewFromMalloc(storage.detach(), size);
         } break;
         case PICT_FACTORY_TAG: {
             SkASSERT(!haveBuffer);
             fFactoryPlayback = SkNEW_ARGS(SkFactoryPlayback, (size));
             for (size_t i = 0; i < size; i++) {
                 SkString str;
-                int len = stream->readPackedUInt();
+                const size_t len = stream->readPackedUInt();
                 str.resize(len);
-                stream->read(str.writable_str(), len);
+                if (stream->read(str.writable_str(), len) != len) {
+                    return false;
+                }
                 fFactoryPlayback->base()[i] = SkFlattenable::NameToFactory(str.c_str());
             }
         } break;
@@ -520,19 +524,31 @@ void SkPicturePlayback::parseStreamTag(SkStream* stream, const SkPictInfo& info,
         case PICT_PICTURE_TAG: {
             fPictureCount = size;
             fPictureRefs = SkNEW_ARRAY(SkPicture*, fPictureCount);
-            for (int i = 0; i < fPictureCount; i++) {
+            bool success = true;
+            int i = 0;
+            for ( ; i < fPictureCount; i++) {
                 fPictureRefs[i] = SkPicture::CreateFromStream(stream, proc);
-                // CreateFromStream can only fail if PICTURE_VERSION does not match
-                // (which should never happen from here, since a sub picture will
-                // have the same PICTURE_VERSION as its parent) or if stream->read
-                // returns 0. In the latter case, we have a bug when writing the
-                // picture to begin with, which will be alerted to here.
-                SkASSERT(fPictureRefs[i] != NULL);
+                if (NULL == fPictureRefs[i]) {
+                    success = false;
+                    break;
+                }
+            }
+            if (!success) {
+                // Delete all of the pictures that were already created (up to but excluding i):
+                for (int j = 0; j < i; j++) {
+                    fPictureRefs[j]->unref();
+                }
+                // Delete the array
+                SkDELETE_ARRAY(fPictureRefs);
+                fPictureCount = 0;
+                return false;
             }
         } break;
         case PICT_BUFFER_SIZE_TAG: {
             SkAutoMalloc storage(size);
-            stream->read(storage.get(), size);
+            if (stream->read(storage.get(), size) != size) {
+                return false;
+            }
 
             SkOrderedReadBuffer buffer(storage.get(), size);
             buffer.setFlags(pictInfoFlagsToReadBufferFlags(info.fFlags));
@@ -544,14 +560,17 @@ void SkPicturePlayback::parseStreamTag(SkStream* stream, const SkPictInfo& info,
             while (!buffer.eof()) {
                 tag = buffer.readUInt();
                 size = buffer.readUInt();
-                this->parseBufferTag(buffer, tag, size);
+                if (!this->parseBufferTag(buffer, tag, size)) {
+                    return false;
+                }
             }
             SkDEBUGCODE(haveBuffer = true;)
         } break;
     }
+    return true;    // success
 }
 
-void SkPicturePlayback::parseBufferTag(SkOrderedReadBuffer& buffer,
+bool SkPicturePlayback::parseBufferTag(SkOrderedReadBuffer& buffer,
                                        uint32_t tag, size_t size) {
     switch (tag) {
         case PICT_BITMAP_BUFFER_TAG: {
@@ -585,13 +604,26 @@ void SkPicturePlayback::parseBufferTag(SkOrderedReadBuffer& buffer,
                 buffer.readRegion(&fRegions->writableAt(i));
             }
         } break;
+        default:
+            // The tag was invalid.
+            return false;
     }
+    return true;    // success
 }
 
-SkPicturePlayback::SkPicturePlayback(SkStream* stream, const SkPictInfo& info,
-                                     SkPicture::InstallPixelRefProc proc) {
-    this->init();
+SkPicturePlayback* SkPicturePlayback::CreateFromStream(SkStream* stream,
+                                                       const SkPictInfo& info,
+                                                       SkPicture::InstallPixelRefProc proc) {
+    SkAutoTDelete<SkPicturePlayback> playback(SkNEW(SkPicturePlayback));
+
+    if (!playback->parseStream(stream, info, proc)) {
+        return NULL;
+    }
+    return playback.detach();
+}
 
+bool SkPicturePlayback::parseStream(SkStream* stream, const SkPictInfo& info,
+                                    SkPicture::InstallPixelRefProc proc) {
     for (;;) {
         uint32_t tag = stream->readU32();
         if (PICT_EOF_TAG == tag) {
@@ -599,8 +631,11 @@ SkPicturePlayback::SkPicturePlayback(SkStream* stream, const SkPictInfo& info,
         }
 
         uint32_t size = stream->readU32();
-        this->parseStreamTag(stream, info, tag, size, proc);
+        if (!this->parseStreamTag(stream, info, tag, size, proc)) {
+            return false; // we're invalid
+        }
     }
+    return true;
 }
 
 ///////////////////////////////////////////////////////////////////////////////

diff --git a/src/core/SkPicturePlayback.h b/src/core/SkPicturePlayback.h
index 06d180fb4400ef14d1b73fcdc57347b83d46b158..86c976c37affcbd358fea929dc06570c1ef48b8d 100644 (file)
--- a/src/core/SkPicturePlayback.h
+++ b/src/core/SkPicturePlayback.h
@@ -62,7 +62,8 @@ public:
     SkPicturePlayback();
     SkPicturePlayback(const SkPicturePlayback& src, SkPictCopyInfo* deepCopyInfo = NULL);
     explicit SkPicturePlayback(const SkPictureRecord& record, bool deepCopy = false);
-    SkPicturePlayback(SkStream*, const SkPictInfo&, SkPicture::InstallPixelRefProc);
+    static SkPicturePlayback* CreateFromStream(SkStream*, const SkPictInfo&,
+                                               SkPicture::InstallPixelRefProc);
 
     virtual ~SkPicturePlayback();
 
@@ -79,6 +80,8 @@ public:
 #endif
 
 protected:
+    bool parseStream(SkStream*, const SkPictInfo&,
+                     SkPicture::InstallPixelRefProc);
 #ifdef SK_DEVELOPER
     virtual bool preDraw(int opIndex, int type);
     virtual void postDraw(int opIndex);
@@ -191,9 +194,9 @@ public:
 #endif
 
 private:    // these help us with reading/writing
-    void parseStreamTag(SkStream*, const SkPictInfo&, uint32_t tag, size_t size,
+    bool parseStreamTag(SkStream*, const SkPictInfo&, uint32_t tag, size_t size,
                         SkPicture::InstallPixelRefProc);
-    void parseBufferTag(SkOrderedReadBuffer&, uint32_t tag, size_t size);
+    bool parseBufferTag(SkOrderedReadBuffer&, uint32_t tag, size_t size);
     void flattenToBuffer(SkOrderedWriteBuffer&) const;
 
 private: