Get rid of leaks in SkCodec::NewFromStream.

SkCodec::NewFromStream claims to delete the passed in SkStream on
failure. This allows the caller to pass an SkStream to the function
and not worry about deleting it depending on the return value.

Most of our SkCodecs did not honor this contract though. Update them
to delete the stream on failure. Further, update SkCodec::NewFromStream
to delete the stream if it did not match any subclass, and delete the
SkCodec if we decided to return NULL because it was too big.

Add a test which tests streams which represent the beginnings of
supported format types but do not contain enough data to create an
SkCodec. The interesting part of the test is when we run it on ASAN,
which will report that we leaked something without the other changes.

BUG=skia:3257

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

diff --git a/src/codec/SkCodec.cpp b/src/codec/SkCodec.cpp
index 5604af8..7bc9146 100644 (file)
--- a/src/codec/SkCodec.cpp
+++ b/src/codec/SkCodec.cpp
@@ -32,8 +32,10 @@ SkCodec* SkCodec::NewFromStream(SkStream* stream) {
     if (!stream) {
         return NULL;
     }
+
+    SkAutoTDelete<SkStream> streamDeleter(stream);
     
-    SkCodec* codec = NULL;
+    SkAutoTDelete<SkCodec> codec(NULL);
     for (uint32_t i = 0; i < SK_ARRAY_COUNT(gDecoderProcs); i++) {
         DecoderProc proc = gDecoderProcs[i];
         const bool correctFormat = proc.IsFormat(stream);
@@ -41,7 +43,7 @@ SkCodec* SkCodec::NewFromStream(SkStream* stream) {
             return NULL;
         }
         if (correctFormat) {
-            codec = proc.NewFromStream(stream);
+            codec.reset(proc.NewFromStream(streamDeleter.detach()));
             break;
         }
     }
@@ -50,12 +52,11 @@ SkCodec* SkCodec::NewFromStream(SkStream* stream) {
     // This is about 4x smaller than a test image that takes a few minutes for
     // dm to decode and draw.
     const int32_t maxSize = 1 << 27;
-    if (codec != NULL &&
-            codec->getInfo().width() * codec->getInfo().height() > maxSize) {
+    if (codec && codec->getInfo().width() * codec->getInfo().height() > maxSize) {
         SkCodecPrintf("Error: Image size too large, cannot decode.\n");
         return NULL;
     } else {
-        return codec;
+        return codec.detach();
     }
 }
 

diff --git a/src/codec/SkCodec_libbmp.cpp b/src/codec/SkCodec_libbmp.cpp
index 75c5715..2b20fae 100644 (file)
--- a/src/codec/SkCodec_libbmp.cpp
+++ b/src/codec/SkCodec_libbmp.cpp
@@ -108,6 +108,7 @@ SkCodec* SkBmpCodec::NewFromIco(SkStream* stream) {
  * Read enough of the stream to initialize the SkBmpCodec. Returns a bool
  * representing success or failure. If it returned true, and codecOut was
  * not NULL, it will be set to a new SkBmpCodec.
+ * Does *not* take ownership of the passed in SkStream.
  *
  */
 bool SkBmpCodec::ReadHeader(SkStream* stream, bool isIco, SkCodec** codecOut) {
@@ -496,8 +497,13 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool isIco, SkCodec** codecOut) {
  *
  */
 SkCodec* SkBmpCodec::NewFromStream(SkStream* stream, bool isIco) {
+    SkAutoTDelete<SkStream> streamDeleter(stream);
     SkCodec* codec = NULL;
     if (ReadHeader(stream, isIco, &codec)) {
+        // codec has taken ownership of stream, so we do not need to
+        // delete it.
+        SkASSERT(codec);
+        streamDeleter.detach();
         return codec;
     }
     return NULL;

diff --git a/src/codec/SkCodec_libbmp.h b/src/codec/SkCodec_libbmp.h
index 650259a..81b514e 100644 (file)
--- a/src/codec/SkCodec_libbmp.h
+++ b/src/codec/SkCodec_libbmp.h
@@ -103,6 +103,7 @@ private:
      * Read enough of the stream to initialize the SkBmpCodec. Returns a bool
      * representing success or failure. If it returned true, and codecOut was
      * not NULL, it will be set to a new SkBmpCodec.
+     * Does *not* take ownership of the passed in SkStream.
      *
      */
     static bool ReadHeader(SkStream*, bool isIco, SkCodec** codecOut);

diff --git a/src/codec/SkCodec_libgif.cpp b/src/codec/SkCodec_libgif.cpp
index 2a1d81f..9356a69 100644 (file)
--- a/src/codec/SkCodec_libgif.cpp
+++ b/src/codec/SkCodec_libgif.cpp
@@ -136,6 +136,7 @@ static uint32_t find_trans_index(const SavedImage& image) {
  * Reads enough of the stream to determine the image format
  */
 SkCodec* SkGifCodec::NewFromStream(SkStream* stream) {
+    SkAutoTDelete<SkStream> streamDeleter(stream);
     // Read gif header, logical screen descriptor, and global color table
     SkAutoTCallIProc<GifFileType, CloseGif> gif(open_gif(stream));
 
@@ -165,7 +166,7 @@ SkCodec* SkGifCodec::NewFromStream(SkStream* stream) {
     // use kPremul directly even when kUnpremul is supported.
     const SkImageInfo& imageInfo = SkImageInfo::Make(width, height,
             kIndex_8_SkColorType, kPremul_SkAlphaType);
-    return SkNEW_ARGS(SkGifCodec, (imageInfo, stream, gif.detach()));
+    return SkNEW_ARGS(SkGifCodec, (imageInfo, streamDeleter.detach(), gif.detach()));
 }
 
 SkGifCodec::SkGifCodec(const SkImageInfo& srcInfo, SkStream* stream,

diff --git a/src/codec/SkCodec_libpng.cpp b/src/codec/SkCodec_libpng.cpp
index 33111ce..121b74f 100644 (file)
--- a/src/codec/SkCodec_libpng.cpp
+++ b/src/codec/SkCodec_libpng.cpp
@@ -346,11 +346,12 @@ static bool read_header(SkStream* stream, png_structp* png_ptrp,
 }
 
 SkCodec* SkPngCodec::NewFromStream(SkStream* stream) {
+    SkAutoTDelete<SkStream> streamDeleter(stream);
     png_structp png_ptr;
     png_infop info_ptr;
     SkImageInfo imageInfo;
     if (read_header(stream, &png_ptr, &info_ptr, &imageInfo)) {
-        return SkNEW_ARGS(SkPngCodec, (imageInfo, stream, png_ptr, info_ptr));
+        return SkNEW_ARGS(SkPngCodec, (imageInfo, streamDeleter.detach(), png_ptr, info_ptr));
     }
     return NULL;
 }

diff --git a/src/codec/SkCodec_wbmp.cpp b/src/codec/SkCodec_wbmp.cpp
index 465c76d..073165d 100644 (file)
--- a/src/codec/SkCodec_wbmp.cpp
+++ b/src/codec/SkCodec_wbmp.cpp
@@ -154,6 +154,7 @@ bool SkWbmpCodec::IsWbmp(SkStream* stream) {
 }
 
 SkCodec* SkWbmpCodec::NewFromStream(SkStream* stream) {
+    SkAutoTDelete<SkStream> streamDeleter(stream);
     SkISize size;
     if (!read_header(stream, &size)) {
         return NULL;
@@ -161,5 +162,5 @@ SkCodec* SkWbmpCodec::NewFromStream(SkStream* stream) {
     SkImageInfo info =
             SkImageInfo::Make(size.width(), size.height(), kGray_8_SkColorType,
                               kOpaque_SkAlphaType);
-    return SkNEW_ARGS(SkWbmpCodec, (info, stream));
+    return SkNEW_ARGS(SkWbmpCodec, (info, streamDeleter.detach()));
 }

diff --git a/tests/CodexTest.cpp b/tests/CodexTest.cpp
index 8252518..26846a4 100644 (file)
--- a/tests/CodexTest.cpp
+++ b/tests/CodexTest.cpp
@@ -109,3 +109,32 @@ DEF_TEST(Codec, r) {
     check(r, "randPixels.png", SkISize::Make(8, 8), true);
     check(r, "yellow_rose.png", SkISize::Make(400, 301), true);
 }
+
+static void test_invalid_stream(skiatest::Reporter* r, const void* stream, size_t len) {
+    SkCodec* codec = SkCodec::NewFromStream(new SkMemoryStream(stream, len, false));
+    // We should not have gotten a codec. Bots should catch us if we leaked anything.
+    REPORTER_ASSERT(r, !codec);
+}
+
+// Ensure that SkCodec::NewFromStream handles freeing the passed in SkStream,
+// even on failure. Test some bad streams.
+DEF_TEST(Codec_leaks, r) {
+    // No codec should claim this as their format, so this tests SkCodec::NewFromStream.
+    const char nonSupportedStream[] = "hello world";
+    // The other strings should look like the beginning of a file type, so we'll call some
+    // internal version of NewFromStream, which must also delete the stream on failure.
+    const unsigned char emptyPng[] = { 0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a };
+    const unsigned char emptyJpeg[] = { 0xFF, 0xD8, 0xFF };
+    const char emptyWebp[] = "RIFF1234WEBPVP";
+    const char emptyBmp[] = { 'B', 'M' };
+    const char emptyIco[] = { '\x00', '\x00', '\x01', '\x00' };
+    const char emptyGif[] = "GIFVER";
+
+    test_invalid_stream(r, nonSupportedStream, sizeof(nonSupportedStream));
+    test_invalid_stream(r, emptyPng, sizeof(emptyPng));
+    test_invalid_stream(r, emptyJpeg, sizeof(emptyJpeg));
+    test_invalid_stream(r, emptyWebp, sizeof(emptyWebp));
+    test_invalid_stream(r, emptyBmp, sizeof(emptyBmp));
+    test_invalid_stream(r, emptyIco, sizeof(emptyIco));
+    test_invalid_stream(r, emptyGif, sizeof(emptyGif));
+}