Adding error checks to SkRBuffer

BUG=
R=robertphillips@google.com, bsalomon@google.com, reed@google.com

Author: sugoi@chromium.org

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

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

9 files changed, 79 insertions, 45 deletions

diff --git a/src/core/SkBuffer.cpp b/src/core/SkBuffer.cpp
index 32a8011ac7..590b05b859 100644
--- a/src/core/SkBuffer.cpp
+++ b/src/core/SkBuffer.cpp
@@ -34,11 +34,12 @@ size_t SkRBuffer::skipToAlign4()
     return n;
 }
 
-void SkRBufferWithSizeCheck::read(void* buffer, size_t size) {
+bool SkRBufferWithSizeCheck::read(void* buffer, size_t size) {
     fError = fError || (fPos + size > fStop);
     if (!fError && (size > 0)) {
         readNoSizeCheck(buffer, size);
     }
+    return !fError;
 }
 
 void* SkWBuffer::skip(size_t size)
diff --git a/src/core/SkBuffer.h b/src/core/SkBuffer.h
index 1a4c6c281c..369d9c02ac 100644
--- a/src/core/SkBuffer.h
+++ b/src/core/SkBuffer.h
@@ -56,23 +56,31 @@ public:
     /** Read the specified number of bytes from the data pointer. If buffer is not
         null, copy those bytes into buffer.
     */
-    virtual void read(void* buffer, size_t size) {
+    virtual bool read(void* buffer, size_t size) {
         if (size) {
             this->readNoSizeCheck(buffer, size);
         }
+        return true;
     }
 
     const void* skip(size_t size); // return start of skipped data
     size_t  skipToAlign4();
 
-    void*       readPtr() { void* ptr; read(&ptr, sizeof(ptr)); return ptr; }
-    SkScalar    readScalar() { SkScalar x; read(&x, 4); return x; }
-    uint32_t    readU32() { uint32_t x; read(&x, 4); return x; }
-    int32_t     readS32() { int32_t x; read(&x, 4); return x; }
-    uint16_t    readU16() { uint16_t x; read(&x, 2); return x; }
-    int16_t     readS16() { int16_t x; read(&x, 2); return x; }
-    uint8_t     readU8() { uint8_t x; read(&x, 1); return x; }
-    bool        readBool() { return this->readU8() != 0; }
+    bool readPtr(void** ptr) { return read(ptr, sizeof(void*)); }
+    bool readScalar(SkScalar* x) { return read(x, 4); }
+    bool readU32(uint32_t* x) { return read(x, 4); }
+    bool readS32(int32_t* x) { return read(x, 4); }
+    bool readU16(uint16_t* x) { return read(x, 2); }
+    bool readS16(int16_t* x) { return read(x, 2); }
+    bool readU8(uint8_t* x) { return read(x, 1); }
+    bool readBool(bool* x) {
+        uint8_t u8;
+        if (this->readU8(&u8)) {
+            *x = (u8 != 0);
+            return true;
+        }
+        return false;
+    }
 
 protected:
     void    readNoSizeCheck(void* buffer, size_t size);
@@ -95,7 +103,7 @@ public:
         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;
+    virtual bool read(void* buffer, size_t size) SK_OVERRIDE;
 
     /** Returns whether or not a read operation attempted to read past the end of the data.
     */
diff --git a/src/core/SkFlattenableBuffers.cpp b/src/core/SkFlattenableBuffers.cpp
index 54d18d1c2f..9da4dd9ee4 100644
--- a/src/core/SkFlattenableBuffers.cpp
+++ b/src/core/SkFlattenableBuffers.cpp
@@ -89,6 +89,10 @@ SkXfermode* SkFlattenableReadBuffer::readXfermode() {
     return this->readFlattenableT<SkXfermode>();
 }
 
+bool SkFlattenableReadBuffer::validate(bool isValid) {
+    return true;
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 
 SkFlattenableWriteBuffer::SkFlattenableWriteBuffer() {
diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp
index e6d606c1b1..f772717e2f 100644
--- a/src/core/SkPath.cpp
+++ b/src/core/SkPath.cpp
@@ -2120,8 +2120,9 @@ size_t SkPath::writeToMemory(void* storage) const {
                      (fSegmentMask << kSegmentMask_SerializationShift) |
                      (fDirection << kDirection_SerializationShift)
 #ifndef DELETE_THIS_CODE_WHEN_SKPS_ARE_REBUILT_AT_V14_AND_ALL_OTHER_INSTANCES_TOO
-                     | (0x1 << kNewFormat_SerializationShift);
+                     | (0x1 << kNewFormat_SerializationShift)
 #endif
+                     ;
 
     buffer.write32(packed);
 
@@ -2134,7 +2135,11 @@ size_t SkPath::writeToMemory(void* storage) const {
 size_t SkPath::readFromMemory(const void* storage, size_t length) {
     SkRBufferWithSizeCheck buffer(storage, length);
 
-    uint32_t packed = buffer.readS32();
+    int32_t packed;
+    if (!buffer.readS32(&packed)) {
+        return 0;
+    }
+
     fIsOval = (packed >> kIsOval_SerializationShift) & 1;
     fConvexity = (packed >> kConvexity_SerializationShift) & 0xFF;
     fFillType = (packed >> kFillType_SerializationShift) & 0xFF;
@@ -2144,18 +2149,21 @@ size_t SkPath::readFromMemory(const void* storage, size_t length) {
     bool newFormat = (packed >> kNewFormat_SerializationShift) & 1;
 #endif
 
-    fPathRef.reset(SkPathRef::CreateFromBuffer(&buffer
+    SkPathRef* pathRef = SkPathRef::CreateFromBuffer(&buffer
 #ifndef DELETE_THIS_CODE_WHEN_SKPS_ARE_REBUILT_AT_V14_AND_ALL_OTHER_INSTANCES_TOO
         , newFormat, packed
 #endif
-        ));
-
-    buffer.skipToAlign4();
+        );
 
     size_t sizeRead = 0;
     if (buffer.isValid()) {
+        fPathRef.reset(pathRef);
         SkDEBUGCODE(this->validate();)
+        buffer.skipToAlign4();
         sizeRead = buffer.pos();
+    } else if (NULL != pathRef) {
+        // If the buffer is not valid, pathRef should be NULL
+        sk_throw();
     }
     return sizeRead;
 }
diff --git a/src/core/SkPathRef.cpp b/src/core/SkPathRef.cpp
index f811b245ec..355700265c 100644
--- a/src/core/SkPathRef.cpp
+++ b/src/core/SkPathRef.cpp
@@ -112,7 +112,11 @@ SkPathRef* SkPathRef::CreateFromBuffer(SkRBuffer* buffer
 #ifndef DELETE_THIS_CODE_WHEN_SKPS_ARE_REBUILT_AT_V14_AND_ALL_OTHER_INSTANCES_TOO
     if (newFormat) {
 #endif
-        int32_t packed = buffer->readU32();
+        int32_t packed;
+        if (!buffer->readS32(&packed)) {
+            SkDELETE(ref);
+            return NULL;
+        }
 
         ref->fIsFinite = (packed >> kIsFinite_SerializationShift) & 1;
 #ifndef DELETE_THIS_CODE_WHEN_SKPS_ARE_REBUILT_AT_V14_AND_ALL_OTHER_INSTANCES_TOO
@@ -121,19 +125,27 @@ SkPathRef* SkPathRef::CreateFromBuffer(SkRBuffer* buffer
     }
 #endif
 
-    ref->fGenerationID = buffer->readU32();
-    int32_t verbCount = buffer->readS32();
-    int32_t pointCount = buffer->readS32();
-    int32_t conicCount = buffer->readS32();
-    ref->resetToSize(verbCount, pointCount, conicCount);
+    int32_t verbCount, pointCount, conicCount;
+    if (!buffer->readU32(&(ref->fGenerationID)) ||
+        !buffer->readS32(&verbCount) ||
+        !buffer->readS32(&pointCount) ||
+        !buffer->readS32(&conicCount)) {
+        SkDELETE(ref);
+        return NULL;
+    }
 
+    ref->resetToSize(verbCount, pointCount, conicCount);
     SkASSERT(verbCount == ref->countVerbs());
     SkASSERT(pointCount == ref->countPoints());
     SkASSERT(conicCount == ref->fConicWeights.count());
-    buffer->read(ref->verbsMemWritable(), verbCount * sizeof(uint8_t));
-    buffer->read(ref->fPoints, pointCount * sizeof(SkPoint));
-    buffer->read(ref->fConicWeights.begin(), conicCount * sizeof(SkScalar));
-    buffer->read(&ref->fBounds, sizeof(SkRect));
+
+    if (!buffer->read(ref->verbsMemWritable(), verbCount * sizeof(uint8_t)) ||
+        !buffer->read(ref->fPoints, pointCount * sizeof(SkPoint)) ||
+        !buffer->read(ref->fConicWeights.begin(), conicCount * sizeof(SkScalar)) ||
+        !buffer->read(&ref->fBounds, sizeof(SkRect))) {
+        SkDELETE(ref);
+        return NULL;
+    }
     ref->fBoundsIsDirty = false;
     return ref;
 }
diff --git a/src/core/SkRegion.cpp b/src/core/SkRegion.cpp
index 0d9198f9f7..baedf2aea8 100644
--- a/src/core/SkRegion.cpp
+++ b/src/core/SkRegion.cpp
@@ -1138,15 +1138,12 @@ size_t SkRegion::readFromMemory(const void* storage, size_t length) {
     SkRegion                tmp;
     int32_t                 count;
 
-    count = buffer.readS32();
-    if (count >= 0) {
-        buffer.read(&tmp.fBounds, sizeof(tmp.fBounds));
+    if (buffer.readS32(&count) && (count >= 0) && buffer.read(&tmp.fBounds, sizeof(tmp.fBounds))) {
         if (count == 0) {
             tmp.fRunHead = SkRegion_gRectRunHeadPtr;
         } else {
-            int32_t ySpanCount = buffer.readS32();
-            int32_t intervalCount = buffer.readS32();
-            if (buffer.isValid()) {
+            int32_t ySpanCount, intervalCount;
+            if (buffer.readS32(&ySpanCount) && buffer.readS32(&intervalCount)) {
                 tmp.allocateRuns(count, ySpanCount, intervalCount);
                 buffer.read(tmp.fRunHead->writable_runs(), count * sizeof(RunType));
             }
diff --git a/src/core/SkValidatingReadBuffer.cpp b/src/core/SkValidatingReadBuffer.cpp
index 3084565ffd..1be142d40f 100644
--- a/src/core/SkValidatingReadBuffer.cpp
+++ b/src/core/SkValidatingReadBuffer.cpp
@@ -20,12 +20,13 @@ SkValidatingReadBuffer::SkValidatingReadBuffer(const void* data, size_t size) :
 SkValidatingReadBuffer::~SkValidatingReadBuffer() {
 }
 
-void SkValidatingReadBuffer::validate(bool isValid) {
+bool SkValidatingReadBuffer::validate(bool isValid) {
     if (!fError && !isValid) {
         // When an error is found, send the read cursor to the end of the stream
         fReader.skip(fReader.available());
         fError = true;
     }
+    return !fError;
 }
 
 void SkValidatingReadBuffer::setMemory(const void* data, size_t size) {
@@ -121,7 +122,7 @@ void SkValidatingReadBuffer::readMatrix(SkMatrix* matrix) {
     size_t size = 0;
     if (!fError) {
         size = matrix->readFromMemory(fReader.peek(), fReader.available());
-        this->validate((SkAlign4(size) != size) || (0 == size));
+        this->validate((SkAlign4(size) == size) && (0 != size));
     }
     if (!fError) {
         (void)this->skip(size);
@@ -146,7 +147,7 @@ void SkValidatingReadBuffer::readRegion(SkRegion* region) {
     size_t size = 0;
     if (!fError) {
         size = region->readFromMemory(fReader.peek(), fReader.available());
-        this->validate((SkAlign4(size) != size) || (0 == size));
+        this->validate((SkAlign4(size) == size) && (0 != size));
     }
     if (!fError) {
         (void)this->skip(size);
@@ -157,7 +158,7 @@ void SkValidatingReadBuffer::readPath(SkPath* path) {
     size_t size = 0;
     if (!fError) {
         size = path->readFromMemory(fReader.peek(), fReader.available());
-        this->validate((SkAlign4(size) != size) || (0 == size));
+        this->validate((SkAlign4(size) == size) && (0 != size));
     }
     if (!fError) {
         (void)this->skip(size);
diff --git a/src/core/SkValidatingReadBuffer.h b/src/core/SkValidatingReadBuffer.h
index 4d1919b62c..2bcdc43e8b 100644
--- a/src/core/SkValidatingReadBuffer.h
+++ b/src/core/SkValidatingReadBuffer.h
@@ -60,7 +60,7 @@ public:
     // TODO: Implement this (securely) when needed
     virtual SkTypeface* readTypeface() SK_OVERRIDE;
 
-    virtual void validate(bool isValid) SK_OVERRIDE;
+    virtual bool validate(bool isValid) SK_OVERRIDE;
 
 private:
     bool readArray(void* value, size_t size, size_t elementSize);
diff --git a/src/ports/SkFontConfigInterface_direct.cpp b/src/ports/SkFontConfigInterface_direct.cpp
index 2c1e4188b9..f1ac7342dc 100644
--- a/src/ports/SkFontConfigInterface_direct.cpp
+++ b/src/ports/SkFontConfigInterface_direct.cpp
@@ -42,15 +42,18 @@ size_t SkFontConfigInterface::FontIdentity::readFromMemory(const void* addr,
                                                            size_t size) {
     SkRBuffer buffer(addr, size);
 
-    fID = buffer.readU32();
-    fTTCIndex = buffer.readU32();
-    size_t strLen = buffer.readU32();
-    int weight = buffer.readU32();
-    int width = buffer.readU32();
-    SkFontStyle::Slant slant = (SkFontStyle::Slant)buffer.readU8();
+    (void)buffer.readU32(&fID);
+    (void)buffer.readS32(&fTTCIndex);
+    uint32_t strLen, weight, width;
+    (void)buffer.readU32(&strLen);
+    (void)buffer.readU32(&weight);
+    (void)buffer.readU32(&width);
+    uint8_t u8;
+    (void)buffer.readU8(&u8);
+    SkFontStyle::Slant slant = (SkFontStyle::Slant)u8;
     fStyle = SkFontStyle(weight, width, slant);
     fString.resize(strLen);
-    buffer.read(fString.writable_str(), strLen);
+    (void)buffer.read(fString.writable_str(), strLen);
     buffer.skipToAlign4();
 
     return buffer.pos();    // the actual number of bytes read