Fix SkGifCodec bugs around truncated data

Prior to this CL, if a GIF file was truncated before reading the local
color map of a frame, incremental decode would do the wrong thing. In
onStartIncrementalDecode, we would either create a color table based on
the global color map, or we would create a dummy one with only one
color (transparent). The dummy color table is correct if there is
neither a global nor a local color map, and allows us to fill the frame
with transparent. But if more data is provided, and it includes an
actual color map and image data, one of the following can happen:
- If the created color table is smaller than the actual one, the
  decoded data may include indices outside of the range of the created
  color table, resulting in a crash.
- If we get lucky, and the created color table is large enough, it may
  still be the wrong colors (and most likely is).

To solve this, make onStartIncrementalDecode fail if there is a local
color map that has not been read yet. A future call may read more data
and read the correct color map.

This is done by returning kIncompleteInput in
SkGifCodec::prepareToDecode if there is a local color map that has not
yet been read. (It is possible that there is no color map at all, in
which case we still need to support decoding that frame. Skip
attempting to decode in that case.)

In onGetPixels, if prepareToDecode returned kIncompleteInput, return
kInvalidInput. Although the input is technically incomplete, no future
call will provide more data (unlike in incremental decoding), and there
is nothing interesting for the client to draw. This also prevents
SkCodec from attempting to fill the data with an SkSwizzler, which has
not been created. (An alternative solution would be create the dummy
color table and an SkSwizzler, which would keep the current behavior.
But I think the new behavior of returning kInvalidInput makes more
sense.)

Add tests to verify the intended behavior:
- getPixels fails.
- startIncrementalDecode fails, but after providing more data it will
  succeed and incremental decoding matches the image decoded from the
  full stream.
- Both succeed if there is no color table at all.

Change-Id: Ifb52fe7f723673406a28e80c8805a552f0ac33b6
Reviewed-on: https://skia-review.googlesource.com/5758
Reviewed-by: Matt Sarett <msarett@google.com>
Commit-Queue: Leon Scroggins <scroggo@google.com>

4 files changed, 148 insertions, 15 deletions

diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp
index 31a3bc7059..197dcc1ec8 100644
--- a/dm/DMSrcSink.cpp
+++ b/dm/DMSrcSink.cpp
@@ -453,9 +453,14 @@ Error CodecSrc::draw(SkCanvas* canvas) const {
                 } else {
                     options.fHasPriorFrame = false;
                 }
