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

BUG=
R=reed@google.com, mtklein@google.com, senorblanco@chromium.org

Committed: https://code.google.com/p/skia/source/detail?r=12114

Committed: https://code.google.com/p/skia/source/detail?r=12119

Author: sugoi@chromium.org

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

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

8 files changed, 96 insertions, 36 deletions

diff --git a/src/core/SkBuffer.cpp b/src/core/SkBuffer.cpp
index 915264d957..32a8011ac7 100644
--- a/src/core/SkBuffer.cpp
+++ b/src/core/SkBuffer.cpp
@@ -34,6 +34,13 @@ size_t SkRBuffer::skipToAlign4()
     return n;
 }
 
+void SkRBufferWithSizeCheck::read(void* buffer, size_t size) {
+    fError = fError || (fPos + size > fStop);
+    if (!fError && (size > 0)) {
+        readNoSizeCheck(buffer, size);
+    }
+}
+
 void* SkWBuffer::skip(size_t size)
 {
     void* result = fPos;
diff --git a/src/core/SkBuffer.h b/src/core/SkBuffer.h
index 9633389907..1a4c6c281c 100644
--- a/src/core/SkBuffer.h
+++ b/src/core/SkBuffer.h
@@ -56,7 +56,7 @@ public:
     /** Read the specified number of bytes from the data pointer. If buffer is not
         null, copy those bytes into buffer.
     */
-    void read(void* buffer, size_t size) {
+    virtual void read(void* buffer, size_t size) {
         if (size) {
             this->readNoSizeCheck(buffer, size);
         }
@@ -74,7 +74,7 @@ public:
     uint8_t     readU8() { uint8_t x; read(&x, 1); return x; }
     bool        readBool() { return this->readU8() != 0; }
 
-private:
+protected:
     void    readNoSizeCheck(void* buffer, size_t size);
 
     const char* fData;
@@ -82,6 +82,28 @@ private:
     const char* fStop;
 };
 
+/** \class SkRBufferWithSizeCheck
+
+    Same as SkRBuffer, except that a size check is performed before the read operation and an
+    error is set if the read operation is attempting to read past the end of the data.
+*/
+class SkRBufferWithSizeCheck : public SkRBuffer {
+public:
+    SkRBufferWithSizeCheck(const void* data, size_t size) : SkRBuffer(data, size), fError(false) {}
+
+    /** Read the specified number of bytes from the data pointer. If buffer is not
+        null and the number of bytes to read does not overflow this object's data,
+        copy those bytes into buffer.
+    */
+    virtual void read(void* buffer, size_t size) SK_OVERRIDE;
+
+    /** Returns whether or not a read operation attempted to read past the end of the data.
+    */
+    bool isValid() const { return !fError; }
+private:
+    bool fError;
+};
+
 /** \class SkWBuffer
 
     Light weight class for writing data to a memory block.
diff --git a/src/core/SkMatrix.cpp b/src/core/SkMatrix.cpp
index 5bcb35b298..cd7bcea176 100644
--- a/src/core/SkMatrix.cpp
+++ b/src/core/SkMatrix.cpp
@@ -1921,20 +1921,25 @@ const SkMatrix& SkMatrix::InvalidMatrix() {
 
 ///////////////////////////////////////////////////////////////////////////////
 
-uint32_t SkMatrix::writeToMemory(void* buffer) const {
+size_t SkMatrix::writeToMemory(void* buffer) const {
     // TODO write less for simple matrices
+    static const size_t sizeInMemory = 9 * sizeof(SkScalar);
     if (buffer) {
-        memcpy(buffer, fMat, 9 * sizeof(SkScalar));
+        memcpy(buffer, fMat, sizeInMemory);
     }
-    return 9 * sizeof(SkScalar);
+    return sizeInMemory;
 }
 
-uint32_t SkMatrix::readFromMemory(const void* buffer) {
+size_t SkMatrix::readFromMemory(const void* buffer, size_t length) {
+    static const size_t sizeInMemory = 9 * sizeof(SkScalar);
+    if (length < sizeInMemory) {
+        return 0;
+    }
     if (buffer) {
-        memcpy(fMat, buffer, 9 * sizeof(SkScalar));
+        memcpy(fMat, buffer, sizeInMemory);
         this->setTypeMask(kUnknown_Mask);
     }
-    return 9 * sizeof(SkScalar);
+    return sizeInMemory;
 }
 
 #ifdef SK_DEVELOPER
diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp
index 60cfe0373c..c480624a16 100644
--- a/src/core/SkPath.cpp
+++ b/src/core/SkPath.cpp
@@ -2066,7 +2066,7 @@ SkPath::Verb SkPath::RawIter::next(SkPoint pts[4]) {
     Format in compressed buffer: [ptCount, verbCount, pts[], verbs[]]
 */
 
-uint32_t SkPath::writeToMemory(void* storage) const {
+size_t SkPath::writeToMemory(void* storage) const {
     SkDEBUGCODE(this->validate();)
 
     if (NULL == storage) {
@@ -2090,11 +2090,11 @@ uint32_t SkPath::writeToMemory(void* storage) const {
     fPathRef->writeToBuffer(&buffer);
 
     buffer.padToAlign4();
-    return SkToU32(buffer.pos());
+    return buffer.pos();
 }
 
-uint32_t SkPath::readFromMemory(const void* storage) {
-    SkRBuffer   buffer(storage);
+size_t SkPath::readFromMemory(const void* storage, size_t length) {
+    SkRBufferWithSizeCheck buffer(storage, length);
 
     uint32_t packed = buffer.readS32();
     fIsOval = (packed >> kIsOval_SerializationShift) & 1;
@@ -2108,14 +2108,18 @@ uint32_t SkPath::readFromMemory(const void* storage) {
 
     fPathRef.reset(SkPathRef::CreateFromBuffer(&buffer
 #ifndef DELETE_THIS_CODE_WHEN_SKPS_ARE_REBUILT_AT_V14_AND_ALL_OTHER_INSTANCES_TOO
-        , newFormat, packed)
+        , newFormat, packed
 #endif
-        );
+        ));
 
     buffer.skipToAlign4();
 
-    SkDEBUGCODE(this->validate();)
-    return SkToU32(buffer.pos());
+    size_t sizeRead = 0;
+    if (buffer.isValid()) {
+        SkDEBUGCODE(this->validate();)
+        sizeRead = buffer.pos();
+    }
+    return sizeRead;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
diff --git a/src/core/SkPicturePlayback.cpp b/src/core/SkPicturePlayback.cpp
index f2d959d3d6..5a016d48d6 100644
--- a/src/core/SkPicturePlayback.cpp
+++ b/src/core/SkPicturePlayback.cpp
@@ -997,7 +997,8 @@ void SkPicturePlayback::draw(SkCanvas& canvas, SkDrawPictureCallback* callback)
             case DRAW_RRECT: {
                 const SkPaint& paint = *getPaint(reader);
                 SkRRect rrect;
-                canvas.drawRRect(*reader.readRRect(&rrect), paint);
+                reader.readRRect(&rrect);
+                canvas.drawRRect(rrect, paint);
             } break;
             case DRAW_SPRITE: {
                 const SkPaint* paint = getPaint(reader);
diff --git a/src/core/SkRRect.cpp b/src/core/SkRRect.cpp
index e3d11cb01e..bcbf37ec59 100644
--- a/src/core/SkRRect.cpp
+++ b/src/core/SkRRect.cpp
@@ -259,7 +259,7 @@ void SkRRect::inset(SkScalar dx, SkScalar dy, SkRRect* dst) const {
 
 ///////////////////////////////////////////////////////////////////////////////
 
-uint32_t SkRRect::writeToMemory(void* buffer) const {
+size_t SkRRect::writeToMemory(void* buffer) const {
     SkASSERT(kSizeInMemory == sizeof(SkRect) + sizeof(fRadii));
 
     memcpy(buffer, &fRect, sizeof(SkRect));
@@ -267,7 +267,11 @@ uint32_t SkRRect::writeToMemory(void* buffer) const {
     return kSizeInMemory;
 }
 
-uint32_t SkRRect::readFromMemory(const void* buffer) {
+size_t SkRRect::readFromMemory(const void* buffer, size_t length) {
+    if (length < kSizeInMemory) {
+        return 0;
+    }
+
     SkScalar storage[12];
     SkASSERT(sizeof(storage) == kSizeInMemory);
 
diff --git a/src/core/SkRegion.cpp b/src/core/SkRegion.cpp
index 02994bffb0..59af1c2f7a 100644
--- a/src/core/SkRegion.cpp
+++ b/src/core/SkRegion.cpp
@@ -1100,9 +1100,9 @@ bool SkRegion::op(const SkRegion& rgna, const SkRegion& rgnb, Op op) {
 
 #include "SkBuffer.h"
 
-uint32_t SkRegion::writeToMemory(void* storage) const {
+size_t SkRegion::writeToMemory(void* storage) const {
     if (NULL == storage) {
-        uint32_t size = sizeof(int32_t); // -1 (empty), 0 (rect), runCount
+        size_t size = sizeof(int32_t); // -1 (empty), 0 (rect), runCount
         if (!this->isEmpty()) {
             size += sizeof(fBounds);
             if (this->isComplex()) {
@@ -1133,11 +1133,11 @@ uint32_t SkRegion::writeToMemory(void* storage) const {
     return buffer.pos();
 }
 
-uint32_t SkRegion::readFromMemory(const void* storage) {
-    SkRBuffer   buffer(storage);
-    SkRegion    tmp;
-    int32_t     count;
-
+size_t SkRegion::readFromMemory(const void* storage, size_t length) {
+    SkRBufferWithSizeCheck  buffer(storage, length);
+    SkRegion                tmp;
+    int32_t                 count;
+    
     count = buffer.readS32();
     if (count >= 0) {
         buffer.read(&tmp.fBounds, sizeof(tmp.fBounds));
@@ -1146,12 +1146,18 @@ uint32_t SkRegion::readFromMemory(const void* storage) {
         } else {
             int32_t ySpanCount = buffer.readS32();
             int32_t intervalCount = buffer.readS32();
-            tmp.allocateRuns(count, ySpanCount, intervalCount);
-            buffer.read(tmp.fRunHead->writable_runs(), count * sizeof(RunType));
+            if (buffer.isValid()) {
+                tmp.allocateRuns(count, ySpanCount, intervalCount);
+                buffer.read(tmp.fRunHead->writable_runs(), count * sizeof(RunType));
+            }
         }
     }
-    this->swap(tmp);
-    return buffer.pos();
+    size_t sizeRead = 0;
+    if (buffer.isValid()) {
+        this->swap(tmp);
+        sizeRead = buffer.pos();
+    }
+    return sizeRead;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
diff --git a/src/core/SkValidatingReadBuffer.cpp b/src/core/SkValidatingReadBuffer.cpp
index 9f094f9617..3084565ffd 100644
--- a/src/core/SkValidatingReadBuffer.cpp
+++ b/src/core/SkValidatingReadBuffer.cpp
@@ -118,8 +118,11 @@ void SkValidatingReadBuffer::readPoint(SkPoint* point) {
 }
 
 void SkValidatingReadBuffer::readMatrix(SkMatrix* matrix) {
-    const size_t size = matrix->readFromMemory(fReader.peek());
-    this->validate(SkAlign4(size) == size);
+    size_t size = 0;
+    if (!fError) {
+        size = matrix->readFromMemory(fReader.peek(), fReader.available());
+        this->validate((SkAlign4(size) != size) || (0 == size));
+    }
     if (!fError) {
         (void)this->skip(size);
     }
@@ -140,16 +143,22 @@ void SkValidatingReadBuffer::readRect(SkRect* rect) {
 }
 
 void SkValidatingReadBuffer::readRegion(SkRegion* region) {
-    const size_t size = region->readFromMemory(fReader.peek());
-    this->validate(SkAlign4(size) == size);
+    size_t size = 0;
+    if (!fError) {
+        size = region->readFromMemory(fReader.peek(), fReader.available());
+        this->validate((SkAlign4(size) != size) || (0 == size));
+    }
     if (!fError) {
         (void)this->skip(size);
     }
 }
 
 void SkValidatingReadBuffer::readPath(SkPath* path) {
-    const size_t size = path->readFromMemory(fReader.peek());
-    this->validate(SkAlign4(size) == size);
+    size_t size = 0;
+    if (!fError) {
+        size = path->readFromMemory(fReader.peek(), fReader.available());
+        this->validate((SkAlign4(size) != size) || (0 == size));
+    }
     if (!fError) {
         (void)this->skip(size);
     }
@@ -189,6 +198,8 @@ bool SkValidatingReadBuffer::readScalarArray(SkScalar* values, size_t size) {
 }
 
 uint32_t SkValidatingReadBuffer::getArrayCount() {
+    const size_t inc = sizeof(uint32_t);
+    fError = fError || !IsPtrAlign4(fReader.peek()) || !fReader.isAvailable(inc);
     return *(uint32_t*)fReader.peek();
 }