From d59948a1714fe32729c77e3ea54e0992d48e8541 Mon Sep 17 00:00:00 2001 From: Matt Sarett Date: Wed, 26 Apr 2017 10:59:48 -0400 Subject: SkPngCodec: Do not return kInvalidConversion on corrupt png In this case, the fuzzer thinks there is a bug because we are returning kInvalidConversion for a corrupt png file. Bug: skia:6550 Change-Id: I33f588442f5eaa8a4d642e9328750779f9a9ef5d Reviewed-on: https://skia-review.googlesource.com/14324 Commit-Queue: Matt Sarett Reviewed-by: Leon Scroggins --- resources/invalid_images/bad_palette.png | Bin 0 -> 368 bytes src/codec/SkPngCodec.cpp | 32 ++++++++++------- src/codec/SkPngCodec.h | 4 +-- tests/CodecTest.cpp | 59 ++++++++++++++++--------------- 4 files changed, 51 insertions(+), 44 deletions(-) create mode 100644 resources/invalid_images/bad_palette.png diff --git a/resources/invalid_images/bad_palette.png b/resources/invalid_images/bad_palette.png new file mode 100644 index 0000000000..984b8f63d6 Binary files /dev/null and b/resources/invalid_images/bad_palette.png differ diff --git a/src/codec/SkPngCodec.cpp b/src/codec/SkPngCodec.cpp index b6f418fd49..1c5f821c65 100644 --- a/src/codec/SkPngCodec.cpp +++ b/src/codec/SkPngCodec.cpp @@ -986,11 +986,11 @@ void SkPngCodec::destroyReadStruct() { // Getting the pixels /////////////////////////////////////////////////////////////////////////////// -bool SkPngCodec::initializeXforms(const SkImageInfo& dstInfo, const Options& options, - SkPMColor ctable[], int* ctableCount) { +SkCodec::Result SkPngCodec::initializeXforms(const SkImageInfo& dstInfo, const Options& options, + SkPMColor ctable[], int* ctableCount) { if (setjmp(PNG_JMPBUF((png_struct*)fPng_ptr))) { SkCodecPrintf("Failed on png_read_update_info.\n"); - return false; + return kInvalidInput; } png_read_update_info(fPng_ptr, fInfo_ptr); @@ -999,7 +999,7 @@ bool SkPngCodec::initializeXforms(const SkImageInfo& dstInfo, const Options& opt fSwizzler.reset(nullptr); if (!this->initializeColorXform(dstInfo, options.fPremulBehavior)) { - return false; + return kInvalidConversion; } // If SkColorSpaceXform directly supports the encoded PNG format, we should skip format @@ -1020,12 +1020,12 @@ bool SkPngCodec::initializeXforms(const SkImageInfo& dstInfo, const Options& opt } if (skipFormatConversion && !options.fSubset) { fXformMode = kColorOnly_XformMode; - return true; + return kSuccess; } if (SkEncodedInfo::kPalette_Color == this->getEncodedInfo().color()) { if (!this->createColorTable(dstInfo, ctableCount)) { - return false; + return kInvalidInput; } } @@ -1033,7 +1033,7 @@ bool SkPngCodec::initializeXforms(const SkImageInfo& dstInfo, const Options& opt copy_color_table(dstInfo, fColorTable.get(), ctable, ctableCount); this->initializeSwizzler(dstInfo, options, skipFormatConversion); - return true; + return kSuccess; } void SkPngCodec::initializeXformParams() { @@ -1115,12 +1115,15 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& dstInfo, void* dst, size_t rowBytes, const Options& options, SkPMColor ctable[], int* ctableCount, int* rowsDecoded) { - if (!conversion_possible(dstInfo, this->getInfo()) || - !this->initializeXforms(dstInfo, options, ctable, ctableCount)) - { + if (!conversion_possible(dstInfo, this->getInfo())) { return kInvalidConversion; } + Result result = this->initializeXforms(dstInfo, options, ctable, ctableCount); + if (kSuccess != result) { + return result; + } + if (options.fSubset) { return kUnimplemented; } @@ -1133,12 +1136,15 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& dstInfo, void* dst, SkCodec::Result SkPngCodec::onStartIncrementalDecode(const SkImageInfo& dstInfo, void* dst, size_t rowBytes, const SkCodec::Options& options, SkPMColor* ctable, int* ctableCount) { - if (!conversion_possible(dstInfo, this->getInfo()) || - !this->initializeXforms(dstInfo, options, ctable, ctableCount)) - { + if (!conversion_possible(dstInfo, this->getInfo())) { return kInvalidConversion; } + Result result = this->initializeXforms(dstInfo, options, ctable, ctableCount); + if (kSuccess != result) { + return result; + } + this->allocateStorage(dstInfo); int firstRow, lastRow; diff --git a/src/codec/SkPngCodec.h b/src/codec/SkPngCodec.h index 4809723db6..d729fb11da 100644 --- a/src/codec/SkPngCodec.h +++ b/src/codec/SkPngCodec.h @@ -103,8 +103,8 @@ private: bool createColorTable(const SkImageInfo& dstInfo, int* ctableCount); // Helper to set up swizzler, color xforms, and color table. Also calls png_read_update_info. - bool initializeXforms(const SkImageInfo& dstInfo, const Options&, SkPMColor* colorPtr, - int* colorCount); + SkCodec::Result initializeXforms(const SkImageInfo& dstInfo, const Options&, + SkPMColor* colorPtr, int* colorCount); void initializeSwizzler(const SkImageInfo& dstInfo, const Options&, bool skipFormatConversion); void allocateStorage(const SkImageInfo& dstInfo); void destroyReadStruct(); diff --git a/tests/CodecTest.cpp b/tests/CodecTest.cpp index 52240542c6..7944877702 100644 --- a/tests/CodecTest.cpp +++ b/tests/CodecTest.cpp @@ -1447,45 +1447,46 @@ DEF_TEST(Codec_rowsDecoded, r) { REPORTER_ASSERT(r, rowsDecoded == 0); } -static void test_invalid_images(skiatest::Reporter* r, const char* path, bool shouldSucceed) { - SkBitmap bitmap; - const bool success = GetResourceAsBitmap(path, &bitmap); - REPORTER_ASSERT(r, success == shouldSucceed); +static void test_invalid_images(skiatest::Reporter* r, const char* path, + SkCodec::Result expectedResult) { + auto* stream = GetResourceAsStream(path); + if (!stream) { + return; + } + + std::unique_ptr codec(SkCodec::NewFromStream(stream)); + REPORTER_ASSERT(r, codec); + + test_info(r, codec.get(), codec->getInfo().makeColorType(kN32_SkColorType), expectedResult, + nullptr); } DEF_TEST(Codec_InvalidImages, r) { // ASAN will complain if there is an issue. - test_invalid_images(r, "invalid_images/int_overflow.ico", false); - test_invalid_images(r, "invalid_images/skbug5887.gif", true); - test_invalid_images(r, "invalid_images/many-progressive-scans.jpg", false); + test_invalid_images(r, "invalid_images/skbug5887.gif", SkCodec::kIncompleteInput); + test_invalid_images(r, "invalid_images/many-progressive-scans.jpg", SkCodec::kInvalidInput); + test_invalid_images(r, "invalid_images/b33251605.bmp", SkCodec::kIncompleteInput); + test_invalid_images(r, "invalid_images/bad_palette.png", SkCodec::kInvalidInput); } -DEF_TEST(Codec_InvalidBmp, r) { - // These files report values that have caused problems with SkFILEStreams. - // They are invalid, and should not create SkCodecs. - for (auto* bmp : { "b33651913.bmp", "b34778578.bmp" } ) { - SkString path = SkOSPath::Join("invalid_images", bmp); - path = GetResourcePath(path.c_str()); - std::unique_ptr stream(new SkFILEStream(path.c_str())); - if (!stream->isValid()) { - return; - } - - std::unique_ptr codec(SkCodec::NewFromStream(stream.release())); - REPORTER_ASSERT(r, !codec); - } -} - -DEF_TEST(Codec_InvalidRLEBmp, r) { - auto* stream = GetResourceAsStream("invalid_images/b33251605.bmp"); - if (!stream) { +static void test_invalid_header(skiatest::Reporter* r, const char* path) { + SkString resourcePath = GetResourcePath(path); + std::unique_ptr stream(new SkFILEStream(resourcePath.c_str())); + if (!stream->isValid()) { return; } - std::unique_ptr codec(SkCodec::NewFromStream(stream)); - REPORTER_ASSERT(r, codec); + std::unique_ptr codec(SkCodec::NewFromStream(stream.release())); + REPORTER_ASSERT(r, !codec); +} - test_info(r, codec.get(), codec->getInfo(), SkCodec::kIncompleteInput, nullptr); +DEF_TEST(Codec_InvalidHeader, r) { + test_invalid_header(r, "invalid_images/int_overflow.ico"); + + // These files report values that have caused problems with SkFILEStreams. + // They are invalid, and should not create SkCodecs. + test_invalid_header(r, "invalid_images/b33651913.bmp"); + test_invalid_header(r, "invalid_images/b34778578.bmp"); } DEF_TEST(Codec_InvalidAnimated, r) { -- cgit v1.2.3