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 <msarett@google.com>
Reviewed-by: Leon Scroggins <scroggo@google.com>

diff --git a/resources/invalid_images/bad_palette.png b/resources/invalid_images/bad_palette.png
new file mode 100644 (file)
index 0000000..984b8f6
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 b6f418f..1c5f821 100644 (file)
--- 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 4809723..d729fb1 100644 (file)
--- 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 5224054..7944877 100644 (file)
--- 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<SkCodec> 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<SkFILEStream> stream(new SkFILEStream(path.c_str()));
-        if (!stream->isValid()) {
-            return;
-        }
-
-        std::unique_ptr<SkCodec> 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<SkFILEStream> stream(new SkFILEStream(resourcePath.c_str()));
+    if (!stream->isValid()) {
         return;
     }
 
-    std::unique_ptr<SkCodec> codec(SkCodec::NewFromStream(stream));
-    REPORTER_ASSERT(r, codec);
+    std::unique_ptr<SkCodec> 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) {