Fixed issue found by clusterfuzz

An integer overflow was causing an issue when reading a string with a very large (or negative) size.

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

Author: sugoi@chromium.org

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

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

diff --git a/include/core/SkReader32.h b/include/core/SkReader32.h
index 7e8038d..51e28ef 100644 (file)
--- a/include/core/SkReader32.h
+++ b/include/core/SkReader32.h
@@ -40,7 +40,7 @@ public:
     const void* peek() const { return fCurr; }
 
     size_t available() const { return fStop - fCurr; }
-    bool isAvailable(size_t size) const { return fCurr + size <= fStop; }
+    bool isAvailable(size_t size) const { return size <= this->available(); }
 
     void rewind() { fCurr = fBase; }
 

diff --git a/src/core/SkValidatingReadBuffer.cpp b/src/core/SkValidatingReadBuffer.cpp
index 95bf83c..0112f18 100644 (file)
--- a/src/core/SkValidatingReadBuffer.cpp
+++ b/src/core/SkValidatingReadBuffer.cpp
@@ -91,7 +91,7 @@ int32_t SkValidatingReadBuffer::read32() {
 }
 
 void SkValidatingReadBuffer::readString(SkString* string) {
-    const size_t len = this->readInt();
+    const size_t len = this->readUInt();
     const void* ptr = fReader.peek();
     const char* cptr = (const char*)ptr;
 

diff --git a/src/core/SkWriter32.cpp b/src/core/SkWriter32.cpp
index 3397c37..00c4636 100644 (file)
--- a/src/core/SkWriter32.cpp
+++ b/src/core/SkWriter32.cpp
@@ -14,7 +14,7 @@
  */
 
 const char* SkReader32::readString(size_t* outLen) {
-    size_t len = this->readInt();
+    size_t len = this->readU32();
     const void* ptr = this->peek();
 
     // skip over the string + '\0' and then pad to a multiple of 4

diff --git a/tests/SerializationTest.cpp b/tests/SerializationTest.cpp
index 94d09c0..c8f8e49 100644 (file)
--- a/tests/SerializationTest.cpp
+++ b/tests/SerializationTest.cpp
@@ -66,6 +66,15 @@ template<> struct SerializationUtils<SkRegion> {
     }
 };
 
+template<> struct SerializationUtils<SkString> {
+    static void Write(SkWriteBuffer& writer, const SkString* string) {
+        writer.writeString(string->c_str());
+    }
+    static void Read(SkValidatingReadBuffer& reader, SkString* string) {
+        reader.readString(string);
+    }
+};
+
 template<> struct SerializationUtils<unsigned char> {
     static void Write(SkWriteBuffer& writer, unsigned char* data, uint32_t arraySize) {
         writer.writeByteArray(data, arraySize);
@@ -111,8 +120,18 @@ template<> struct SerializationUtils<SkScalar> {
     }
 };
 
-template<typename T>
-static void TestObjectSerialization(T* testObj, skiatest::Reporter* reporter) {
+template<typename T, bool testInvalid> struct SerializationTestUtils {
+    static void InvalidateData(unsigned char* data) {}
+};
+
+template<> struct SerializationTestUtils<SkString, true> {
+    static void InvalidateData(unsigned char* data) {
+        data[3] |= 0x80; // Reverse sign of 1st integer
+    }
+};
+
+template<typename T, bool testInvalid>
+static void TestObjectSerializationNoAlign(T* testObj, skiatest::Reporter* reporter) {
     SkWriteBuffer writer(SkWriteBuffer::kValidation_Flag);
     SerializationUtils<T>::Write(writer, testObj);
     size_t bytesWritten = writer.bytesWritten();
@@ -121,6 +140,8 @@ static void TestObjectSerialization(T* testObj, skiatest::Reporter* reporter) {
     unsigned char dataWritten[1024];
     writer.writeToMemory(dataWritten);
 
+    SerializationTestUtils<T, testInvalid>::InvalidateData(dataWritten);
+
     // Make sure this fails when it should (test with smaller size, but still multiple of 4)
     SkValidatingReadBuffer buffer(dataWritten, bytesWritten - 4);
     T obj;
@@ -134,9 +155,15 @@ 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.isValid());
+    REPORTER_ASSERT(reporter, buffer2.isValid() == !testInvalid);
+    // Note: This following test should always succeed, regardless of whether the buffer is valid,
+    // since if it is invalid, it will simply skip to the end, as if it had read the whole buffer.
     REPORTER_ASSERT(reporter, static_cast<size_t>(peekAfter - peekBefore) == bytesWritten);
+}
 
+template<typename T>
+static void TestObjectSerialization(T* testObj, skiatest::Reporter* reporter) {
+    TestObjectSerializationNoAlign<T, false>(testObj, reporter);
     TestAlignment(testObj, reporter);
 }
 
@@ -309,6 +336,13 @@ DEF_TEST(Serialization, reporter) {
         TestObjectSerialization(&region, reporter);
     }
 
+    // Test string serialization
+    {
+        SkString string("string");
+        TestObjectSerializationNoAlign<SkString, false>(&string, reporter);
+        TestObjectSerializationNoAlign<SkString, true>(&string, reporter);
+    }
+
     // Test rrect serialization
     {
         // SkRRect does not initialize anything.