From 1026ccf1d2de57ae6e7d2f30ea92c245942121d3 Mon Sep 17 00:00:00 2001 From: Mike Reed Date: Sun, 8 Jan 2017 14:35:29 -0500 Subject: [PATCH] make SkRBuffer always validate BUG=skia:6102 Change-Id: Ic9fb259b2e980d00e179340945c50492f1803a4f Reviewed-on: https://skia-review.googlesource.com/6736 Reviewed-by: Mike Reed Commit-Queue: Mike Reed --- src/core/SkBuffer.cpp | 112 +++++++++++------------------------------- src/core/SkBuffer.h | 77 ++++++----------------------- src/core/SkPath.cpp | 2 +- src/core/SkRegion.cpp | 6 +-- tests/StreamTest.cpp | 15 ++++++ 5 files changed, 62 insertions(+), 150 deletions(-) diff --git a/src/core/SkBuffer.cpp b/src/core/SkBuffer.cpp index df8dc69594..7fd4170c8f 100644 --- a/src/core/SkBuffer.cpp +++ b/src/core/SkBuffer.cpp @@ -9,56 +9,49 @@ #include -//////////////////////////////////////////////////////////////////////////////////////// +/////////////////////////////////////////////////////////////////////////////////////////////////// -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(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(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 diff --git a/src/core/SkBuffer.h b/src/core/SkBuffer.h index c466fb65ea..c9c1a8e686 100644 --- a/src/core/SkBuffer.h +++ b/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 diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp index 7f233f2da6..53678de40e 100644 --- a/src/core/SkPath.cpp +++ b/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)) { diff --git a/src/core/SkRegion.cpp b/src/core/SkRegion.cpp index b3b1831aed..1123cf06ee 100644 --- a/src/core/SkRegion.cpp +++ b/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) { diff --git a/tests/StreamTest.cpp b/tests/StreamTest.cpp index 4c1b3dfe98..ff23ca2341 100644 --- a/tests/StreamTest.cpp +++ b/tests/StreamTest.cpp @@ -428,3 +428,18 @@ DEF_TEST(StreamEmptyStreamMemoryBase, r) { std::unique_ptr 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()); +}