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>
diff --git a/tests/CodecPartialTest.cpp b/tests/CodecPartialTest.cpp
index 9611d09..e29037d 100644
--- a/tests/CodecPartialTest.cpp
+++ b/tests/CodecPartialTest.cpp
@@ -209,7 +209,7 @@
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 @@
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);
+ }
+}