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 d444152..4383f65 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 a1aa35e..1df56a5 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 08097a9..13128aa 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 06d180f..86c976c 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: