commit e58089fa3c17cad3d8836256218ccb1e6020cf54
author commit-bot@chromium.org <commit-bot@chromium.org@2bbb7eff-a529-9590-31e7-b0007b416f81> Wed Oct 23 18:33:18 2013 +0000
committer commit-bot@chromium.org <commit-bot@chromium.org@2bbb7eff-a529-9590-31e7-b0007b416f81> Wed Oct 23 18:33:18 2013 +0000
tree 228b753a0e6e152fa1fc6ceb7b7eaaa7a0a73e9e
parent 3218b023674b5d0bfc3eca0a6bd6800e24660b8e

Follow up to serialization validation code

1 ) Added check for bool to make sure is it either 0 or 1 and not garbage

2 ) Added more solid kernel size checks in SkMatrixConvolutionImageFilter

3 ) Make sure array size is validated in SkMergeImageFilter

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

Author: sugoi@chromium.org

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

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

core/SkValidatingReadBuffer.cpp

diff --git a/core/SkValidatingReadBuffer.cpp b/core/SkValidatingReadBuffer.cpp
index 0ffe650..a0e1a41 100644
--- a/core/SkValidatingReadBuffer.cpp
+++ b/core/SkValidatingReadBuffer.cpp
@@ -44,7 +44,12 @@
 // true, which the caller should check to see if an error occurred during the read operation.
 
 bool SkValidatingReadBuffer::readBool() {
-    return this->readInt() != 0;
+    uint32_t value = this->readInt();
+    // Boolean value should be either 0 or 1
+    if (value & ~1) {
+        fError = true;
+    }
+    return value != 0;
 }
 
 SkColor SkValidatingReadBuffer::readColor() {

effects/SkMatrixConvolutionImageFilter.cpp

diff --git a/effects/SkMatrixConvolutionImageFilter.cpp b/effects/SkMatrixConvolutionImageFilter.cpp
index 909facb..cac30e6 100644
--- a/effects/SkMatrixConvolutionImageFilter.cpp
+++ b/effects/SkMatrixConvolutionImageFilter.cpp
@@ -61,17 +61,27 @@
     : INHERITED(buffer) {
     fKernelSize.fWidth = buffer.readInt();
     fKernelSize.fHeight = buffer.readInt();
-    uint32_t size = fKernelSize.fWidth * fKernelSize.fHeight;
-    fKernel = SkNEW_ARRAY(SkScalar, size);
-    SkDEBUGCODE(uint32_t readSize = )buffer.readScalarArray(fKernel);
-    SkASSERT(readSize == size);
+    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;
+        fKernel = SkNEW_ARRAY(SkScalar, size);
+        uint32_t readSize = buffer.readScalarArray(fKernel);
+        SkASSERT(readSize == size);
+        buffer.validate(readSize == size);
+    } else {
+        fKernel = 0;
+    }
     fGain = buffer.readScalar();
     fBias = buffer.readScalar();
     fTarget.fX = buffer.readInt();
     fTarget.fY = buffer.readInt();
     fTileMode = (TileMode) buffer.readInt();
     fConvolveAlpha = buffer.readBool();
-    buffer.validate(SkScalarIsFinite(fGain) &&
+    buffer.validate((fKernel != 0) &&
+                    SkScalarIsFinite(fGain) &&
                     SkScalarIsFinite(fBias) &&
                     tile_mode_is_valid(fTileMode));
 }

effects/SkMergeImageFilter.cpp

diff --git a/effects/SkMergeImageFilter.cpp b/effects/SkMergeImageFilter.cpp
index a5c32ac..4de1093 100755
--- a/effects/SkMergeImageFilter.cpp
+++ b/effects/SkMergeImageFilter.cpp
@@ -161,7 +161,9 @@
     if (hasModes) {
         this->initAllocModes();
         int nbInputs = countInputs();
-        SkASSERT(buffer.getArrayCount() == nbInputs * sizeof(fModes[0]));
+        bool sizeMatches = buffer.getArrayCount() == nbInputs * sizeof(fModes[0]);
+        buffer.validate(sizeMatches);
+        SkASSERT(sizeMatches);
         buffer.readByteArray(fModes);
         for (int i = 0; i < nbInputs; ++i) {
             buffer.validate(SkIsValidMode((SkXfermode::Mode)fModes[i]));