From fc4ee229a653d0e9d71f828e513c9d458c1eab57 Mon Sep 17 00:00:00 2001 From: Leon Scroggins III Date: Fri, 14 Jul 2017 11:48:52 -0400 Subject: Fix double delete in SkBmpCodec Previously, if ReadHeader returned false, it deleted the input stream. But there are a couple of cases where ReadHeader creates an SkCodec and then returns false. The SkCodec deletes the stream, and then so does NewFromStream. Make sure that we do not double delete by only deleting if no SkCodec was created. Add a test, so such a double delete will be caught by the bots. Bug: b/37623797 Change-Id: I787422c9af58f0b92ad9e9ef9ad87c54a12f5e31 Reviewed-on: https://skia-review.googlesource.com/23620 Reviewed-by: Derek Sollenberger Commit-Queue: Leon Scroggins --- resources/invalid_images/b37623797.ico | Bin 0 -> 63 bytes src/codec/SkBmpCodec.cpp | 40 +++++++++++++++------------------ src/codec/SkBmpCodec.h | 4 ++-- tests/CodecTest.cpp | 1 + 4 files changed, 21 insertions(+), 24 deletions(-) create mode 100644 resources/invalid_images/b37623797.ico diff --git a/resources/invalid_images/b37623797.ico b/resources/invalid_images/b37623797.ico new file mode 100644 index 0000000000..ebeb57acda Binary files /dev/null and b/resources/invalid_images/b37623797.ico differ diff --git a/src/codec/SkBmpCodec.cpp b/src/codec/SkBmpCodec.cpp index 3142f1e271..3e224c13a9 100644 --- a/src/codec/SkBmpCodec.cpp +++ b/src/codec/SkBmpCodec.cpp @@ -136,9 +136,11 @@ static BmpHeaderType get_header_type(size_t infoBytes) { * Read enough of the stream to initialize the SkBmpCodec. Returns a bool * representing success or failure. If it returned true, and codecOut was * not nullptr, it will be set to a new SkBmpCodec. - * Does *not* take ownership of the passed in SkStream. + * If codecOut is set to a new SkCodec, it will take ownership of the stream. + * Otherwise, the stream will not be deleted. */ -bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, SkCodec** codecOut) { +bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, + std::unique_ptr* codecOut) { // The total bytes in the bmp file // We only need to use this value for RLE decoding, so we will only // check that it is valid in the RLE case. @@ -481,13 +483,10 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, SkCodec** codecOut) { // Set the image info and create a codec. const SkEncodedInfo info = SkEncodedInfo::Make(color, alpha, bitsPerComponent); - std::unique_ptr codec(new SkBmpStandardCodec(width, height, - info, stream, bitsPerPixel, numColors, bytesPerColor, offset - bytesRead, - rowOrder, isOpaque, inIco)); - if (!codec->didCreateSrcBuffer()) { - return false; - } - *codecOut = codec.release(); + codecOut->reset(new SkBmpStandardCodec(width, height, info, stream, bitsPerPixel, + numColors, bytesPerColor, offset - bytesRead, + rowOrder, isOpaque, inIco)); + return static_cast(codecOut->get())->didCreateSrcBuffer(); } return true; } @@ -539,12 +538,9 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, SkCodec** codecOut) { alpha = SkEncodedInfo::kOpaque_Alpha; } const SkEncodedInfo info = SkEncodedInfo::Make(color, alpha, 8); - std::unique_ptr codec(new SkBmpMaskCodec(width, height, info, - stream, bitsPerPixel, masks.release(), rowOrder)); - if (!codec->didCreateSrcBuffer()) { - return false; - } - *codecOut = codec.release(); + codecOut->reset(new SkBmpMaskCodec(width, height, info, stream, bitsPerPixel, + masks.release(), rowOrder)); + return static_cast(codecOut->get())->didCreateSrcBuffer(); } return true; } @@ -572,8 +568,9 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, SkCodec** codecOut) { // For that reason, we always indicate that we are kBGRA. const SkEncodedInfo info = SkEncodedInfo::Make(SkEncodedInfo::kBGRA_Color, SkEncodedInfo::kBinary_Alpha, 8); - *codecOut = new SkBmpRLECodec(width, height, info, stream, bitsPerPixel, numColors, - bytesPerColor, offset - bytesRead, rowOrder); + codecOut->reset(new SkBmpRLECodec(width, height, info, stream, bitsPerPixel, + numColors, bytesPerColor, offset - bytesRead, + rowOrder)); } return true; } @@ -589,15 +586,14 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, SkCodec** codecOut) { */ SkCodec* SkBmpCodec::NewFromStream(SkStream* stream, bool inIco) { std::unique_ptr streamDeleter(stream); - SkCodec* codec = nullptr; - if (ReadHeader(stream, inIco, &codec)) { + std::unique_ptr codec; + bool success = ReadHeader(stream, inIco, &codec); + if (codec) { // codec has taken ownership of stream, so we do not need to // delete it. - SkASSERT(codec); streamDeleter.release(); - return codec; } - return nullptr; + return success ? codec.release() : nullptr; } SkBmpCodec::SkBmpCodec(int width, int height, const SkEncodedInfo& info, SkStream* stream, diff --git a/src/codec/SkBmpCodec.h b/src/codec/SkBmpCodec.h index cf1460582d..bf9727fa1c 100644 --- a/src/codec/SkBmpCodec.h +++ b/src/codec/SkBmpCodec.h @@ -47,9 +47,9 @@ protected: * Read enough of the stream to initialize the SkBmpCodec. Returns a bool * representing success or failure. If it returned true, and codecOut was * not nullptr, it will be set to a new SkBmpCodec. - * Does *not* take ownership of the passed in SkStream. + * If an SkCodec is created, it will take ownership of the SkStream. */ - static bool ReadHeader(SkStream*, bool inIco, SkCodec** codecOut); + static bool ReadHeader(SkStream*, bool inIco, std::unique_ptr* codecOut); bool onRewind() override; diff --git a/tests/CodecTest.cpp b/tests/CodecTest.cpp index 1d7d89aa34..ac4afd190e 100644 --- a/tests/CodecTest.cpp +++ b/tests/CodecTest.cpp @@ -664,6 +664,7 @@ DEF_TEST(Codec_Empty, r) { #if defined(SK_CODEC_DECODES_RAW) && (!defined(_WIN32)) test_invalid(r, "empty_images/zero_height.tiff"); #endif + test_invalid(r, "invalid_images/b37623797.ico"); } #ifdef PNG_READ_UNKNOWN_CHUNKS_SUPPORTED -- cgit v1.2.3