Adding size parameter to read array functions
authorcommit-bot@chromium.org <commit-bot@chromium.org@2bbb7eff-a529-9590-31e7-b0007b416f81>
Thu, 31 Oct 2013 18:37:50 +0000 (18:37 +0000)
committercommit-bot@chromium.org <commit-bot@chromium.org@2bbb7eff-a529-9590-31e7-b0007b416f81>
Thu, 31 Oct 2013 18:37:50 +0000 (18:37 +0000)
In some cases, the allocated array into which the data will be read is using getArrayCount() to allocate itself, which should be safe, but some cases use fixed length arrays or compute the array size before reading, which could overflow if the stream is compromised.

To prevent that from happening, I added a check that will verify that the number of bytes to read will not exceed the capacity of the input buffer argument passed to all the read...Array() functions.

I chose to use the byte array for this initial version, so that "size" represents the same value across all read...Array() functions, but I could also use the element count, if it is preferred.

Note : readPointArray and writePointArray are unused, so I could also remove them

BUG=
R=reed@google.com, mtklein@google.com, senorblanco@chromium.org

Author: sugoi@chromium.org

Review URL: https://codereview.chromium.org/37803002

git-svn-id: http://skia.googlecode.com/svn/trunk@12058 2bbb7eff-a529-9590-31e7-b0007b416f81

21 files changed:
gyp/tests.gyp
include/core/SkFlattenableBuffers.h
src/core/SkColorTable.cpp
src/core/SkFlattenableBuffers.cpp
src/core/SkMallocPixelRef.cpp
src/core/SkOrderedReadBuffer.cpp
src/core/SkOrderedReadBuffer.h
src/core/SkValidatingReadBuffer.cpp
src/core/SkValidatingReadBuffer.h
src/effects/SkBicubicImageFilter.cpp
src/effects/SkColorMatrixFilter.cpp
src/effects/SkDashPathEffect.cpp
src/effects/SkEmbossMaskFilter.cpp
src/effects/SkKernel33MaskFilter.cpp
src/effects/SkMatrixConvolutionImageFilter.cpp
src/effects/SkMergeImageFilter.cpp
src/effects/SkTableColorFilter.cpp
src/effects/SkTableMaskFilter.cpp
src/effects/gradients/SkGradientShader.cpp
src/images/SkImageRef.cpp
tests/SerializationTest.cpp [new file with mode: 0644]

index 6b286bb..ec5bca0 100644 (file)
         '../tests/RTreeTest.cpp',
         '../tests/SHA1Test.cpp',
         '../tests/ScalarTest.cpp',
+        '../tests/SerializationTest.cpp',
         '../tests/ShaderImageFilterTest.cpp',
         '../tests/ShaderOpacityTest.cpp',
         '../tests/Sk64Test.cpp',
index 6e44550..8a94bb1 100644 (file)
@@ -99,11 +99,24 @@ public:
     virtual void readPath(SkPath* path) = 0;
 
     // binary data and arrays
