Checking structure sizes before reading them from memory to avoid overflowing the buffer's stream.
BUG=
R=reed@google.com
Review URL: https://codereview.chromium.org/41253002
git-svn-id: http://skia.googlecode.com/svn/trunk/src@12114 2bbb7eff-a529-9590-31e7-b0007b416f81
diff --git a/core/SkBuffer.cpp b/core/SkBuffer.cpp
index 915264d..32a8011 100644
--- a/core/SkBuffer.cpp
+++ b/core/SkBuffer.cpp
@@ -34,6 +34,13 @@
return n;
}
+void SkRBufferWithSizeCheck::read(void* buffer, size_t size) {
+ fError = fError || (fPos + size > fStop);
+ if (!fError && (size > 0)) {
+ readNoSizeCheck(buffer, size);
+ }
+}
+
void* SkWBuffer::skip(size_t size)
{
void* result = fPos;
diff --git a/core/SkBuffer.h b/core/SkBuffer.h
index 9633389..1a4c6c2 100644
--- a/core/SkBuffer.h
+++ b/core/SkBuffer.h
@@ -56,7 +56,7 @@
/** Read the specified number of bytes from the data pointer. If buffer is not
null, copy those bytes into buffer.
*/
- void read(void* buffer, size_t size) {
+ virtual void read(void* buffer, size_t size) {
if (size) {
this->readNoSizeCheck(buffer, size);
}
@@ -74,7 +74,7 @@
uint8_t readU8() { uint8_t x; read(&x, 1); return x; }
bool readBool() { return this->readU8() != 0; }
-private:
+protected:
void readNoSizeCheck(void* buffer, size_t size);
const char* fData;
@@ -82,6 +82,28 @@
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.
+ */
+ virtual void read(void* buffer, size_t size) SK_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;
+};
+
/** \class SkWBuffer
Light weight class for writing data to a memory block.
diff --git a/core/SkMatrix.cpp b/core/SkMatrix.cpp
index 5bcb35b..cd7bcea 100644
--- a/core/SkMatrix.cpp
+++ b/core/SkMatrix.cpp
@@ -1921,20 +1921,25 @@
///////////////////////////////////////////////////////////////////////////////
-uint32_t SkMatrix::writeToMemory(void* buffer) const {
+size_t SkMatrix::writeToMemory(void* buffer) const {
// TODO write less for simple matrices
+ static const size_t sizeInMemory = 9 * sizeof(SkScalar);
if (buffer) {
- memcpy(buffer, fMat, 9 * sizeof(SkScalar));
+ memcpy(buffer, fMat, sizeInMemory);
}
- return 9 * sizeof(SkScalar);
+ return sizeInMemory;
}
-uint32_t SkMatrix::readFromMemory(const void* buffer) {
+size_t SkMatrix::readFromMemory(const void* buffer, size_t length) {
+ static const size_t sizeInMemory = 9 * sizeof(SkScalar);
+ if (length < sizeInMemory) {
+ return 0;
+ }
if (buffer) {
- memcpy(fMat, buffer, 9 * sizeof(SkScalar));
+ memcpy(fMat, buffer, sizeInMemory);
this->setTypeMask(kUnknown_Mask);
}
- return 9 * sizeof(SkScalar);
+ return sizeInMemory;
}
#ifdef SK_DEVELOPER
diff --git a/core/SkPath.cpp b/core/SkPath.cpp
index 60cfe03..982d6ad 100644
--- a/core/SkPath.cpp
+++ b/core/SkPath.cpp
@@ -2066,7 +2066,7 @@
Format in compressed buffer: [ptCount, verbCount, pts[], verbs[]]
*/
-uint32_t SkPath::writeToMemory(void* storage) const {
+size_t SkPath::writeToMemory(void* storage) const {
SkDEBUGCODE(this->validate();)
if (NULL == storage) {
@@ -2090,11 +2090,11 @@
fPathRef->writeToBuffer(&buffer);
buffer.padToAlign4();
- return SkToU32(buffer.pos());
+ return buffer.pos();
}
-uint32_t SkPath::readFromMemory(const void* storage) {
- SkRBuffer buffer(storage);
+size_t SkPath::readFromMemory(const void* storage, size_t length) {
+ SkRBufferWithSizeCheck buffer(storage, length);
uint32_t packed = buffer.readS32();
fIsOval = (packed >> kIsOval_SerializationShift) & 1;
@@ -2108,14 +2108,14 @@
fPathRef.reset(SkPathRef::CreateFromBuffer(&buffer
#ifndef DELETE_THIS_CODE_WHEN_SKPS_ARE_REBUILT_AT_V14_AND_ALL_OTHER_INSTANCES_TOO
- , newFormat, packed)
+ , newFormat, packed
#endif
- );
+ ));
buffer.skipToAlign4();
SkDEBUGCODE(this->validate();)
- return SkToU32(buffer.pos());
+ return buffer.isValid() ? buffer.pos() : 0;
}
///////////////////////////////////////////////////////////////////////////////
diff --git a/core/SkPicturePlayback.cpp b/core/SkPicturePlayback.cpp
index f2d959d..5a016d4 100644
--- a/core/SkPicturePlayback.cpp
+++ b/core/SkPicturePlayback.cpp
@@ -997,7 +997,8 @@
case DRAW_RRECT: {
const SkPaint& paint = *getPaint(reader);
SkRRect rrect;
- canvas.drawRRect(*reader.readRRect(&rrect), paint);
+ reader.readRRect(&rrect);
+ canvas.drawRRect(rrect, paint);
} break;
case DRAW_SPRITE: {
const SkPaint* paint = getPaint(reader);
diff --git a/core/SkRRect.cpp b/core/SkRRect.cpp
index e3d11cb..bcbf37e 100644
--- a/core/SkRRect.cpp
+++ b/core/SkRRect.cpp
@@ -259,7 +259,7 @@
///////////////////////////////////////////////////////////////////////////////
-uint32_t SkRRect::writeToMemory(void* buffer) const {
+size_t SkRRect::writeToMemory(void* buffer) const {
SkASSERT(kSizeInMemory == sizeof(SkRect) + sizeof(fRadii));
memcpy(buffer, &fRect, sizeof(SkRect));
@@ -267,7 +267,11 @@
return kSizeInMemory;
}
-uint32_t SkRRect::readFromMemory(const void* buffer) {
+size_t SkRRect::readFromMemory(const void* buffer, size_t length) {
+ if (length < kSizeInMemory) {
+ return 0;
+ }
+
SkScalar storage[12];
SkASSERT(sizeof(storage) == kSizeInMemory);
diff --git a/core/SkRegion.cpp b/core/SkRegion.cpp
index 02994bf..468be67 100644
--- a/core/SkRegion.cpp
+++ b/core/SkRegion.cpp
@@ -1100,9 +1100,9 @@
#include "SkBuffer.h"
-uint32_t SkRegion::writeToMemory(void* storage) const {
+size_t SkRegion::writeToMemory(void* storage) const {
if (NULL == storage) {
- uint32_t size = sizeof(int32_t); // -1 (empty), 0 (rect), runCount
+ size_t size = sizeof(int32_t); // -1 (empty), 0 (rect), runCount
if (!this->isEmpty()) {
size += sizeof(fBounds);
if (this->isComplex()) {
@@ -1133,11 +1133,11 @@
return buffer.pos();
}
-uint32_t SkRegion::readFromMemory(const void* storage) {
- SkRBuffer buffer(storage);
- SkRegion tmp;
- int32_t count;
-
+size_t SkRegion::readFromMemory(const void* storage, size_t length) {
+ SkRBufferWithSizeCheck buffer(storage, length);
+ SkRegion tmp;
+ int32_t count;
+
count = buffer.readS32();
if (count >= 0) {
buffer.read(&tmp.fBounds, sizeof(tmp.fBounds));
@@ -1150,8 +1150,12 @@
buffer.read(tmp.fRunHead->writable_runs(), count * sizeof(RunType));
}
}
- this->swap(tmp);
- return buffer.pos();
+ size_t sizeRead = 0;
+ if (buffer.isValid()) {
+ this->swap(tmp);
+ sizeRead = buffer.pos();
+ }
+ return sizeRead;
}
///////////////////////////////////////////////////////////////////////////////
diff --git a/core/SkValidatingReadBuffer.cpp b/core/SkValidatingReadBuffer.cpp
index 9f094f9..384dd10 100644
--- a/core/SkValidatingReadBuffer.cpp
+++ b/core/SkValidatingReadBuffer.cpp
@@ -118,8 +118,8 @@
}
void SkValidatingReadBuffer::readMatrix(SkMatrix* matrix) {
- const size_t size = matrix->readFromMemory(fReader.peek());
- this->validate(SkAlign4(size) == size);
+ const size_t size = matrix->readFromMemory(fReader.peek(), fReader.available());
+ this->validate((SkAlign4(size) != size) || (0 == size));
if (!fError) {
(void)this->skip(size);
}
@@ -140,16 +140,16 @@
}
void SkValidatingReadBuffer::readRegion(SkRegion* region) {
- const size_t size = region->readFromMemory(fReader.peek());
- this->validate(SkAlign4(size) == size);
+ const size_t size = region->readFromMemory(fReader.peek(), fReader.available());
+ this->validate((SkAlign4(size) != size) || (0 == size));
if (!fError) {
(void)this->skip(size);
}
}
void SkValidatingReadBuffer::readPath(SkPath* path) {
- const size_t size = path->readFromMemory(fReader.peek());
- this->validate(SkAlign4(size) == size);
+ const size_t size = path->readFromMemory(fReader.peek(), fReader.available());
+ this->validate((SkAlign4(size) != size) || (0 == size));
if (!fError) {
(void)this->skip(size);
}
@@ -189,6 +189,8 @@
}
uint32_t SkValidatingReadBuffer::getArrayCount() {
+ const size_t inc = sizeof(uint32_t);
+ fError = fError || !IsPtrAlign4(fReader.peek()) || !fReader.isAvailable(inc);
return *(uint32_t*)fReader.peek();
}
