Fixed a few places where uninitialized memory could have been read

Also added early exit in SkImageFilter's constructor to avoid attempting to deserialize all inputs once a bad input has been found. This avoids hanging if a filter pretends to have 1 billion inputs when that's just an error on the number of inputs read by the filter.

BUG=326206,326197,326229
R=senorblanco@chromium.org, senorblanco@google.com, reed@google.com, sugoi@google.com

Author: sugoi@chromium.org

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

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

diff --git a/include/core/SkFlattenableBuffers.h b/include/core/SkFlattenableBuffers.h
index 575dec8917661d3356f0e2035aecb0033c5dceeb..00cb77a8d35f1ba98e80493767c081764f4d51d0 100644 (file)
--- a/include/core/SkFlattenableBuffers.h
+++ b/include/core/SkFlattenableBuffers.h
@@ -154,6 +154,12 @@ public:
       */
     virtual bool validate(bool isValid);
 
+    /** This function returns true by default
+      * If isValidating() is true, it will return false if the internal error flag is set.
+      * Otherwise, it will return true.
+      */
+    virtual bool isValid() const { return true; }
+
 private:
     template <typename T> T* readFlattenableT();
     uint32_t fFlags;

diff --git a/samplecode/SampleFilterFuzz.cpp b/samplecode/SampleFilterFuzz.cpp
index d15c6ebc905dc3030a3af839a128672ae3a3d6e6..06e14f0e7da7b3549d086942d7f875ca565146af 100644 (file)
--- a/samplecode/SampleFilterFuzz.cpp
+++ b/samplecode/SampleFilterFuzz.cpp
@@ -31,6 +31,9 @@
 #include <stdio.h>
 #include <time.h>
 
+//#define SK_ADD_RANDOM_BIT_FLIPS
+//#define SK_FUZZER_IS_VERBOSE
+
 static const uint32_t kSeed = (uint32_t)(time(NULL));
 static SkRandom gRand(kSeed);
 static bool return_large = false;

diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp
index d2a308b03ab950fe536980ea0a84a8d6122f62f9..7e204f22d33811fd729815881bbf88352492a638 100644 (file)
--- a/src/core/SkBitmap.cpp
+++ b/src/core/SkBitmap.cpp
@@ -1557,8 +1557,11 @@ void SkBitmap::unflatten(SkFlattenableReadBuffer& buffer) {
     buffer.validate((width >= 0) && (height >= 0) && (rowBytes >= 0) &&
                     SkIsValidConfig(config) && validate_alphaType(config, alphaType));
 
-    this->setConfig(config, width, height, rowBytes, alphaType);
-    buffer.validate(fRowBytes >= (fWidth * fBytesPerPixel));
+    bool configIsValid = this->setConfig(config, width, height, rowBytes, alphaType);
+    // Note : Using (fRowBytes >= (fWidth * fBytesPerPixel)) in the following test can create false
+    //        positives if the multiplication causes an integer overflow. Use the division instead.
+    buffer.validate(configIsValid && (fBytesPerPixel > 0) &&
+                    ((fRowBytes / fBytesPerPixel) >= fWidth));
 
     int reftype = buffer.readInt();
     if (buffer.validate((SERIALIZE_PIXELTYPE_REF_DATA == reftype) ||

diff --git a/src/core/SkImageFilter.cpp b/src/core/SkImageFilter.cpp
index 2a221fb869db95371fbea1d45162e490d474cf8f..cda635b4fbf1d8b6d6b2a72c3ff232013fefb060 100644 (file)
--- a/src/core/SkImageFilter.cpp
+++ b/src/core/SkImageFilter.cpp
@@ -61,10 +61,14 @@ SkImageFilter::SkImageFilter(int inputCount, SkFlattenableReadBuffer& buffer) {
             } else {
                 fInputs[i] = NULL;
             }
+            if (!buffer.isValid()) {
+                fInputCount = i; // Do not use fInputs past that point in the destructor
+                break;
+            }
         }
         SkRect rect;
         buffer.readRect(&rect);
-        if (buffer.validate(SkIsValidRect(rect))) {
+        if (buffer.isValid() && buffer.validate(SkIsValidRect(rect))) {
             uint32_t flags = buffer.readUInt();
             fCropRect = CropRect(rect, flags);
         }

diff --git a/src/core/SkValidatingReadBuffer.cpp b/src/core/SkValidatingReadBuffer.cpp
index 4a8ac472472fcbd0edcc77a1d4021ac164df5c9b..692d0dd4d18c83b0f7f5bd13e0bc229ba478253a 100644 (file)
--- a/src/core/SkValidatingReadBuffer.cpp
+++ b/src/core/SkValidatingReadBuffer.cpp
@@ -29,6 +29,10 @@ bool SkValidatingReadBuffer::validate(bool isValid) {
     return !fError;
 }
 
+bool SkValidatingReadBuffer::isValid() const {
+    return !fError;
+}
+
 void SkValidatingReadBuffer::setMemory(const void* data, size_t size) {
     this->validate(IsPtrAlign4(data) && (SkAlign4(size) == size));
     if (!fError) {

diff --git a/src/core/SkValidatingReadBuffer.h b/src/core/SkValidatingReadBuffer.h
index 2bcdc43e8b03358c06d58793084baa810c87fe84..febf0c0b42f9f3f6bac17cbc9075dfac54609d8e 100644 (file)
--- a/src/core/SkValidatingReadBuffer.h
+++ b/src/core/SkValidatingReadBuffer.h
@@ -61,6 +61,7 @@ public:
     virtual SkTypeface* readTypeface() SK_OVERRIDE;
 
     virtual bool validate(bool isValid) SK_OVERRIDE;
+    virtual bool isValid() const SK_OVERRIDE;
 
 private:
     bool readArray(void* value, size_t size, size_t elementSize);

diff --git a/src/effects/SkColorFilters.cpp b/src/effects/SkColorFilters.cpp
index 8ef9edf2b21129304607f8cd55527e1eba977db9..d482a09d1f870323b21e5b34486f452558c011d2 100644 (file)
--- a/src/effects/SkColorFilters.cpp
+++ b/src/effects/SkColorFilters.cpp
@@ -101,8 +101,10 @@ protected:
     SkModeColorFilter(SkFlattenableReadBuffer& buffer) {
         fColor = buffer.readColor();
         fMode = (SkXfermode::Mode)buffer.readUInt();
-        this->updateCache();
-        buffer.validate(SkIsValidMode(fMode));
+        if (buffer.isValid()) {
+            this->updateCache();
+            buffer.validate(SkIsValidMode(fMode));
+        }
     }
 
 private:

diff --git a/src/effects/SkColorMatrixFilter.cpp b/src/effects/SkColorMatrixFilter.cpp
index 5b36a8f7e55df1dec60b77de2b29ba6fa81aec39..fc1b77b7d5e27f4f6ffbe4bf5ae2718daf3726d2 100644 (file)
--- a/src/effects/SkColorMatrixFilter.cpp
+++ b/src/effects/SkColorMatrixFilter.cpp
@@ -308,10 +308,8 @@ void SkColorMatrixFilter::flatten(SkFlattenableWriteBuffer& buffer) const {
 SkColorMatrixFilter::SkColorMatrixFilter(SkFlattenableReadBuffer& buffer)
         : INHERITED(buffer) {
     SkASSERT(buffer.getArrayCount() == 20);
-    buffer.readScalarArray(fMatrix.fMat, 20);
-    this->initState(fMatrix.fMat);
-    for (int i = 0; i < 20; ++i) {
-        buffer.validate(SkScalarIsFinite(fMatrix.fMat[i]));
+    if (buffer.readScalarArray(fMatrix.fMat, 20)) {
+        this->initState(fMatrix.fMat);
     }
 }
 

diff --git a/src/effects/SkMergeImageFilter.cpp b/src/effects/SkMergeImageFilter.cpp
index b90c830725bf6d9cba9e0ae77099f1cf557a0d80..33673b063927a249b89c77cefcac88e43d869a88 100755 (executable)
--- a/src/effects/SkMergeImageFilter.cpp
+++ b/src/effects/SkMergeImageFilter.cpp
@@ -165,9 +165,10 @@ SkMergeImageFilter::SkMergeImageFilter(SkFlattenableReadBuffer& buffer)
         int nbInputs = countInputs();
         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]));
+        if (buffer.readByteArray(fModes, size)) {
+            for (int i = 0; i < nbInputs; ++i) {
+                buffer.validate(SkIsValidMode((SkXfermode::Mode)fModes[i]));
+            }
         }
     } else {
         fModes = 0;

diff --git a/src/effects/SkTileImageFilter.cpp b/src/effects/SkTileImageFilter.cpp
index 7d3b72f0c6200bbd15f428b5bb22f866dc4baccb..c5eace0b74ddd598311af434bb9128cc30437a82 100644 (file)
--- a/src/effects/SkTileImageFilter.cpp
+++ b/src/effects/SkTileImageFilter.cpp
@@ -61,7 +61,7 @@ SkTileImageFilter::SkTileImageFilter(SkFlattenableReadBuffer& buffer)
   : INHERITED(1, buffer) {
     buffer.readRect(&fSrcRect);
     buffer.readRect(&fDstRect);
-    buffer.validate(SkIsValidRect(fSrcRect) && SkIsValidRect(fDstRect));
+    buffer.validate(buffer.isValid() && SkIsValidRect(fSrcRect) && SkIsValidRect(fDstRect));
 }
 
 void SkTileImageFilter::flatten(SkFlattenableWriteBuffer& buffer) const {

diff --git a/tests/SerializationTest.cpp b/tests/SerializationTest.cpp
index e7bb437030264626668d8595f5c1430a739c24af..7ae06d737e4f7643aa7e56abe06a01850ca8eb14 100644 (file)
--- a/tests/SerializationTest.cpp
+++ b/tests/SerializationTest.cpp
@@ -123,7 +123,7 @@ static void TestObjectSerialization(T* testObj, skiatest::Reporter* reporter) {
     SkValidatingReadBuffer buffer(dataWritten, bytesWritten - 4);
     T obj;
     SerializationUtils<T>::Read(buffer, &obj);
-    REPORTER_ASSERT(reporter, !buffer.validate(true));
+    REPORTER_ASSERT(reporter, !buffer.isValid());
 
     // Make sure this succeeds when it should
     SkValidatingReadBuffer buffer2(dataWritten, bytesWritten);
@@ -132,7 +132,7 @@ static void TestObjectSerialization(T* testObj, skiatest::Reporter* reporter) {
     SerializationUtils<T>::Read(buffer2, &obj2);
     const unsigned char* peekAfter = static_cast<const unsigned char*>(buffer2.skip(0));
     // This should have succeeded, since there are enough bytes to read this
-    REPORTER_ASSERT(reporter, buffer2.validate(true));
+    REPORTER_ASSERT(reporter, buffer2.isValid());
     REPORTER_ASSERT(reporter, static_cast<size_t>(peekAfter - peekBefore) == bytesWritten);
 
     TestAlignment(testObj, reporter);
@@ -154,7 +154,7 @@ static T* TestFlattenableSerialization(T* testObj, bool shouldSucceed,
     SkValidatingReadBuffer buffer(dataWritten, bytesWritten - 4);
     T* obj = NULL;
     SerializationUtils<T>::Read(buffer, &obj);
-    REPORTER_ASSERT(reporter, !buffer.validate(true));
+    REPORTER_ASSERT(reporter, !buffer.isValid());
     REPORTER_ASSERT(reporter, NULL == obj);
 
     // Make sure this succeeds when it should
@@ -165,12 +165,12 @@ static T* TestFlattenableSerialization(T* testObj, bool shouldSucceed,
     const unsigned char* peekAfter = static_cast<const unsigned char*>(buffer2.skip(0));
     if (shouldSucceed) {
         // This should have succeeded, since there are enough bytes to read this
-        REPORTER_ASSERT(reporter, buffer2.validate(true));
+        REPORTER_ASSERT(reporter, buffer2.isValid());
         REPORTER_ASSERT(reporter, static_cast<size_t>(peekAfter - peekBefore) == bytesWritten);
         REPORTER_ASSERT(reporter, NULL != obj2);
     } else {
         // If the deserialization was supposed to fail, make sure it did
-        REPORTER_ASSERT(reporter, !buffer.validate(true));
+        REPORTER_ASSERT(reporter, !buffer.isValid());
         REPORTER_ASSERT(reporter, NULL == obj2);
     }