Checking structure sizes before reading them from memory to avoid overflowing the buffer's stream.

include/core/SkReader32.h

Index: include/core/SkReader32.h
diff --git a/include/core/SkReader32.h b/include/core/SkReader32.h
index 7a8d22a80c166c82539f05fe9d83186269416762..c0aaf5d26846b2cc39e078601c85667e22e84abd 100644
--- a/include/core/SkReader32.h
+++ b/include/core/SkReader32.h
@@ -106,27 +106,20 @@ public:
     int32_t readS32() { return this->readInt(); }
     uint32_t readU32() { return this->readInt(); }
 
-    void readPath(SkPath* path) {

[sugoi1, 2013/10/30 18:07:03]

These 4 functions had essentially the same code, so I unified them in a single
template function.

-        size_t size = path->readFromMemory(this->peek());
-        SkASSERT(SkAlign4(size) == size);
-        (void)this->skip(size);
+    bool readPath(SkPath* path) {
+        return readObjectFromMemory(path);
     }
 
-    void readMatrix(SkMatrix* matrix) {
-        size_t size = matrix->readFromMemory(this->peek());
-        SkASSERT(SkAlign4(size) == size);
-        (void)this->skip(size);
+    bool readMatrix(SkMatrix* matrix) {
+        return readObjectFromMemory(matrix);
     }
 
-    SkRRect* readRRect(SkRRect* rrect) {
-        rrect->readFromMemory(this->skip(SkRRect::kSizeInMemory));
-        return rrect;
+    bool readRRect(SkRRect* rrect) {
+        return readObjectFromMemory(rrect);
     }
 
-    void readRegion(SkRegion* rgn) {
-        size_t size = rgn->readFromMemory(this->peek());
-        SkASSERT(SkAlign4(size) == size);
-        (void)this->skip(size);
+    bool readRegion(SkRegion* rgn) {
+        return readObjectFromMemory(rgn);
     }
 
     /**
@@ -143,6 +136,16 @@ public:
     size_t readIntoString(SkString* copy);
 
 private:
+    template <typename T> bool readObjectFromMemory(T* obj) {
+        size_t size = obj->readFromMemory(this->peek(), this->available());
+        // If readFromMemory() fails (which means that available() was too small), it returns 0
+        bool success = size > 0;
+        SkASSERT(SkAlign4(size) == size && success);

[reed1, 2013/10/30 18:10:21]

not sure we can keep the assert, if the call now returns a bool for
success/failure. Will make it very hard to write unit-tests...

[sugoi1, 2013/10/30 18:26:55]

Done.

+        // In case of failure, we want to skip to the end
+        (void)this->skip(success ? size : this->available());

[reed1, 2013/10/30 18:10:21]

does skip() return something that might also indicate failure? If so, perhaps we
should return that, instead of success.?

[sugoi1, 2013/10/30 18:26:55]

Hmmm... skip() returns the address of the pointer at the beginning of the
skipped area, that's not really helpful. Also, skipping 0 is legal, so that
doesn't really help either.

+        return success;
+    }
+
     // these are always 4-byte aligned
     const char* fCurr;  // current position within buffer
     const char* fStop;  // end of buffer