From 8f457e3230f1a4ce737f512ffbb5c919b8d02407 Mon Sep 17 00:00:00 2001 From: "commit-bot@chromium.org" Date: Fri, 8 Nov 2013 19:22:57 +0000 Subject: [PATCH] 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 --- include/core/SkFlattenableBuffers.h | 10 ++++++- src/core/SkBuffer.cpp | 3 +- src/core/SkBuffer.h | 28 ++++++++++++------- src/core/SkFlattenableBuffers.cpp | 4 +++ src/core/SkPath.cpp | 20 ++++++++++---- src/core/SkPathRef.cpp | 32 +++++++++++++++------- src/core/SkRegion.cpp | 9 ++---- src/core/SkValidatingReadBuffer.cpp | 9 +++--- src/core/SkValidatingReadBuffer.h | 2 +- src/ports/SkFontConfigInterface_direct.cpp | 17 +++++++----- tests/SerializationTest.cpp | 16 +++++------ 11 files changed, 96 insertions(+), 54 deletions(-) diff --git a/include/core/SkFlattenableBuffers.h b/include/core/SkFlattenableBuffers.h index 8a94bb1db2..575dec8917 100644 --- a/include/core/SkFlattenableBuffers.h +++ b/include/core/SkFlattenableBuffers.h @@ -144,7 +144,15 @@ public: return SkData::NewFromMalloc(buffer, len); } - virtual void validate(bool isValid) {} + /** This function validates that the isValid input parameter is true + * If isValidating() is false, then true is always returned + * If isValidating() is true, then true is returned until validate() is called with isValid + * set to false. When isValid is false, an error flag will be set internally and, from that + * point on, validate() will return false. The error flag cannot be unset. + * + * @param isValid result of a test that is expected to be true + */ + virtual bool validate(bool isValid); private: template T* readFlattenableT(); 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(); } +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 diff --git a/tests/SerializationTest.cpp b/tests/SerializationTest.cpp index f047ec1284..f49abe9106 100644 --- a/tests/SerializationTest.cpp +++ b/tests/SerializationTest.cpp @@ -109,18 +109,18 @@ static void TestObjectSerialization(T* testObj, skiatest::Reporter* reporter) { // Make sure this fails when it should (test with smaller size, but still multiple of 4) SkValidatingReadBuffer buffer(dataWritten, bytesWritten - 4); - const unsigned char* peekBefore = static_cast(buffer.skip(0)); - SerializationUtils::Read(buffer, testObj); - const unsigned char* peekAfter = static_cast(buffer.skip(0)); - // This should have failed, since the buffer is too small to read a matrix from it - REPORTER_ASSERT(reporter, peekBefore == peekAfter); + T obj; + SerializationUtils::Read(buffer, &obj); + REPORTER_ASSERT(reporter, !buffer.validate(true)); // Make sure this succeeds when it should SkValidatingReadBuffer buffer2(dataWritten, bytesWritten); - peekBefore = static_cast(buffer2.skip(0)); - SerializationUtils::Read(buffer2, testObj); - peekAfter = static_cast(buffer2.skip(0)); + const unsigned char* peekBefore = static_cast(buffer2.skip(0)); + T obj2; + SerializationUtils::Read(buffer2, &obj2); + const unsigned char* peekAfter = static_cast(buffer2.skip(0)); // This should have succeeded, since there are enough bytes to read this + REPORTER_ASSERT(reporter, buffer2.validate(true)); REPORTER_ASSERT(reporter, static_cast(peekAfter - peekBefore) == bytesWritten); TestAlignment(testObj, reporter);