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

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();
     }
 }
 

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;

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);

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,

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;
 }

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()));
 }

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));
+}