commit e490420efc931db8d2c8f397ddf704aab980295d
author scroggo@google.com <scroggo@google.com@2bbb7eff-a529-9590-31e7-b0007b416f81> Wed Jan 09 22:02:58 2013 +0000
committer scroggo@google.com <scroggo@google.com@2bbb7eff-a529-9590-31e7-b0007b416f81> Wed Jan 09 22:02:58 2013 +0000
tree c41533c4643114e662cdc5c705d54847a06a169a
parent 74dda9018e8bdd650698e763404f19144c6d0a42

Ensure that SkMemoryStream does not crash dereferencing fData.

SkMemoryStream attempts to dereference fData in multiple places.
Instead of allowing it to be NULL, resulting in a crash, set it
to SkData::NewEmpty().

Add a test for SkStream that will crash when its SkData is set to NULL.

Review URL: https://codereview.appspot.com/7061059

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

include/core/SkStream.h

diff --git a/include/core/SkStream.h b/include/core/SkStream.h
index 8f0548e..7ec3117 100644
--- a/include/core/SkStream.h
+++ b/include/core/SkStream.h
@@ -188,7 +188,13 @@
     /** if copyData is true, the stream makes a private copy of the data
     */
     SkMemoryStream(const void* data, size_t length, bool copyData = false);
-    SkMemoryStream(SkData *data);
+
+    /**
+     *  Use the specified data as the memory for this stream. The stream will
+     *  call ref() on the data (assuming it is not null).
+     */
+    SkMemoryStream(SkData*);
+
     virtual ~SkMemoryStream();
 
     /** Resets the stream to the specified data and length,

src/core/SkStream.cpp

diff --git a/src/core/SkStream.cpp b/src/core/SkStream.cpp
index 2851e5f..f173b9e 100644
--- a/src/core/SkStream.cpp
+++ b/src/core/SkStream.cpp
@@ -299,9 +299,13 @@
     fOffset = 0;
 }
 
-SkMemoryStream::SkMemoryStream(SkData *data) {
-    fData = SkSafeRef(data);
-    fOffset = 0;
+SkMemoryStream::SkMemoryStream(SkData* data) {
+    if (NULL == data) {
+        fData = SkData::NewEmpty();
+    } else {
+        fData = data;
+        fData->ref();
+    }
 }
 
 SkMemoryStream::~SkMemoryStream() {
@@ -326,7 +330,13 @@
 }
 
 SkData* SkMemoryStream::setData(SkData* data) {
-    SkRefCnt_SafeAssign(fData, data);
+    fData->unref();
+    if (NULL == data) {
+        fData = SkData::NewEmpty();
+    } else {
+        fData = data;
+        fData->ref();
+    }
     return data;
 }
 

tests/StreamTest.cpp

diff --git a/tests/StreamTest.cpp b/tests/StreamTest.cpp
index b5e3cd9..ed51aa7 100644
--- a/tests/StreamTest.cpp
+++ b/tests/StreamTest.cpp
@@ -130,10 +130,29 @@
     }
 }
 
+// Test that setting an SkMemoryStream to a NULL data does not result in a crash when calling
+// methods that access fData.
+static void TestDereferencingData(SkMemoryStream* memStream) {
+    memStream->read(NULL, 0);
+    memStream->getMemoryBase();
+    SkAutoDataUnref data(memStream->copyToData());
+}
+
+static void TestNullData() {
+    SkData* nullData = NULL;
+    SkMemoryStream memStream(nullData);
+    TestDereferencingData(&memStream);
+
+    memStream.setData(nullData);
+    TestDereferencingData(&memStream);
+
+}
+
 static void TestStreams(skiatest::Reporter* reporter) {
     TestRStream(reporter);
     TestWStream(reporter);
     TestPackedUInt(reporter);
+    TestNullData();
 }
 
 #include "TestClassDef.h"