-    virtual uint32_t readByteArray(void* value) = 0;
-    virtual uint32_t readColorArray(SkColor* colors) = 0;
-    virtual uint32_t readIntArray(int32_t* values) = 0;
-    virtual uint32_t readPointArray(SkPoint* points) = 0;
-    virtual uint32_t readScalarArray(SkScalar* values) = 0;
+
+    /**
+      * In the following read.*Array(...) functions, the size parameter specifies the allocation
+      * size in number of elements (or in bytes, for void*) of the pointer parameter. If the
+      * pointer parameter's size does not match the size to be read, the pointer parameter's memory
+      * will then stay uninitialized, the cursor will be moved to the end of the stream and, in the
+      * case where isValidating() is true, an error flag will be set internally (see
+      * SkValidatingReadBuffer).
+      * If the sizes match, then "size" amount of memory will be read.
+      *
+      * @param size amount of memory expected to be read
+      * @return true if the size parameter matches the size to be read, false otherwise
+      */
+    virtual bool readByteArray(void* value, size_t size) = 0;
+    virtual bool readColorArray(SkColor* colors, size_t size) = 0;
+    virtual bool readIntArray(int32_t* values, size_t size) = 0;
+    virtual bool readPointArray(SkPoint* points, size_t size) = 0;
+    virtual bool readScalarArray(SkScalar* values, size_t size) = 0;
 
     /** This helper peeks into the buffer and reports back the length of the next array in
      *  the buffer but does not change the state of the buffer.
@@ -127,7 +140,7 @@ public:
     SkData* readByteArrayAsData() {
         size_t len = this->getArrayCount();
         void* buffer = sk_malloc_throw(len);
-        (void)this->readByteArray(buffer);
+        (void)this->readByteArray(buffer, len);
         return SkData::NewFromMalloc(buffer, len);
     }
 
index 242ea6b..38a46c5 100644 (file)
@@ -93,10 +93,10 @@ SkColorTable::SkColorTable(SkFlattenableReadBuffer& buffer) {
     fAlphaType = SkToU8(buffer.readUInt());
     fCount = buffer.getArrayCount();
     fColors = (SkPMColor*)sk_malloc_throw(fCount * sizeof(SkPMColor));
-    SkDEBUGCODE(const uint32_t countRead =) buffer.readColorArray(fColors);
+    SkDEBUGCODE(bool success =) buffer.readColorArray(fColors, fCount);
 #ifdef SK_DEBUG
     SkASSERT((unsigned)fCount <= 256);
-    SkASSERT(countRead == fCount);
+    SkASSERT(success);
 #endif
 }
 
index dbf66da..54d18d1 100644 (file)
@@ -37,7 +37,7 @@ SkFlattenableReadBuffer::~SkFlattenableReadBuffer() { }
 void* SkFlattenableReadBuffer::readFunctionPtr() {
     void* proc;
     SkASSERT(sizeof(void*) == this->getArrayCount());
-    this->readByteArray(&proc);
+    this->readByteArray(&proc, sizeof(void*));
     return proc;
 }
 
index 341ac9e..f229e9d 100644 (file)
@@ -54,7 +54,7 @@ SkMallocPixelRef::SkMallocPixelRef(SkFlattenableReadBuffer& buffer)
         : INHERITED(buffer, NULL) {
     fSize = buffer.getArrayCount();
     fStorage = sk_malloc_throw(fSize);
-    buffer.readByteArray(fStorage);
+    buffer.readByteArray(fStorage, fSize);
     if (buffer.readBool()) {
         fCTable = SkNEW_ARGS(SkColorTable, (buffer));
     } else {
index d9aa8bd..3184118 100644 (file)
@@ -137,38 +137,37 @@ void SkOrderedReadBuffer::readPath(SkPath* path) {
     fReader.readPath(path);
 }
 
-uint32_t SkOrderedReadBuffer::readByteArray(void* value) {
-    const uint32_t length = fReader.readU32();
-    memcpy(value, fReader.skip(SkAlign4(length)), length);
-    return length;
+bool SkOrderedReadBuffer::readArray(void* value, size_t size, size_t elementSize) {
+    const size_t count = this->getArrayCount();
+    if (count == size) {
+        (void)fReader.skip(sizeof(uint32_t)); // Skip array count
+        const size_t byteLength = count * elementSize;
+        memcpy(value, fReader.skip(SkAlign4(byteLength)), byteLength);
+        return true;
+    }
+    SkASSERT(false);
+    fReader.skip(fReader.available());
+    return false;
+}
+
+bool SkOrderedReadBuffer::readByteArray(void* value, size_t size) {
+    return readArray(static_cast<unsigned char*>(value), size, sizeof(unsigned char));
 }
 
-uint32_t SkOrderedReadBuffer::readColorArray(SkColor* colors) {
-    const uint32_t count = fReader.readU32();
-    const uint32_t byteLength = count * sizeof(SkColor);
-    memcpy(colors, fReader.skip(SkAlign4(byteLength)), byteLength);
-    return count;
+bool SkOrderedReadBuffer::readColorArray(SkColor* colors, size_t size) {
+    return readArray(colors, size, sizeof(SkColor));
 }
 
-uint32_t SkOrderedReadBuffer::readIntArray(int32_t* values) {
-    const uint32_t count = fReader.readU32();
-    const uint32_t byteLength = count * sizeof(int32_t);
-    memcpy(values, fReader.skip(SkAlign4(byteLength)), byteLength);
-    return count;
+bool SkOrderedReadBuffer::readIntArray(int32_t* values, size_t size) {
+    return readArray(values, size, sizeof(int32_t));
 }
 
-uint32_t SkOrderedReadBuffer::readPointArray(SkPoint* points) {
-    const uint32_t count = fReader.readU32();
-    const uint32_t byteLength = count * sizeof(SkPoint);
-    memcpy(points, fReader.skip(SkAlign4(byteLength)), byteLength);
-    return count;
+bool SkOrderedReadBuffer::readPointArray(SkPoint* points, size_t size) {
+    return readArray(points, size, sizeof(SkPoint));
 }
 
-uint32_t SkOrderedReadBuffer::readScalarArray(SkScalar* values) {
-    const uint32_t count = fReader.readU32();
-    const uint32_t byteLength = count * sizeof(SkScalar);
-    memcpy(values, fReader.skip(SkAlign4(byteLength)), byteLength);
-    return count;
+bool SkOrderedReadBuffer::readScalarArray(SkScalar* values, size_t size) {
+    return readArray(values, size, sizeof(SkScalar));
 }
 
 uint32_t SkOrderedReadBuffer::getArrayCount() {
index d864465..2c4f480 100644 (file)
@@ -61,11 +61,11 @@ public:
     virtual void readPath(SkPath* path) SK_OVERRIDE;
 
     // binary data and arrays
-    virtual uint32_t readByteArray(void* value) SK_OVERRIDE;
-    virtual uint32_t readColorArray(SkColor* colors) SK_OVERRIDE;
-    virtual uint32_t readIntArray(int32_t* values) SK_OVERRIDE;
-    virtual uint32_t readPointArray(SkPoint* points) SK_OVERRIDE;
-    virtual uint32_t readScalarArray(SkScalar* values) SK_OVERRIDE;
+    virtual bool readByteArray(void* value, size_t size) SK_OVERRIDE;
+    virtual bool readColorArray(SkColor* colors, size_t size) SK_OVERRIDE;
+    virtual bool readIntArray(int32_t* values, size_t size) SK_OVERRIDE;
+    virtual bool readPointArray(SkPoint* points, size_t size) SK_OVERRIDE;
+    virtual bool readScalarArray(SkScalar* values, size_t size) SK_OVERRIDE;
 
     // helpers to get info about arrays and binary data
     virtual uint32_t getArrayCount() SK_OVERRIDE;
@@ -113,6 +113,8 @@ public:
     }
 
 private:
+    bool readArray(void* value, size_t size, size_t elementSize);
+
     SkReader32 fReader;
     void* fMemoryPtr;
 
index 323273d..9f094f9 100644 (file)
@@ -20,8 +20,16 @@ SkValidatingReadBuffer::SkValidatingReadBuffer(const void* data, size_t size) :
 SkValidatingReadBuffer::~SkValidatingReadBuffer() {
 }
 
+void 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;
+    }
+}
+
 void SkValidatingReadBuffer::setMemory(const void* data, size_t size) {
-    fError = fError || !IsPtrAlign4(data) || (SkAlign4(size) != size);
+    this->validate(IsPtrAlign4(data) && (SkAlign4(size) == size));
     if (!fError) {
         fReader.setMemory(data, size);
     }
@@ -30,7 +38,7 @@ void SkValidatingReadBuffer::setMemory(const void* data, size_t size) {
 const void* SkValidatingReadBuffer::skip(size_t size) {
     size_t inc = SkAlign4(size);
     const void* addr = fReader.peek();
-    fError = fError || !IsPtrAlign4(addr) || !fReader.isAvailable(inc);
+    this->validate(IsPtrAlign4(addr) && fReader.isAvailable(inc));
     if (!fError) {
         fReader.skip(size);
     }
@@ -45,9 +53,7 @@ const void* SkValidatingReadBuffer::skip(size_t size) {
 bool SkValidatingReadBuffer::readBool() {
     uint32_t value = this->readInt();
     // Boolean value should be either 0 or 1
-    if (value & ~1) {
-        fError = true;
-    }
+    this->validate(!(value & ~1));
     return value != 0;
 }
 
@@ -61,13 +67,13 @@ SkFixed SkValidatingReadBuffer::readFixed() {
 
 int32_t SkValidatingReadBuffer::readInt() {
     const size_t inc = sizeof(int32_t);
-    fError = fError || !IsPtrAlign4(fReader.peek()) || !fReader.isAvailable(inc);
+    this->validate(IsPtrAlign4(fReader.peek()) && fReader.isAvailable(inc));
     return fError ? 0 : fReader.readInt();
 }
 
 SkScalar SkValidatingReadBuffer::readScalar() {
     const size_t inc = sizeof(SkScalar);
-    fError = fError || !IsPtrAlign4(fReader.peek()) || !fReader.isAvailable(inc);
+    this->validate(IsPtrAlign4(fReader.peek()) && fReader.isAvailable(inc));
     return fError ? 0 : fReader.readScalar();
 }
 
@@ -87,7 +93,7 @@ void SkValidatingReadBuffer::readString(SkString* string) {
     // skip over the string + '\0' and then pad to a multiple of 4
     const size_t alignedSize = SkAlign4(len + 1);
     this->skip(alignedSize);
-    fError = fError || (cptr[len] != '\0');
+    this->validate(cptr[len] == '\0');
     if (!fError) {
         string->set(cptr, len);
     }
@@ -95,7 +101,7 @@ void SkValidatingReadBuffer::readString(SkString* string) {
 
 void* SkValidatingReadBuffer::readEncodedString(size_t* length, SkPaint::TextEncoding encoding) {
     const int32_t encodingType = fReader.readInt();
-    fError = fError || (encodingType != encoding);
+    this->validate(encodingType == encoding);
     *length = this->readInt();
     const void* ptr = this->skip(SkAlign4(*length));
     void* data = NULL;
@@ -113,7 +119,7 @@ void SkValidatingReadBuffer::readPoint(SkPoint* point) {
 
 void SkValidatingReadBuffer::readMatrix(SkMatrix* matrix) {
     const size_t size = matrix->readFromMemory(fReader.peek());
-    fError = fError || (SkAlign4(size) != size);
+    this->validate(SkAlign4(size) == size);
     if (!fError) {
         (void)this->skip(size);
     }
@@ -135,7 +141,7 @@ void SkValidatingReadBuffer::readRect(SkRect* rect) {
 
 void SkValidatingReadBuffer::readRegion(SkRegion* region) {
     const size_t size = region->readFromMemory(fReader.peek());
-    fError = fError || (SkAlign4(size) != size);
+    this->validate(SkAlign4(size) == size);
     if (!fError) {
         (void)this->skip(size);
     }
@@ -143,64 +149,43 @@ void SkValidatingReadBuffer::readRegion(SkRegion* region) {
 
 void SkValidatingReadBuffer::readPath(SkPath* path) {
     const size_t size = path->readFromMemory(fReader.peek());
-    fError = fError || (SkAlign4(size) != size);
+    this->validate(SkAlign4(size) == size);
     if (!fError) {
         (void)this->skip(size);
     }
 }
 
-uint32_t SkValidatingReadBuffer::readByteArray(void* value) {
-    const uint32_t length = this->readUInt();
-    const void* ptr = this->skip(SkAlign4(length));
+bool SkValidatingReadBuffer::readArray(void* value, size_t size, size_t elementSize) {
+    const uint32_t count = this->getArrayCount();
+    this->validate(size == count);
+    (void)this->skip(sizeof(uint32_t)); // Skip array count
+    const size_t byteLength = count * elementSize;
+    const void* ptr = this->skip(SkAlign4(byteLength));
     if (!fError) {
-        memcpy(value, ptr, length);
-        return length;
+        memcpy(value, ptr, byteLength);
+        return true;
     }
-    return 0;
+    return false;
 }
 
-uint32_t SkValidatingReadBuffer::readColorArray(SkColor* colors) {
-    const uint32_t count = this->readUInt();
-    const uint32_t byteLength = count * sizeof(SkColor);
-    const void* ptr = this->skip(SkAlign4(byteLength));
-    if (!fError) {
-        memcpy(colors, ptr, byteLength);
-        return count;
-    }
-    return 0;
+bool SkValidatingReadBuffer::readByteArray(void* value, size_t size) {
+    return readArray(static_cast<unsigned char*>(value), size, sizeof(unsigned char));
 }
 
-uint32_t SkValidatingReadBuffer::readIntArray(int32_t* values) {
-    const uint32_t count = this->readUInt();
-    const uint32_t byteLength = count * sizeof(int32_t);
-    const void* ptr = this->skip(SkAlign4(byteLength));
-    if (!fError) {
-        memcpy(values, ptr, byteLength);
-        return count;
-    }
-    return 0;
+bool SkValidatingReadBuffer::readColorArray(SkColor* colors, size_t size) {
+    return readArray(colors, size, sizeof(SkColor));
 }
 
-uint32_t SkValidatingReadBuffer::readPointArray(SkPoint* points) {
-    const uint32_t count = this->readUInt();
-    const uint32_t byteLength = count * sizeof(SkPoint);
-    const void* ptr = this->skip(SkAlign4(byteLength));
-    if (!fError) {
-        memcpy(points, ptr, byteLength);
-        return count;
-    }
-    return 0;
+bool SkValidatingReadBuffer::readIntArray(int32_t* values, size_t size) {
+    return readArray(values, size, sizeof(int32_t));
 }
 
-uint32_t SkValidatingReadBuffer::readScalarArray(SkScalar* values) {
-    const uint32_t count = this->readUInt();
-    const uint32_t byteLength = count * sizeof(SkScalar);
-    const void* ptr = this->skip(SkAlign4(byteLength));
-    if (!fError) {
-        memcpy(values, ptr, byteLength);
-        return count;
-    }
-    return 0;
+bool SkValidatingReadBuffer::readPointArray(SkPoint* points, size_t size) {
+    return readArray(points, size, sizeof(SkPoint));
+}
+
+bool SkValidatingReadBuffer::readScalarArray(SkScalar* values, size_t size) {
+    return readArray(values, size, sizeof(SkScalar));
 }
 
 uint32_t SkValidatingReadBuffer::getArrayCount() {
@@ -212,12 +197,17 @@ void SkValidatingReadBuffer::readBitmap(SkBitmap* bitmap) {
     const int height = this->readInt();
     const size_t length = this->readUInt();
     // A size of zero means the SkBitmap was simply flattened.
-    fError = fError || (length != 0);
+    this->validate(length == 0);
     if (fError) {
         return;
     }
     bitmap->unflatten(*this);
-    fError = fError || (bitmap->width() != width) || (bitmap->height() != height);
+    this->validate((bitmap->width() == width) && (bitmap->height() == height));
+}
+
+SkTypeface* SkValidatingReadBuffer::readTypeface() {
+    // TODO: Implement this (securely) when needed
+    return NULL;
 }
 
 SkFlattenable* SkValidatingReadBuffer::readFlattenable(SkFlattenable::Type type) {
@@ -248,7 +238,7 @@ SkFlattenable* SkValidatingReadBuffer::readFlattenable(SkFlattenable::Type type)
         obj = (*factory)(*this);
         // check that we read the amount we expected
         uint32_t sizeRead = fReader.offset() - offset;
-        fError = fError || (sizeRecorded != sizeRead);
+        this->validate(sizeRecorded == sizeRead);
         if (fError) {
             // we could try to fix up the offset...
             delete obj;
index c854b0a..4d1919b 100644 (file)
@@ -47,24 +47,24 @@ public:
     virtual void readPath(SkPath* path) SK_OVERRIDE;
 
     // binary data and arrays
-    virtual uint32_t readByteArray(void* value) SK_OVERRIDE;
-    virtual uint32_t readColorArray(SkColor* colors) SK_OVERRIDE;
-    virtual uint32_t readIntArray(int32_t* values) SK_OVERRIDE;
-    virtual uint32_t readPointArray(SkPoint* points) SK_OVERRIDE;
-    virtual uint32_t readScalarArray(SkScalar* values) SK_OVERRIDE;
+    virtual bool readByteArray(void* value, size_t size) SK_OVERRIDE;
+    virtual bool readColorArray(SkColor* colors, size_t size) SK_OVERRIDE;
+    virtual bool readIntArray(int32_t* values, size_t size) SK_OVERRIDE;
+    virtual bool readPointArray(SkPoint* points, size_t size) SK_OVERRIDE;
+    virtual bool readScalarArray(SkScalar* values, size_t size) SK_OVERRIDE;
 
     // helpers to get info about arrays and binary data
     virtual uint32_t getArrayCount() SK_OVERRIDE;
 
     virtual void readBitmap(SkBitmap* bitmap) SK_OVERRIDE;
     // TODO: Implement this (securely) when needed
-    virtual SkTypeface* readTypeface() SK_OVERRIDE { return NULL; }
+    virtual SkTypeface* readTypeface() SK_OVERRIDE;
 
-    virtual void validate(bool isValid) SK_OVERRIDE {
-        fError = fError || !isValid;
-    }
+    virtual void validate(bool isValid) SK_OVERRIDE;
 
 private:
+    bool readArray(void* value, size_t size, size_t elementSize);
+
     void setMemory(const void* data, size_t size);
 
     static bool IsPtrAlign4(const void* ptr) {
index 778df3f..d667415 100644 (file)
@@ -41,8 +41,8 @@ SkBicubicImageFilter* SkBicubicImageFilter::CreateMitchell(const SkSize& scale,
 }
 
 SkBicubicImageFilter::SkBicubicImageFilter(SkFlattenableReadBuffer& buffer) : INHERITED(buffer) {
-    SkDEBUGCODE(uint32_t readSize =) buffer.readScalarArray(fCoefficients);
-    SkASSERT(readSize == 16);
+    SkDEBUGCODE(bool success =) buffer.readScalarArray(fCoefficients, 16);
+    SkASSERT(success);
     fScale.fWidth = buffer.readScalar();
     fScale.fHeight = buffer.readScalar();
     buffer.validate(SkScalarIsFinite(fScale.fWidth) &&
index f7b283e..d8eb1f1 100644 (file)
@@ -308,7 +308,7 @@ void SkColorMatrixFilter::flatten(SkFlattenableWriteBuffer& buffer) const {
 SkColorMatrixFilter::SkColorMatrixFilter(SkFlattenableReadBuffer& buffer)
         : INHERITED(buffer) {
     SkASSERT(buffer.getArrayCount() == 20);
-    buffer.readScalarArray(fMatrix.fMat);
+    buffer.readScalarArray(fMatrix.fMat, 20);
     this->initState(fMatrix.fMat);
     for (int i = 0; i < 20; ++i) {
         buffer.validate(SkScalarIsFinite(fMatrix.fMat[i]));
index 4aa46ab..4099058 100644 (file)
@@ -555,5 +555,5 @@ SkDashPathEffect::SkDashPathEffect(SkFlattenableReadBuffer& buffer) : INHERITED(
 
     fCount = buffer.getArrayCount();
     fIntervals = (SkScalar*)sk_malloc_throw(sizeof(SkScalar) * fCount);
-    buffer.readScalarArray(fIntervals);
+    buffer.readScalarArray(fIntervals, fCount);
 }
index 31beb51..2d31250 100644 (file)
@@ -132,7 +132,7 @@ bool SkEmbossMaskFilter::filterMask(SkMask* dst, const SkMask& src,
 SkEmbossMaskFilter::SkEmbossMaskFilter(SkFlattenableReadBuffer& buffer)
         : SkMaskFilter(buffer) {
     SkASSERT(buffer.getArrayCount() == sizeof(Light));
-    buffer.readByteArray(&fLight);
+    buffer.readByteArray(&fLight, sizeof(Light));
     SkASSERT(fLight.fPad == 0); // for the font-cache lookup to be clean
     fBlurSigma = buffer.readScalar();
 #ifndef DELETE_THIS_CODE_WHEN_SKPS_ARE_REBUILT_AT_V13_AND_ALL_OTHER_INSTANCES_TOO
index 485001b..821eebd 100644 (file)
@@ -120,8 +120,8 @@ void SkKernel33MaskFilter::flatten(SkFlattenableWriteBuffer& wb) const {
 
 SkKernel33MaskFilter::SkKernel33MaskFilter(SkFlattenableReadBuffer& rb)
         : SkKernel33ProcMaskFilter(rb) {
-    SkDEBUGCODE(const uint32_t count = )rb.readIntArray(&fKernel[0][0]);
-    SkASSERT(9 == count);
+    SkDEBUGCODE(bool success = )rb.readIntArray(&fKernel[0][0], 9);
+    SkASSERT(success);
     fShift = rb.readInt();
 }
 
index cac30e6..4864aec 100644 (file)
@@ -59,18 +59,20 @@ SkMatrixConvolutionImageFilter::SkMatrixConvolutionImageFilter(
 
 SkMatrixConvolutionImageFilter::SkMatrixConvolutionImageFilter(SkFlattenableReadBuffer& buffer)
     : INHERITED(buffer) {
+    // We need to be able to read at most SK_MaxS32 bytes, so divide that
+    // by the size of a scalar to know how many scalars we can read.
+    static const int32_t kMaxSize = SK_MaxS32 / sizeof(SkScalar);
     fKernelSize.fWidth = buffer.readInt();
     fKernelSize.fHeight = buffer.readInt();
     if ((fKernelSize.fWidth >= 1) && (fKernelSize.fHeight >= 1) &&
         // Make sure size won't be larger than a signed int,
         // which would still be extremely large for a kernel,
         // but we don't impose a hard limit for kernel size
-        (SK_MaxS32 / fKernelSize.fWidth >= fKernelSize.fHeight)) {
-        uint32_t size = fKernelSize.fWidth * fKernelSize.fHeight;
+        (kMaxSize / fKernelSize.fWidth >= fKernelSize.fHeight)) {
+        size_t size = fKernelSize.fWidth * fKernelSize.fHeight;
         fKernel = SkNEW_ARRAY(SkScalar, size);
-        uint32_t readSize = buffer.readScalarArray(fKernel);
-        SkASSERT(readSize == size);
-        buffer.validate(readSize == size);
+        SkDEBUGCODE(bool success =) buffer.readScalarArray(fKernel, size);
+        SkASSERT(success);
     } else {
         fKernel = 0;
     }
index 4de1093..93e2335 100755 (executable)
@@ -161,10 +161,9 @@ SkMergeImageFilter::SkMergeImageFilter(SkFlattenableReadBuffer& buffer) : INHERI
     if (hasModes) {
         this->initAllocModes();
         int nbInputs = countInputs();
-        bool sizeMatches = buffer.getArrayCount() == nbInputs * sizeof(fModes[0]);
-        buffer.validate(sizeMatches);
-        SkASSERT(sizeMatches);
-        buffer.readByteArray(fModes);
+        size_t size = nbInputs * sizeof(fModes[0]);
+        SkASSERT(buffer.getArrayCount() == size);
+        buffer.readByteArray(fModes, size);
         for (int i = 0; i < nbInputs; ++i) {
             buffer.validate(SkIsValidMode((SkXfermode::Mode)fModes[i]));
         }
index 083b54c..e15baf6 100644 (file)
@@ -189,7 +189,7 @@ SkTable_ColorFilter::SkTable_ColorFilter(SkFlattenableReadBuffer& buffer) : INHE
 
     size_t size = buffer.getArrayCount();
     SkASSERT(size <= sizeof(storage));
-    buffer.readByteArray(storage);
+    buffer.readByteArray(storage, size);
 
     SkDEBUGCODE(size_t raw = ) SkPackBits::Unpack8(storage, size, fStorage);
 
index 5bff4de..8d3b81a 100644 (file)
@@ -77,7 +77,7 @@ void SkTableMaskFilter::flatten(SkFlattenableWriteBuffer& wb) const {
 SkTableMaskFilter::SkTableMaskFilter(SkFlattenableReadBuffer& rb)
         : INHERITED(rb) {
     SkASSERT(256 == rb.getArrayCount());
-    rb.readByteArray(fTable);
+    rb.readByteArray(fTable, 256);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
index c90adfe..2776199 100644 (file)
@@ -159,7 +159,7 @@ SkGradientShaderBase::SkGradientShaderBase(SkFlattenableReadBuffer& buffer) : IN
     } else {
         fOrigColors = fStorage;
     }
-    buffer.readColorArray(fOrigColors);
+    buffer.readColorArray(fOrigColors, colorCount);
 
     {
         uint32_t packed = buffer.readUInt();
index ead835f..1a8284b 100644 (file)
@@ -176,7 +176,7 @@ SkImageRef::SkImageRef(SkFlattenableReadBuffer& buffer, SkBaseMutex* mutex)
 
     size_t length = buffer.getArrayCount();
     fStream = SkNEW_ARGS(SkMemoryStream, (length));
-    buffer.readByteArray((void*)fStream->getMemoryBase());
+    buffer.readByteArray((void*)fStream->getMemoryBase(), length);
 
     fPrev = fNext = NULL;
     fFactory = NULL;
diff --git a/tests/SerializationTest.cpp b/tests/SerializationTest.cpp
new file mode 100644 (file)
index 0000000..d2c6925
--- /dev/null
@@ -0,0 +1,150 @@
+/*
+ * Copyright 2013 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "SkOrderedWriteBuffer.h"
+#include "SkValidatingReadBuffer.h"
+#include "Test.h"
+
+static void Tests(skiatest::Reporter* reporter) {
+    {
+        static const uint32_t arraySize = 512;
+        unsigned char data[arraySize] = {0};
+        SkOrderedWriteBuffer writer(1024);
+        writer.setFlags(SkOrderedWriteBuffer::kValidation_Flag);
+        writer.writeByteArray(data, arraySize);
+        uint32_t bytesWritten = writer.bytesWritten();
+        // This should write the length (in 4 bytes) and the array
+        REPORTER_ASSERT(reporter, (4 + arraySize) == bytesWritten);
+
+        unsigned char dataWritten[1024];
+        writer.writeToMemory(dataWritten);
+
+        // Make sure this fails when it should
+        SkValidatingReadBuffer buffer(dataWritten, bytesWritten);
+        unsigned char dataRead[arraySize];
+        bool success = buffer.readByteArray(dataRead, 256);
+        // This should have failed, since 256 < sizeInBytes
+        REPORTER_ASSERT(reporter, !success);
+
+        // Make sure this succeeds when it should
+        SkValidatingReadBuffer buffer2(dataWritten, bytesWritten);
+        success = buffer2.readByteArray(dataRead, arraySize);
+        // This should have succeeded, since there are enough bytes to read this
+        REPORTER_ASSERT(reporter, success);
+    }
+
+    {
+        static const uint32_t arraySize = 64;
+        SkColor data[arraySize];
+        SkOrderedWriteBuffer writer(1024);
+        writer.setFlags(SkOrderedWriteBuffer::kValidation_Flag);
+        writer.writeColorArray(data, arraySize);
+        uint32_t bytesWritten = writer.bytesWritten();
+        // This should write the length (in 4 bytes) and the array
+        REPORTER_ASSERT(reporter, (4 + arraySize * sizeof(SkColor)) == bytesWritten);
+
+        unsigned char dataWritten[1024];
+        writer.writeToMemory(dataWritten);
+
+        // Make sure this fails when it should
+        SkValidatingReadBuffer buffer(dataWritten, bytesWritten);
+        SkColor dataRead[arraySize];
+        bool success = buffer.readColorArray(dataRead, 32);
+        // This should have failed, since 256 < sizeInBytes
+        REPORTER_ASSERT(reporter, !success);
+
+        // Make sure this succeeds when it should
+        SkValidatingReadBuffer buffer2(dataWritten, bytesWritten);
+        success = buffer2.readColorArray(dataRead, arraySize);
+        // This should have succeeded, since there are enough bytes to read this
+        REPORTER_ASSERT(reporter, success);
+    }
+
+    {
+        static const uint32_t arraySize = 64;
+        int32_t data[arraySize];
+        SkOrderedWriteBuffer writer(1024);
+        writer.setFlags(SkOrderedWriteBuffer::kValidation_Flag);
+        writer.writeIntArray(data, arraySize);
+        uint32_t bytesWritten = writer.bytesWritten();
+        // This should write the length (in 4 bytes) and the array
+        REPORTER_ASSERT(reporter, (4 + arraySize * sizeof(int32_t)) == bytesWritten);
+
+        unsigned char dataWritten[1024];
+        writer.writeToMemory(dataWritten);
+
+        // Make sure this fails when it should
+        SkValidatingReadBuffer buffer(dataWritten, bytesWritten);
+        int32_t dataRead[arraySize];
+        bool success = buffer.readIntArray(dataRead, 32);
+        // This should have failed, since 256 < sizeInBytes
+        REPORTER_ASSERT(reporter, !success);
+
+        // Make sure this succeeds when it should
+        SkValidatingReadBuffer buffer2(dataWritten, bytesWritten);
+        success = buffer2.readIntArray(dataRead, arraySize);
+        // This should have succeeded, since there are enough bytes to read this
+        REPORTER_ASSERT(reporter, success);
+    }
+
+    {
+        static const uint32_t arraySize = 64;
+        SkPoint data[arraySize];
+        SkOrderedWriteBuffer writer(1024);
+        writer.setFlags(SkOrderedWriteBuffer::kValidation_Flag);
+        writer.writePointArray(data, arraySize);
+        uint32_t bytesWritten = writer.bytesWritten();
+        // This should write the length (in 4 bytes) and the array
+        REPORTER_ASSERT(reporter, (4 + arraySize * sizeof(SkPoint)) == bytesWritten);
+
+        unsigned char dataWritten[1024];
+        writer.writeToMemory(dataWritten);
+
+        // Make sure this fails when it should
+        SkValidatingReadBuffer buffer(dataWritten, bytesWritten);
+        SkPoint dataRead[arraySize];
+        bool success = buffer.readPointArray(dataRead, 32);
+        // This should have failed, since 256 < sizeInBytes
+        REPORTER_ASSERT(reporter, !success);
+
+        // Make sure this succeeds when it should
+        SkValidatingReadBuffer buffer2(dataWritten, bytesWritten);
+        success = buffer2.readPointArray(dataRead, arraySize);
+        // This should have succeeded, since there are enough bytes to read this
+        REPORTER_ASSERT(reporter, success);
+    }
+
+    {
+        static const uint32_t arraySize = 64;
+        SkScalar data[arraySize];
+        SkOrderedWriteBuffer writer(1024);
+        writer.setFlags(SkOrderedWriteBuffer::kValidation_Flag);
+        writer.writeScalarArray(data, arraySize);
+        uint32_t bytesWritten = writer.bytesWritten();
+        // This should write the length (in 4 bytes) and the array
+        REPORTER_ASSERT(reporter, (4 + arraySize * sizeof(SkScalar)) == bytesWritten);
+
+        unsigned char dataWritten[1024];
+        writer.writeToMemory(dataWritten);
+
+        // Make sure this fails when it should
+        SkValidatingReadBuffer buffer(dataWritten, bytesWritten);
+        SkScalar dataRead[arraySize];
+        bool success = buffer.readScalarArray(dataRead, 32);
+        // This should have failed, since 256 < sizeInBytes
+        REPORTER_ASSERT(reporter, !success);
+
+        // Make sure this succeeds when it should
+        SkValidatingReadBuffer buffer2(dataWritten, bytesWritten);
+        success = buffer2.readScalarArray(dataRead, arraySize);
+        // This should have succeeded, since there are enough bytes to read this
+        REPORTER_ASSERT(reporter, success);
+    }
+}
+
+#include "TestClassDef.h"
+DEFINE_TESTCLASS("Serialization", SerializationClass, Tests)