make SkRBuffer always validate

BUG=skia:6102

Change-Id: Ic9fb259b2e980d00e179340945c50492f1803a4f
Reviewed-on: https://skia-review.googlesource.com/6736
Reviewed-by: Mike Reed <reed@google.com>
Commit-Queue: Mike Reed <reed@google.com>

src/core/SkBuffer.cpp

@@ -9,56 +9,49 @@
 
 #include <string.h>
 
-////////////////////////////////////////////////////////////////////////////////////////
+///////////////////////////////////////////////////////////////////////////////////////////////////
 
-void SkRBuffer::readNoSizeCheck(void* buffer, size_t size)
-{
-    SkASSERT((fData != 0 && fStop == 0) || fPos + size <= fStop);
-    if (buffer)
-        memcpy(buffer, fPos, size);
-    fPos += size;
-}
-
-const void* SkRBuffer::skip(size_t size)
-{
-    const void* result = fPos;
-    readNoSizeCheck(nullptr, size);
-    return result;
-}
-
-size_t SkRBuffer::skipToAlign4()
-{
-    size_t pos = this->pos();
-    size_t n = SkAlign4(pos) - pos;
-    fPos += n;
-    return n;
-}
-
-bool SkRBufferWithSizeCheck::read(void* buffer, size_t size) {
-    fError = fError || (size > static_cast<size_t>(fStop - fPos));
-    if (!fError && (size > 0)) {
-        readNoSizeCheck(buffer, size);
+bool SkRBuffer::read(void* buffer, size_t size) {
+    if (fValid && size <= this->available()) {
+        if (buffer) {
+            memcpy(buffer, fPos, size);
+        }
+        fPos += size;
+        return true;
+    } else {
+        fValid = false;
+        return false;
     }
-    return !fError;
 }
 
-void* SkWBuffer::skip(size_t size)
-{
+bool SkRBuffer::skipToAlign4() {
+    intptr_t pos = reinterpret_cast<intptr_t>(fPos);
+    size_t n = SkAlign4(pos) - pos;
+    if (fValid && n <= this->available()) {
+        fPos += n;
+        return true;
+    } else {
+        fValid = false;
+        return false;
+    }
+}
+    
+///////////////////////////////////////////////////////////////////////////////////////////////////
+
+void* SkWBuffer::skip(size_t size) {
     void* result = fPos;
     writeNoSizeCheck(nullptr, size);
     return fData == nullptr ? nullptr : result;
 }
 
-void SkWBuffer::writeNoSizeCheck(const void* buffer, size_t size)
-{
+void SkWBuffer::writeNoSizeCheck(const void* buffer, size_t size) {
     SkASSERT(fData == 0 || fStop == 0 || fPos + size <= fStop);
     if (fData && buffer)
         memcpy(fPos, buffer, size);
     fPos += size;
 }
 
-size_t SkWBuffer::padToAlign4()
-{
+size_t SkWBuffer::padToAlign4() {
     size_t pos = this->pos();
     size_t n = SkAlign4(pos) - pos;
 
@@ -85,53 +78,4 @@ size_t SkWBuffer::padToAlign4()
     #define AssertBuffer32(buffer)
 #endif
 
-void* sk_buffer_write_int32(void* buffer, int32_t value)
-{
-    AssertBuffer32(buffer);
-    *(int32_t*)buffer = value;
-    return (char*)buffer + sizeof(int32_t);
-}
-
-void* sk_buffer_write_int32(void* buffer, const int32_t values[], int count)
-{
-    AssertBuffer32(buffer);
-    SkASSERT(count >= 0);
-
-    memcpy((int32_t*)buffer, values, count * sizeof(int32_t));
-    return (char*)buffer + count * sizeof(int32_t);
-}
-
-const void* sk_buffer_read_int32(const void* buffer, int32_t* value)
-{
-    AssertBuffer32(buffer);
-    if (value)
-        *value = *(const int32_t*)buffer;
-    return (const char*)buffer + sizeof(int32_t);
-}
-
-const void* sk_buffer_read_int32(const void* buffer, int32_t values[], int count)
-{
-    AssertBuffer32(buffer);
-    SkASSERT(count >= 0);
-
-    if (values)
-        memcpy(values, (const int32_t*)buffer, count * sizeof(int32_t));
-    return (const char*)buffer + count * sizeof(int32_t);
-}
-
-void* sk_buffer_write_ptr(void* buffer, void* ptr)
-{
-    AssertBuffer32(buffer);
-    *(void**)buffer = ptr;
-    return (char*)buffer + sizeof(void*);
-}
-
-const void* sk_buffer_read_ptr(const void* buffer, void** ptr)
-{
-    AssertBuffer32(buffer);
-    if (ptr)
-        *ptr = *(void**)buffer;
-    return (const char*)buffer + sizeof(void*);
-}
-
 #endif

src/core/SkBuffer.h

@@ -22,14 +22,7 @@
 class SkRBuffer : SkNoncopyable {
 public:
     SkRBuffer() : fData(0), fPos(0), fStop(0) {}
-    /** Initialize RBuffer with a data pointer, but no specified length.
-        This signals the RBuffer to not perform range checks during reading.
-    */
-    SkRBuffer(const void* data) {
-        fData = (const char*)data;
-        fPos = (const char*)data;
-        fStop = 0;  // no bounds checking
-    }
+
     /** Initialize RBuffer with a data point and length.
     */
     SkRBuffer(const void* data, size_t size) {
@@ -39,79 +32,39 @@ public:
         fStop = (const char*)data + size;
     }
 
-    virtual ~SkRBuffer() { }
-
     /** Return the number of bytes that have been read from the beginning
         of the data pointer.
     */
-    size_t  pos() const { return fPos - fData; }
+    size_t pos() const { return fPos - fData; }
     /** Return the total size of the data pointer. Only defined if the length was
         specified in the constructor or in a call to reset().
     */
-    size_t  size() const { return fStop - fData; }
+    size_t size() const { return fStop - fData; }
     /** Return true if the buffer has read to the end of the data pointer.
         Only defined if the length was specified in the constructor or in a call
         to reset(). Always returns true if the length was not specified.
     */
-    bool    eof() const { return fPos >= fStop; }
+    bool eof() const { return fPos >= fStop; }
+
+    size_t available() const { return fStop - fPos; }
+
+    bool isValid() const { return fValid; }
 
     /** Read the specified number of bytes from the data pointer. If buffer is not
         null, copy those bytes into buffer.
     */
-    virtual bool read(void* buffer, size_t size) {
-        if (size) {
-            this->readNoSizeCheck(buffer, size);
-        }
-        return true;
-    }
+    bool read(void* buffer, size_t size);
+    bool skipToAlign4();
 
-    const void* skip(size_t size); // return start of skipped data
-    size_t  skipToAlign4();
-
-    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);
+    bool readU8(uint8_t* x)   { return this->read(x, 1); }
+    bool readS32(int32_t* x)  { return this->read(x, 4); }
+    bool readU32(uint32_t* x) { return this->read(x, 4); }
 
+private:
     const char* fData;
     const char* fPos;
     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.
-    */
-    bool read(void* buffer, size_t size) 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;
+    bool        fValid = true;
 };
 
 /** \class SkWBuffer

src/core/SkPath.cpp

@@ -2054,7 +2054,7 @@ size_t SkPath::writeToMemory(void* storage) const {
 }
 
 size_t SkPath::readFromMemory(const void* storage, size_t length) {
-    SkRBufferWithSizeCheck buffer(storage, length);
+    SkRBuffer buffer(storage, length);
 
     int32_t packed;
     if (!buffer.readS32(&packed)) {

src/core/SkRegion.cpp

@@ -1128,9 +1128,9 @@ size_t SkRegion::writeToMemory(void* storage) const {
 }
 
 size_t SkRegion::readFromMemory(const void* storage, size_t length) {
-    SkRBufferWithSizeCheck  buffer(storage, length);
-    SkRegion                tmp;
-    int32_t                 count;
+    SkRBuffer   buffer(storage, length);
+    SkRegion    tmp;
+    int32_t     count;
 
     if (buffer.readS32(&count) && (count >= 0) && buffer.read(&tmp.fBounds, sizeof(tmp.fBounds))) {
         if (count == 0) {

tests/StreamTest.cpp

@@ -428,3 +428,18 @@ DEF_TEST(StreamEmptyStreamMemoryBase, r) {
     std::unique_ptr<SkStreamAsset> asset(tmp.detachAsStream());
     REPORTER_ASSERT(r, nullptr == asset->getMemoryBase());
 }
+
+#include "SkBuffer.h"
+
+DEF_TEST(RBuffer, reporter) {
+    int32_t value = 0;
+    SkRBuffer buffer(&value, 4);
+    REPORTER_ASSERT(reporter, buffer.isValid());
+
+    int32_t tmp;
+    REPORTER_ASSERT(reporter, buffer.read(&tmp, 4));
+    REPORTER_ASSERT(reporter, buffer.isValid());
+
+    REPORTER_ASSERT(reporter, !buffer.read(&tmp, 4));
+    REPORTER_ASSERT(reporter, !buffer.isValid());
+}