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>

1 files changed, 29 insertions, 7 deletions

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);