Add SkCodec::Result indicating error in the data

Previously, SkGifCodec treated an error in the LZW data as incomplete,
since we can still draw the partially decoded image. But a client doing
incremental decodes needs to distinguish this from truly incomplete
data. In the case of an error, the client should not attempt to provide
more data and decode again.

Add kErrorInInput, and return it when SkGifCodec sees a fatal error.
Treat it the same as kIncompleteInput when it comes to filling and DM.

Bug: skia:6825
Change-Id: Ic6ce3a62c0b065ed34dcd8006309e43272a3db9f
Reviewed-on: https://skia-review.googlesource.com/21530
Commit-Queue: Leon Scroggins <scroggo@google.com>
Reviewed-by: Mike Reed <reed@google.com>
Reviewed-by: Chris Blume <cblume@chromium.org>

8 files changed, 42 insertions, 22 deletions

diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp
index bceb703cda..0febd2abb0 100644
--- a/dm/DMSrcSink.cpp
+++ b/dm/DMSrcSink.cpp
@@ -513,6 +513,7 @@ Error CodecSrc::draw(SkCanvas* canvas) const {
                 }
                 switch (result) {
                     case SkCodec::kSuccess:
+                    case SkCodec::kErrorInInput:
                     case SkCodec::kIncompleteInput: {
                         // If the next frame depends on this one, store it in priorFrame.
                         // It is possible that we may discard a frame that future frames depend on,
@@ -532,7 +533,7 @@ Error CodecSrc::draw(SkCanvas* canvas) const {
                         canvas->translate(SkIntToScalar(xTranslate), SkIntToScalar(yTranslate));
                         draw_to_canvas(canvas, bitmapInfo, pixels.get(), rowBytes,
                                        colorPtr, colorCount, fDstColorType);
-                        if (result == SkCodec::kIncompleteInput) {
+                        if (result != SkCodec::kSuccess) {
                             return "";
                         }
                         break;
@@ -556,8 +557,9 @@ Error CodecSrc::draw(SkCanvas* canvas) const {
             switch (codec->getPixels(decodeInfo, pixels.get(), rowBytes, &options,
                     colorPtr, &colorCount)) {
                 case SkCodec::kSuccess:
-                    // We consider incomplete to be valid, since we should still decode what is
+                    // We consider these to be valid, since we should still decode what is
                     // available.
+                case SkCodec::kErrorInInput:
                 case SkCodec::kIncompleteInput:
                     break;
                 default:
@@ -589,7 +591,8 @@ Error CodecSrc::draw(SkCanvas* canvas) const {
                 if (SkCodec::kSuccess == codec->startIncrementalDecode(decodeInfo, dst,
                         rowBytes, &options, colorPtr, &colorCount)) {
                     int rowsDecoded;
-                    if (SkCodec::kIncompleteInput == codec->incrementalDecode(&rowsDecoded)) {
+                    auto result = codec->incrementalDecode(&rowsDecoded);
+                    if (SkCodec::kIncompleteInput == result || SkCodec::kErrorInInput == result) {
                         codec->fillIncompleteImage(decodeInfo, dst, rowBytes,
                                                    SkCodec::kNo_ZeroInitialized, height,
                                                    rowsDecoded);
@@ -747,6 +750,7 @@ Error CodecSrc::draw(SkCanvas* canvas) const {
                             &options, colorPtr, &colorCount);
                     switch (result) {
                         case SkCodec::kSuccess:
+                        case SkCodec::kErrorInInput:
                         case SkCodec::kIncompleteInput:
                             break;
                         default:
@@ -880,6 +884,7 @@ Error AndroidCodecSrc::draw(SkCanvas* canvas) const {
 
     switch (codec->getAndroidPixels(decodeInfo, pixels.get(), rowBytes, &options)) {
         case SkCodec::kSuccess:
+        case SkCodec::kErrorInInput:
         case SkCodec::kIncompleteInput:
             break;
         default:
@@ -1124,8 +1129,13 @@ Error ColorCodecSrc::draw(SkCanvas* canvas) const {
 
     size_t rowBytes = bitmap.rowBytes();
     SkCodec::Result r = codec->getPixels(decodeInfo, bitmap.getPixels(), rowBytes);
-    if (SkCodec::kSuccess != r && SkCodec::kIncompleteInput != r) {
-        return SkStringPrintf("Couldn't getPixels %s. Error code %d", fPath.c_str(), r);
+    switch (r) {
+        case SkCodec::kSuccess:
+        case SkCodec::kErrorInInput:
+        case SkCodec::kIncompleteInput:
+            break;
+        default:
+            return SkStringPrintf("Couldn't getPixels %s. Error code %d", fPath.c_str(), r);
     }
 
     switch (fMode) {
diff --git a/fuzz/fuzz.cpp b/fuzz/fuzz.cpp
index 8e2a73376f..d451481049 100644
--- a/fuzz/fuzz.cpp
+++ b/fuzz/fuzz.cpp
@@ -243,6 +243,9 @@ static void fuzz_img(sk_sp<SkData> bytes, uint8_t scale, uint8_t mode) {
                 case SkCodec::kIncompleteInput:
                     SkDebugf("[terminated] Partial Success\n");
                     break;
+                case SkCodec::kErrorInInput:
+                    SkDebugf("[terminated] Partial Success with error\n");
+                    break;
                 case SkCodec::kInvalidConversion:
                     SkDebugf("Incompatible colortype conversion\n");
                     // Crash to allow afl-fuzz to know this was a bug.
@@ -376,6 +379,7 @@ static void fuzz_img(sk_sp<SkData> bytes, uint8_t scale, uint8_t mode) {
                     switch (result) {
                         case SkCodec::kSuccess:
                         case SkCodec::kIncompleteInput:
+                        case SkCodec::kErrorInInput:
                             SkDebugf("okay\n");
                             break;
                         case SkCodec::kInvalidConversion:
@@ -428,7 +432,7 @@ static void fuzz_img(sk_sp<SkData> bytes, uint8_t scale, uint8_t mode) {
                 }
 
                 result = codec->incrementalDecode();
-                if (result == SkCodec::kIncompleteInput) {
+                if (result == SkCodec::kIncompleteInput || result == SkCodec::kErrorInInput) {
                     SkDebugf("okay\n");
                     // Frames beyond this one will not decode.
                     break;
diff --git a/include/codec/SkCodec.h b/include/codec/SkCodec.h
index 6b3aa5e020..2043d230b1 100644
--- a/include/codec/SkCodec.h
+++ b/include/codec/SkCodec.h
@@ -195,6 +195,13 @@ public:
          */
         kIncompleteInput,
         /**
+         *  Like kIncompleteInput, except the input had an error.
+         *
+         *  If returned from an incremental decode, decoding cannot continue,
+         *  even with more data.
+         */
+        kErrorInInput,
+        /**
          *  The generator cannot convert to match the request, ignoring
          *  dimensions.
          */
diff --git a/src/codec/SkCodec.cpp b/src/codec/SkCodec.cpp
index 60d25210c8..26b31ae11f 100644
--- a/src/codec/SkCodec.cpp
+++ b/src/codec/SkCodec.cpp
@@ -317,8 +317,10 @@ SkCodec::Result SkCodec::getPixels(const SkImageInfo& info, void* pixels, size_t
     const Result result = this->onGetPixels(info, pixels, rowBytes, *options, ctable, ctableCount,
             &rowsDecoded);
 
-    if ((kIncompleteInput == result || kSuccess == result) && ctableCount) {
-        SkASSERT(*ctableCount >= 0 && *ctableCount <= 256);
+    if (ctableCount) {
+        if (kIncompleteInput == result || kSuccess == result || kErrorInInput == result) {
+            SkASSERT(*ctableCount >= 0 && *ctableCount <= 256);
+        }
     }
 
     // A return value of kIncompleteInput indicates a truncated image stream.
@@ -326,7 +328,7 @@ SkCodec::Result SkCodec::getPixels(const SkImageInfo& info, void* pixels, size_t
     // Some subclasses will take care of filling any uninitialized memory on
     // their own.  They indicate that all of the memory has been filled by
     // setting rowsDecoded equal to the height.
-    if (kIncompleteInput == result && rowsDecoded != info.height()) {
+    if ((kIncompleteInput == result || kErrorInInput == result) && rowsDecoded != info.height()) {
         // FIXME: (skbug.com/5772) fillIncompleteImage will fill using the swizzler's width, unless
         // there is a subset. In that case, it will use the width of the subset. From here, the
         // subset will only be non-null in the case of SkWebpCodec, but it treats the subset
diff --git a/src/codec/SkCodecImageGenerator.cpp b/src/codec/SkCodecImageGenerator.cpp
index bf794d60d3..20d547ad36 100644
--- a/src/codec/SkCodecImageGenerator.cpp
+++ b/src/codec/SkCodecImageGenerator.cpp
@@ -49,6 +49,7 @@ bool SkCodecImageGenerator::onGetPixels(const SkImageInfo& info, void* pixels, s
     switch (result) {
         case SkCodec::kSuccess:
         case SkCodec::kIncompleteInput:
+        case SkCodec::kErrorInInput:
             return true;
         default:
             return false;
@@ -66,6 +67,7 @@ bool SkCodecImageGenerator::onGetYUV8Planes(const SkYUVSizeInfo& sizeInfo, void*
     switch (result) {
         case SkCodec::kSuccess:
         case SkCodec::kIncompleteInput:
+        case SkCodec::kErrorInInput:
             return true;
         default:
             return false;
diff --git a/src/codec/SkGifCodec.cpp b/src/codec/SkGifCodec.cpp
index eefe8986b7..89889d2555 100644
--- a/src/codec/SkGifCodec.cpp
+++ b/src/codec/SkGifCodec.cpp
@@ -407,20 +407,15 @@ SkCodec::Result SkGifCodec::decodeFrame(bool firstAttempt, const Options& opts,
         return kSuccess;
     }
 
-    // Note: there is a difference between the following call to SkGifImageReader::decode
-    // returning false and leaving frameDecoded false:
-    // - If the method returns false, there was an error in the stream. We still treat this as
-    //   incomplete, since we have already decoded some rows.
-    // - If frameDecoded is false, that just means that we do not have enough data. If more data
-    //   is supplied, we may be able to continue decoding this frame. We also treat this as
-    //   incomplete.
-    // FIXME: Ensure that we do not attempt to continue decoding if the method returns false and
-    // more data is supplied.
     bool frameDecoded = false;
-    if (!fReader->decode(frameIndex, &frameDecoded) || !frameDecoded) {
+    const bool fatalError = !fReader->decode(frameIndex, &frameDecoded);
+    if (fatalError || !frameDecoded) {
         if (rowsDecoded) {
             *rowsDecoded = fRowsDecoded;
         }
+        if (fatalError) {
+            return kErrorInInput;
+        }
         return kIncompleteInput;
     }
 
diff --git a/src/codec/SkSampledCodec.cpp b/src/codec/SkSampledCodec.cpp
index 2b6483b058..045f184f29 100644
--- a/src/codec/SkSampledCodec.cpp
+++ b/src/codec/SkSampledCodec.cpp
@@ -251,12 +251,12 @@ SkCodec::Result SkSampledCodec::sampledDecode(const SkImageInfo& info, void* pix
             if (incResult == SkCodec::kSuccess) {
                 return SkCodec::kSuccess;
             }
-            SkASSERT(incResult == SkCodec::kIncompleteInput);
+            SkASSERT(incResult == SkCodec::kIncompleteInput || incResult == SkCodec::kErrorInInput);
 
             SkASSERT(rowsDecoded <= info.height());
             this->codec()->fillIncompleteImage(info, pixels, rowBytes, options.fZeroInitialized,
                                                info.height(), rowsDecoded);
-            return SkCodec::kIncompleteInput;
+            return incResult;
         } else if (startResult != SkCodec::kUnimplemented) {
             return startResult;
         } // kUnimplemented means use the old method.
diff --git a/tests/CodecTest.cpp b/tests/CodecTest.cpp
index 04c5a088bd..7ec903728e 100644
--- a/tests/CodecTest.cpp
+++ b/tests/CodecTest.cpp
@@ -1450,7 +1450,7 @@ static void test_invalid_images(skiatest::Reporter* r, const char* path,
 
 DEF_TEST(Codec_InvalidImages, r) {
     // ASAN will complain if there is an issue.
-    test_invalid_images(r, "invalid_images/skbug5887.gif", SkCodec::kIncompleteInput);
+    test_invalid_images(r, "invalid_images/skbug5887.gif", SkCodec::kErrorInInput);
     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);