-                const SkCodec::Result result = codec->getPixels(decodeInfo, pixels.get(),
-                                                                rowBytes, &options,
-                                                                colorPtr, &colorCount);
+                SkCodec::Result result = codec->getPixels(decodeInfo, pixels.get(),
+                                                          rowBytes, &options,
+                                                          colorPtr, &colorCount);
+                if (SkCodec::kInvalidInput == result && i > 0) {
+                    // Some of our test images have truncated later frames. Treat that
+                    // the same as incomplete.
+                    result = SkCodec::kIncompleteInput;
+                }
                 switch (result) {
                     case SkCodec::kSuccess:
                     case SkCodec::kIncompleteInput: {
diff --git a/src/codec/SkGifCodec.cpp b/src/codec/SkGifCodec.cpp
index d13f97ae72..9a5948b2d0 100644
--- a/src/codec/SkGifCodec.cpp
+++ b/src/codec/SkGifCodec.cpp
@@ -239,6 +239,14 @@ SkCodec::Result SkGifCodec::prepareToDecode(const SkImageInfo& dstInfo, SkPMColo
         return gif_error("frame index out of range!\n", kIncompleteInput);
     }
 
+    auto& localMap = fReader->frameContext(frameIndex)->localColorMap();
+    if (localMap.numColors() && !localMap.isDefined()) {
+        // We have parsed enough to know that there is a color map, but cannot
+        // parse the map itself yet. Exit now, so we do not build an incorrect
+        // table.
+        return gif_error("color map not available yet\n", kIncompleteInput);
+    }
+
     fTmpBuffer.reset(new uint8_t[dstInfo.minRowBytes()]);
 
     this->initializeColorTable(dstInfo, frameIndex);
@@ -296,8 +304,19 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
                                         int* inputColorCount,
                                         int* rowsDecoded) {
     Result result = this->prepareToDecode(dstInfo, inputColorPtr, inputColorCount, opts);
-    if (kSuccess != result) {
-        return result;
+    switch (result) {
+        case kSuccess:
+            break;
+        case kIncompleteInput:
+            // onStartIncrementalDecode treats this as incomplete, since it may
+            // provide more data later, but in this case, no more data will be
+            // provided, and there is nothing to draw. We also cannot return
+            // kIncompleteInput, which will make SkCodec attempt to fill
+            // remaining rows, but that requires an SkSwizzler, which we have
+            // not created.
+            return kInvalidInput;
+        default:
+            return result;
     }
 
     if (dstInfo.dimensions() != this->getInfo().dimensions()) {
@@ -426,6 +445,11 @@ SkCodec::Result SkGifCodec::decodeFrame(bool firstAttempt, const Options& opts,
         }
     }
 
+    if (!fCurrColorTableIsReal) {
+        // Nothing to draw this frame.
+        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
@@ -560,11 +584,9 @@ bool SkGifCodec::haveDecodedRow(size_t frameIndex, const unsigned char* rowBegin
         fRowsDecoded++;
     }
 
-    if (!fCurrColorTableIsReal) {
-        // No color table, so nothing to draw this frame.
-        // FIXME: We can abort even earlier - no need to decode this frame.
-        return true;
-    }
+    // decodeFrame will early exit if this is false, so this method will not be
+    // called.
+    SkASSERT(fCurrColorTableIsReal);
 
     // The swizzler takes care of offsetting into the dst width-wise.
     void* dstLine = SkTAddOffset<void>(fDst, dstRow * fDstRowBytes);
diff --git a/tests/CodecPartialTest.cpp b/tests/CodecPartialTest.cpp
index 9611d0982e..e29037dd14 100644
--- a/tests/CodecPartialTest.cpp
+++ b/tests/CodecPartialTest.cpp
@@ -209,7 +209,7 @@ DEF_TEST(Codec_partialAnim, r) {
         const SkCodec::Result result = fullCodec->getPixels(info, frame.getPixels(),
                 frame.rowBytes(), &opts, nullptr, nullptr);
 
-        if (result == SkCodec::kIncompleteInput) {
+        if (result == SkCodec::kIncompleteInput || result == SkCodec::kInvalidInput) {
             frameByteCounts.push_back(stream->getPosition() - lastOffset);
 
             // We need to distinguish between a partial frame and no more frames.
@@ -320,3 +320,75 @@ DEF_TEST(Codec_rewind, r) {
     test_interleaved(r, "plane_interlaced.png");
     test_interleaved(r, "box.gif");
 }
+
+// Modified version of the giflib logo, from
+// http://giflib.sourceforge.net/whatsinagif/bits_and_bytes.html
+// The global color map has been replaced with a local color map.
+static unsigned char gNoGlobalColorMap[] = {
+  // Header
+  0x47, 0x49, 0x46, 0x38, 0x39, 0x61,
+
+  // Logical screen descriptor
+  0x0A, 0x00, 0x0A, 0x00, 0x11, 0x00, 0x00,
+
+  // Image descriptor
+  0x2C, 0x00, 0x00, 0x00, 0x00, 0x0A, 0x00, 0x0A, 0x00, 0x81,
+
+  // Local color table
+  0xFF, 0xFF, 0xFF, 0xFF, 0x00, 0x00, 0x00, 0x00, 0xFF, 0x00, 0x00, 0x00,
+
+  // Image data
+  0x02, 0x16, 0x8C, 0x2D, 0x99, 0x87, 0x2A, 0x1C, 0xDC, 0x33, 0xA0, 0x02, 0x75,
+  0xEC, 0x95, 0xFA, 0xA8, 0xDE, 0x60, 0x8C, 0x04, 0x91, 0x4C, 0x01, 0x00,
+
+  // Trailer
+  0x3B,
+};
+
+// Test that a gif file truncated before its local color map behaves as expected.
+DEF_TEST(Codec_GifPreMap, r) {
+    sk_sp<SkData> data = SkData::MakeWithoutCopy(gNoGlobalColorMap, sizeof(gNoGlobalColorMap));
+    std::unique_ptr<SkCodec> codec(SkCodec::NewFromData(data));
+    if (!codec) {
+        ERRORF(r, "failed to create codec");
+        return;
+    }
+
+    SkBitmap truth;
+    auto info = standardize_info(codec.get());
+    truth.allocPixels(info);
+
+    auto result = codec->getPixels(info, truth.getPixels(), truth.rowBytes());
+    REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+
+    // Truncate to 23 bytes, just before the color map. This should fail to decode.
+    codec.reset(SkCodec::NewFromData(SkData::MakeWithoutCopy(gNoGlobalColorMap, 23)));
+    REPORTER_ASSERT(r, codec);
+    if (codec) {
+        SkBitmap bm;
+        bm.allocPixels(info);
+        result = codec->getPixels(info, bm.getPixels(), bm.rowBytes());
+        REPORTER_ASSERT(r, result == SkCodec::kInvalidInput);
+    }
+
+    // Again, truncate to 23 bytes, this time for an incremental decode. We
+    // cannot start an incremental decode until we have more data. If we did,
+    // we would be using the wrong color table.
+    HaltingStream* stream = new HaltingStream(data, 23);
+    codec.reset(SkCodec::NewFromStream(stream));
+    REPORTER_ASSERT(r, codec);
+    if (codec) {
+        SkBitmap bm;
+        bm.allocPixels(info);
+        result = codec->startIncrementalDecode(info, bm.getPixels(), bm.rowBytes());
+        REPORTER_ASSERT(r, result == SkCodec::kIncompleteInput);
+
+        stream->addNewData(data->size());
+        result = codec->startIncrementalDecode(info, bm.getPixels(), bm.rowBytes());
+        REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+
+        result = codec->incrementalDecode();
+        REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+        compare_bitmaps(r, truth, bm);
+    }
+}
diff --git a/tests/GifTest.cpp b/tests/GifTest.cpp
index 17dce92186..12bfb5348f 100644
--- a/tests/GifTest.cpp
+++ b/tests/GifTest.cpp
@@ -26,9 +26,18 @@ static unsigned char gGIFData[] = {
 };
 
 static unsigned char gGIFDataNoColormap[] = {
-  0x47, 0x49, 0x46, 0x38, 0x39, 0x61, 0x01, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00,
-  0x21, 0xf9, 0x04, 0x01, 0x0a, 0x00, 0x01, 0x00, 0x2c, 0x00, 0x00, 0x00, 0x00,
-  0x01, 0x00, 0x01, 0x00, 0x00, 0x02, 0x02, 0x4c, 0x01, 0x00, 0x3b
+  // Header
+  0x47, 0x49, 0x46, 0x38, 0x39, 0x61,
+  // Screen descriptor
+  0x01, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00,
+  // Graphics control extension
+  0x21, 0xf9, 0x04, 0x01, 0x0a, 0x00, 0x01, 0x00,
+  // Image descriptor
+  0x2c, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00,
+  // Image data
+  0x02, 0x02, 0x4c, 0x01, 0x00,
+  // Trailer
+  0x3b
 };
 
 static unsigned char gInterlacedGIF[] = {
@@ -177,7 +186,32 @@ DEF_TEST(Gif, reporter) {
 
     test_gif_data_no_colormap(reporter, static_cast<void *>(gGIFDataNoColormap),
                               sizeof(gGIFDataNoColormap));
-    // "libgif warning [missing colormap]"
+
+    // Since there is no color map, we do not even need to parse the image data
+    // to know that we should draw transparent. Truncate the file before the
+    // data. This should still succeed.
+    test_gif_data_no_colormap(reporter, static_cast<void *>(gGIFDataNoColormap), 31);
+
+    // Likewise, incremental decoding should succeed here.
+    {
+        sk_sp<SkData> data = SkData::MakeWithoutCopy(gGIFDataNoColormap, 31);
+        std::unique_ptr<SkCodec> codec(SkCodec::NewFromData(data));
+        REPORTER_ASSERT(reporter, codec);
+        if (codec) {
+            auto info = codec->getInfo().makeColorType(kN32_SkColorType);
+            SkBitmap bm;
+            bm.allocPixels(info);
+            REPORTER_ASSERT(reporter, SkCodec::kSuccess == codec->startIncrementalDecode(
+                    info, bm.getPixels(), bm.rowBytes()));
+            REPORTER_ASSERT(reporter, SkCodec::kSuccess == codec->incrementalDecode());
+            REPORTER_ASSERT(reporter, bm.width() == 1);
+            REPORTER_ASSERT(reporter, bm.height() == 1);
+            REPORTER_ASSERT(reporter, !(bm.empty()));
+            if (!(bm.empty())) {
+                REPORTER_ASSERT(reporter, bm.getColor(0, 0) == 0x00000000);
+            }
+        }
+    }
 
     // test short Gif.  80 is missing a few bytes.
     test_gif_data_short(reporter, static_cast<void *>(gGIFData), 80);