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/src@12202 2bbb7eff-a529-9590-31e7-b0007b416f81
diff --git a/core/SkBuffer.cpp b/core/SkBuffer.cpp
index 32a8011..590b05b 100644
--- a/core/SkBuffer.cpp
+++ b/core/SkBuffer.cpp
@@ -34,11 +34,12 @@
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/core/SkBuffer.h b/core/SkBuffer.h
index 1a4c6c2..369d9c0 100644
--- a/core/SkBuffer.h
+++ b/core/SkBuffer.h
@@ -56,23 +56,31 @@
/** 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 @@
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/core/SkFlattenableBuffers.cpp b/core/SkFlattenableBuffers.cpp
index 54d18d1..9da4dd9 100644
--- a/core/SkFlattenableBuffers.cpp
+++ b/core/SkFlattenableBuffers.cpp
@@ -89,6 +89,10 @@
return this->readFlattenableT<SkXfermode>();
}
+bool SkFlattenableReadBuffer::validate(bool isValid) {
+ return true;
+}
+
///////////////////////////////////////////////////////////////////////////////
SkFlattenableWriteBuffer::SkFlattenableWriteBuffer() {
diff --git a/core/SkPath.cpp b/core/SkPath.cpp
index e6d606c..f772717 100644
--- a/core/SkPath.cpp
+++ b/core/SkPath.cpp
@@ -2120,8 +2120,9 @@
(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::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 @@
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/core/SkPathRef.cpp b/core/SkPathRef.cpp
index f811b24..3557002 100644
--- a/core/SkPathRef.cpp
+++ b/core/SkPathRef.cpp
@@ -112,7 +112,11 @@
#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 @@
}
#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/core/SkRegion.cpp b/core/SkRegion.cpp
index 0d9198f..baedf2a 100644
--- a/core/SkRegion.cpp
+++ b/core/SkRegion.cpp
@@ -1138,15 +1138,12 @@
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/core/SkValidatingReadBuffer.cpp b/core/SkValidatingReadBuffer.cpp
index 3084565..1be142d 100644
--- a/core/SkValidatingReadBuffer.cpp
+++ b/core/SkValidatingReadBuffer.cpp
@@ -20,12 +20,13 @@
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 @@
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 @@
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 @@
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/core/SkValidatingReadBuffer.h b/core/SkValidatingReadBuffer.h
index 4d1919b..2bcdc43 100644
--- a/core/SkValidatingReadBuffer.h
+++ b/core/SkValidatingReadBuffer.h
@@ -60,7 +60,7 @@
// 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/ports/SkFontConfigInterface_direct.cpp b/ports/SkFontConfigInterface_direct.cpp
index 2c1e418..f1ac734 100644
--- a/ports/SkFontConfigInterface_direct.cpp
+++ b/ports/SkFontConfigInterface_direct.cpp
@@ -42,15 +42,18 @@
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
