diff --git a/src/codec/SkCodec.cpp b/src/codec/SkCodec.cpp index 5604af8d3d..7bc9146041 100644 --- a/src/codec/SkCodec.cpp +++ b/src/codec/SkCodec.cpp @@ -32,8 +32,10 @@ SkCodec* SkCodec::NewFromStream(SkStream* stream) { if (!stream) { return NULL; } + + SkAutoTDelete streamDeleter(stream); - SkCodec* codec = NULL; + SkAutoTDelete 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 75c5715bda..2b20faebad 100644 --- 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 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 650259ad16..81b514e8f5 100644 --- 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 2a1d81fc00..9356a6973b 100644 --- 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 streamDeleter(stream); // Read gif header, logical screen descriptor, and global color table SkAutoTCallIProc 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 33111cee67..121b74ff0b 100644 --- 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 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 465c76d4dc..073165d2ca 100644 --- 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 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 825251854d..26846a4550 100644 --- 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)); +}