Reland "New GIF codec; new third_party/wuffs dep"
This reverts commit 7d1c9ec49f8a846cef833a0f956cf9c91a298145.
Bug: skia:8235
Change-Id: I830ba00a87e85c80f7e8583f5dfa105cd60029b2
Reviewed-on: https://skia-review.googlesource.com/c/165301
Commit-Queue: Leon Scroggins <scroggo@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: Leon Scroggins <scroggo@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
diff --git a/tests/CodecAnimTest.cpp b/tests/CodecAnimTest.cpp
index 6944d25..99ed876 100644
--- a/tests/CodecAnimTest.cpp
+++ b/tests/CodecAnimTest.cpp
@@ -30,6 +30,9 @@
if (!data) {
return;
}
+ // See also Codec_GifTruncated2 in GifTest.cpp for this magic 23.
+ //
+ // TODO: just move this getFrameInfo call to Codec_GifTruncated2?
SkCodec::MakeFromData(SkData::MakeSubset(data.get(), 0, 23))->getFrameInfo();
}
diff --git a/tests/CodecPartialTest.cpp b/tests/CodecPartialTest.cpp
index c5bb214..452ba85 100644
--- a/tests/CodecPartialTest.cpp
+++ b/tests/CodecPartialTest.cpp
@@ -380,13 +380,21 @@
REPORTER_ASSERT(r, result == SkCodec::kSuccess);
// Truncate to 23 bytes, just before the color map. This should fail to decode.
+ //
+ // See also Codec_GifTruncated2 in GifTest.cpp for this magic 23.
codec = SkCodec::MakeFromData(SkData::MakeWithoutCopy(gNoGlobalColorMap, 23));
REPORTER_ASSERT(r, codec);
if (codec) {
SkBitmap bm;
bm.allocPixels(info);
result = codec->getPixels(info, bm.getPixels(), bm.rowBytes());
+
+ // See the comments in Codec_GifTruncated2.
+#ifdef SK_HAS_WUFFS_LIBRARY
+ REPORTER_ASSERT(r, result == SkCodec::kIncompleteInput);
+#else
REPORTER_ASSERT(r, result == SkCodec::kInvalidInput);
+#endif
}
// Again, truncate to 23 bytes, this time for an incremental decode. We
@@ -399,11 +407,24 @@
SkBitmap bm;
bm.allocPixels(info);
result = codec->startIncrementalDecode(info, bm.getPixels(), bm.rowBytes());
+
+ // See the comments in Codec_GifTruncated2.
+#ifdef SK_HAS_WUFFS_LIBRARY
+ REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+
+ // Note that this is incrementalDecode, not startIncrementalDecode.
+ result = codec->incrementalDecode();
REPORTER_ASSERT(r, result == SkCodec::kIncompleteInput);
stream->addNewData(data->size());
+#else
+ REPORTER_ASSERT(r, result == SkCodec::kIncompleteInput);
+
+ // Note that this is startIncrementalDecode, not incrementalDecode.
+ stream->addNewData(data->size());
result = codec->startIncrementalDecode(info, bm.getPixels(), bm.rowBytes());
REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+#endif
result = codec->incrementalDecode();
REPORTER_ASSERT(r, result == SkCodec::kSuccess);
diff --git a/tests/CodecTest.cpp b/tests/CodecTest.cpp
index 225710c..4656d87 100644
--- a/tests/CodecTest.cpp
+++ b/tests/CodecTest.cpp
@@ -1419,6 +1419,74 @@
test_invalid_header(r, "invalid_images/b34778578.bmp");
}
+/*
+For the Codec_InvalidAnimated test, immediately below,
+resources/invalid_images/skbug6046.gif is:
+
+00000000: 4749 4638 3961 2000 0000 0000 002c ff00 GIF89a ......,..
+00000010: 7400 0600 0000 4001 0021 f904 0a00 0000 t.....@..!......
+00000020: 002c ff00 0000 ff00 7400 0606 0606 0601 .,......t.......
+00000030: 0021 f904 0000 0000 002c ff00 0000 ffcc .!.......,......
+00000040: 1b36 5266 deba 543d .6Rf..T=
+
+It nominally contains 3 frames, but all of them are invalid. It came from a
+fuzzer doing random mutations and copies. The breakdown:
+
+@000 6 bytes magic "GIF89a"
+@006 7 bytes Logical Screen Descriptor: 0x20 0x00 ... 0x00
+ - width = 32
+ - height = 0
+ - flags = 0x00
+ - background color index, pixel aspect ratio bytes ignored
+@00D 10 bytes Image Descriptor header: 0x2C 0xFF ... 0x40
+ - origin_x = 255
+ - origin_y = 116
+ - width = 6
+ - height = 0
+ - flags = 0x40, interlaced
+@017 2 bytes Image Descriptor body (pixel data): 0x01 0x00
+ - lit_width = 1, INVALID, OUTSIDE THE RANGE [2, 8]
+ - 0x00 byte means "end of data" for this frame
+@019 8 bytes Graphic Control Extension: 0x21 0xF9 ... 0x00
+ - valid, but irrelevant here.
+@021 10 bytes Image Descriptor header: 0x2C 0xFF ... 0x06
+ - origin_x = 255
+ - origin_y = 0
+ - width = 255
+ - height = 116
+ - flags = 0x06, INVALID, 0x80 BIT ZERO IMPLIES 0x07 BITS SHOULD BE ZERO
+@02B 14 bytes Image Descriptor body (pixel data): 0x06 0x06 ... 0x00
+ - lit_width = 6
+ - 0x06 precedes a 6 byte block of data
+ - 0x04 precedes a 4 byte block of data
+ - 0x00 byte means "end of data" for this frame
+@039 10 bytes Image Descriptor header: 0x2C 0xFF ... 0x06
+ - origin_x = 255
+ - origin_y = 0
+ - width = 52479
+ - height = 13851
+ - flags = 0x52, INVALID, 0x80 BIT ZERO IMPLIES 0x07 BITS SHOULD BE ZERO
+@043 5 bytes Image Descriptor body (pixel data): 0x66 0xDE ... unexpected-EOF
+ - lit_width = 102, INVALID, OUTSIDE THE RANGE [2, 8]
+ - 0xDE precedes a 222 byte block of data, INVALIDLY TRUNCATED
+
+On Image Descriptor flags INVALIDITY,
+https://www.w3.org/Graphics/GIF/spec-gif89a.txt section 20.c says that "Size of
+Local Color Table [the low 3 bits]... should be 0 if there is no Local Color
+Table specified [the high bit]."
+
+On LZW literal width (also known as Minimum Code Size) INVALIDITY outside of
+the range [2, 8], https://www.w3.org/Graphics/GIF/spec-gif89a.txt Appendix F
+says that "Normally this will be the same as the number of [palette index]
+bits. Because of some algorithmic constraints however, black & white images
+which have one color bit must be indicated as having a code size of 2."
+
+In practice, some GIF decoders, including the old third_party/gif code, don't
+enforce this. It says: "currentFrame->setDataSize(this->getOneByte())" with the
+only further check being against an upper bound of SK_MAX_DICTIONARY_ENTRY_BITS
+(the constant 12).
+*/
+
DEF_TEST(Codec_InvalidAnimated, r) {
// ASAN will complain if there is an issue.
auto path = "invalid_images/skbug6046.gif";
@@ -1444,6 +1512,36 @@
const auto reqFrame = frameInfos[i].fRequiredFrame;
opts.fPriorFrame = reqFrame == i - 1 ? reqFrame : SkCodec::kNoFrame;
auto result = codec->startIncrementalDecode(info, bm.getPixels(), bm.rowBytes(), &opts);
+
+#ifdef SK_HAS_WUFFS_LIBRARY
+ // We are transitioning from an old GIF implementation to a new (Wuffs)
+ // GIF implementation.
+ //
+ // This test (without SK_HAS_WUFFS_LIBRARY) is overly specific to the
+ // old implementation. As a fuzzer-discovered test, it's likely that
+ // what's fundamentally being tested isn't that decoding an invalid GIF
+ // leads to kSuccess, but that decoding an invalid GIF doesn't lead to
+ // an ASAN violation.
+ //
+ // Each of the 3 frames of the source GIF image is fundamentally
+ // invalid, as per the "breakdown" comment above. The old
+ // implementation is happy to call startIncrementalDecode 3 times. The
+ // new implementation is happy for the first two times, but on the 3rd,
+ // SkCodec::startIncrementalDecode calls SkCodec::handleFrameIndex
+ // which calls SkCodec::getPixels on the requiredFrame (the 0'th
+ // frame), and the new implementation subsequently hits the
+ // invalid-ness and returns kErrorInInput instead of kSuccess.
+ //
+ // Once the transition is complete, we can remove the #ifdef and delete
+ // the rest of the test function.
+ if (i == 2) {
+ if (result != SkCodec::kErrorInInput) {
+ ERRORF(r, "Unexpected result for decoding frame %i (out of %i) with error %i\n", i,
+ frameInfos.size(), result);
+ }
+ return;
+ }
+#endif
if (result != SkCodec::kSuccess) {
ERRORF(r, "Failed to start decoding frame %i (out of %i) with error %i\n", i,
frameInfos.size(), result);
@@ -1535,6 +1633,30 @@
test_info(r, codec.get(), codec->getInfo(), SkCodec::kInvalidInput, nullptr);
}
+/*
+For the Codec_ossfuzz6274 test, immediately below,
+resources/invalid_images/ossfuzz6274.gif is:
+
+00000000: 4749 4638 3961 2000 2000 f120 2020 2020 GIF89a . ..
+00000010: 2020 2020 2020 2020 2021 f903 ff20 2020 !...
+00000020: 002c 0000 0000 2000 2000 2000 00 .,.... . . ..
+
+@000 6 bytes magic "GIF89a"
+@006 7 bytes Logical Screen Descriptor: 0x20 0x00 ... 0x00
+ - width = 32
+ - height = 32
+ - flags = 0xF1, global color table, 4 RGB entries
+ - background color index, pixel aspect ratio bytes ignored
+@00D 12 bytes Color Table: 0x20 0x20 ... 0x20
+@019 20 bytes Graphic Control Extension: 0x21 0xF9 ... unexpected-EOF
+ - 0x03 precedes a 3 byte block of data, INVALID, MUST BE 4
+ - 0x20 precedes a 32 byte block of data, INVALIDly truncated
+
+https://www.w3.org/Graphics/GIF/spec-gif89a.txt section 23.c says that the
+block size (for an 0x21 0xF9 Graphic Control Extension) must be "the fixed
+value 4".
+*/
+
DEF_TEST(Codec_ossfuzz6274, r) {
if (GetResourcePath().isEmpty()) {
return;
@@ -1542,6 +1664,31 @@
const char* file = "invalid_images/ossfuzz6274.gif";
auto image = GetResourceAsImage(file);
+
+#ifdef SK_HAS_WUFFS_LIBRARY
+ // We are transitioning from an old GIF implementation to a new (Wuffs) GIF
+ // implementation.
+ //
+ // This test (without SK_HAS_WUFFS_LIBRARY) is overly specific to the old
+ // implementation. In the new implementation, the MakeFromStream factory
+ // method returns a nullptr SkImage*, instead of returning a non-null but
+ // otherwise all-transparent SkImage*.
+ //
+ // Either way, the end-to-end result is the same - the source input is
+ // rejected as an invalid GIF image - but the two implementations differ in
+ // how that's represented.
+ //
+ // Once the transition is complete, we can remove the #ifdef and delete the
+ // rest of the test function.
+ //
+ // See Codec_GifTruncated3 for the equivalent of the rest of the test
+ // function, on different (but still truncated) source data.
+ if (image) {
+ ERRORF(r, "Invalid data gave non-nullptr image");
+ }
+ return;
+#endif
+
if (!image) {
ERRORF(r, "Missing %s", file);
return;
diff --git a/tests/GifTest.cpp b/tests/GifTest.cpp
index fc61ffc..af2d303 100644
--- a/tests/GifTest.cpp
+++ b/tests/GifTest.cpp
@@ -9,6 +9,7 @@
#include "Resources.h"
#include "SkAndroidCodec.h"
#include "SkBitmap.h"
+#include "SkCanvas.h"
#include "SkData.h"
#include "SkImage.h"
#include "SkStream.h"
@@ -187,6 +188,20 @@
test_gif_data_no_colormap(reporter, static_cast<void *>(gGIFDataNoColormap),
sizeof(gGIFDataNoColormap));
+#ifdef SK_HAS_WUFFS_LIBRARY
+ // We are transitioning from an old GIF implementation to a new (Wuffs) GIF
+ // implementation.
+ //
+ // This test (without SK_HAS_WUFFS_LIBRARY) is overly specific to the old
+ // implementation. It claims that, for invalid (truncated) input, we can
+ // still 'decode' all of the pixels because no matter what palette index
+ // each pixel is, they're all equivalently transparent. It's not obvious
+ // that this off-spec behavior is worth preserving. Are real world users
+ // decoding truncated all-transparent GIF images??
+ //
+ // Once the transition is complete, we can remove the #ifdef and delete the
+ // #else branch.
+#else
// 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.
@@ -212,6 +227,7 @@
}
}
}
+#endif
// test short Gif. 80 is missing a few bytes.
test_gif_data_short(reporter, static_cast<void *>(gGIFData), 80);
@@ -261,26 +277,150 @@
REPORTER_ASSERT(r, !codec);
}
+/*
+For the Codec_GifTruncated2 test, immediately below,
+resources/images/box.gif's first 23 bytes are:
+
+00000000: 4749 4638 3961 c800 3700 203f 002c 0000 GIF89a..7. ?.,..
+00000010: 0000 c800 3700 85 ....7..
+
+The breakdown:
+
+@000 6 bytes magic "GIF89a"
+@006 7 bytes Logical Screen Descriptor: 0xC8 0x00 ... 0x00
+ - width = 200
+ - height = 55
+ - flags = 0x20
+ - background color index, pixel aspect ratio bytes ignored
+@00D 10 bytes Image Descriptor header: 0x2C 0x00 ... 0x85
+ - origin_x = 0
+ - origin_y = 0
+ - width = 200
+ - height = 55
+ - flags = 0x85, local color table, 64 RGB entries
+
+In particular, 23 bytes is after the header, but before the color table.
+*/
+
DEF_TEST(Codec_GifTruncated2, r) {
+ // Truncate box.gif at 21, 22 and 23 bytes.
+ //
+ // See also Codec_GifTruncated3 in this file, below.
+ //
+ // See also Codec_trunc in CodecAnimTest.cpp for this magic 23.
+ //
+ // See also Codec_GifPreMap in CodecPartialTest.cpp for this magic 23.
+ for (int i = 21; i < 24; i++) {
+ sk_sp<SkData> data(GetResourceAsData("images/box.gif"));
+ if (!data) {
+ return;
+ }
+
+ data = SkData::MakeSubset(data.get(), 0, i);
+ std::unique_ptr<SkCodec> codec(SkCodec::MakeFromData(data));
+
+ if (i <= 21) {
+ if (codec) {
+ ERRORF(r, "Invalid data gave non-nullptr codec");
+ }
+ return;
+ }
+
+ if (!codec) {
+ ERRORF(r, "Failed to create codec with partial data (truncated at %d)", i);
+ return;
+ }
+
+#ifdef SK_HAS_WUFFS_LIBRARY
+ // We are transitioning from an old GIF implementation to a new (Wuffs)
+ // GIF implementation.
+ //
+ // The input is truncated in the Image Descriptor, before the local
+ // color table, and before (21) or after (22, 23) the first frame's
+ // XYWH (left / top / width / height) can be decoded. A detailed
+ // breakdown of those 23 bytes is in a comment above this function.
+ //
+ // With the old implementation, this test claimed that "no frame is
+ // complete enough that it has its metadata". In terms of the
+ // underlying file format, this claim is true for truncating at 21
+ // bytes, but not true for 22 or 23.
+ //
+ // At 21 bytes, both the old and new implementation's MakeFromStream
+ // factory method returns a nullptr SkCodec*, because creating a
+ // SkCodec requires knowing the image width and height (as its
+ // constructor takes an SkEncodedInfo argument), and specifically for
+ // GIF, decoding the image width and height requires decoding the first
+ // frame's XYWH, as per
+ // https://raw.githubusercontent.com/google/wuffs/master/test/data/artificial/gif-frame-out-of-bounds.gif.make-artificial.txt
+ //
+ // At 22 or 23 bytes, the first frame is complete enough that we can
+ // fill in all of a SkCodec::FrameInfo's fields (other than
+ // fFullyReceived). Specifically, we can fill in fRequiredFrame and
+ // fAlphaType, even though we haven't yet decoded the frame's RGB
+ // palette entries, as we do know the frame rectangle and that every
+ // palette entry is fully opaque, due to the lack of a Graphic Control
+ // Extension before the Image Descriptor.
+ //
+ // The new implementation correctly reports that the first frame's
+ // metadata is complete enough. The old implementation does not.
+ //
+ // Once the transition is complete, we can remove the #ifdef and delete
+ // the #else code.
+ REPORTER_ASSERT(r, codec->getFrameCount() == 1);
+#else
+ // The old implementation claimed:
+ //
+ // Although we correctly created a codec, no frame is
+ // complete enough that it has its metadata. Returning 0
+ // ensures that Chromium will not try to create a frame
+ // too early.
+ REPORTER_ASSERT(r, codec->getFrameCount() == 0);
+#endif
+ }
+}
+
+#ifdef SK_HAS_WUFFS_LIBRARY
+// This tests that, after truncating the input, the pixels are still
+// zero-initialized. If you comment out the SkSampler::Fill call in
+// SkWuffsCodec::onStartIncrementalDecode, the test could still pass (in a
+// standard configuration) but should fail with the MSAN memory sanitizer.
+DEF_TEST(Codec_GifTruncated3, r) {
sk_sp<SkData> data(GetResourceAsData("images/box.gif"));
if (!data) {
return;
}
- // This is after the header, but before the color table.
data = SkData::MakeSubset(data.get(), 0, 23);
- std::unique_ptr<SkCodec> codec(SkCodec::MakeFromData(data));
- if (!codec) {
- ERRORF(r, "Failed to create codec with partial data");
+ sk_sp<SkImage> image(SkImage::MakeFromEncoded(data));
+
+ if (!image) {
+ ERRORF(r, "Missing image");
return;
}
- // Although we correctly created a codec, no frame is
- // complete enough that it has its metadata. Returning 0
- // ensures that Chromium will not try to create a frame
- // too early.
- REPORTER_ASSERT(r, codec->getFrameCount() == 0);
+ REPORTER_ASSERT(r, image->width() == 200);
+ REPORTER_ASSERT(r, image->height() == 55);
+
+ SkBitmap bm;
+ if (!bm.tryAllocPixels(SkImageInfo::MakeN32Premul(200, 55))) {
+ ERRORF(r, "Failed to allocate pixels");
+ return;
+ }
+
+ bm.eraseColor(SK_ColorTRANSPARENT);
+
+ SkCanvas canvas(bm);
+ canvas.drawImage(image, 0, 0, nullptr);
+
+ for (int i = 0; i < image->width(); ++i)
+ for (int j = 0; j < image->height(); ++j) {
+ SkColor actual = SkUnPreMultiply::PMColorToColor(*bm.getAddr32(i, j));
+ if (actual != SK_ColorTRANSPARENT) {
+ ERRORF(r, "did not initialize pixels! %i, %i is %x", i, j, actual);
+ }
+ }
}
+#endif
DEF_TEST(Codec_gif_out_of_palette, r) {
if (GetResourcePath().isEmpty()) {