[xfa] Fix GIF decode continuation
Fixes the GIF decoder to continue decoding across various boundaries.
The old logic only works for a single scanline within a single image
data sub-block, which obviously doesn't hold for real GIF images.
Critically, the existing logic fails to recover from source buffer
underflow and destination buffer overflow correctly. To fix this:
1. LZWDecompressor::Decode() doesn't discard previously-decoded state.
2. ProgressiveDecoder incrementally feeds LZWDecompressor new data.
Note that ProgressiveDecoder.BUG_895009 does not contain a valid GIF,
which the fixed decoder correctly fails to decode; the test expectations
have been updated accordingly.
Fixed: pdfium:904, pdfium:1337
Change-Id: I79db5c8962ff40a9b47e680f5e34e62950c1d495
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/92390
Auto-Submit: K. Moon <kmoon@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
diff --git a/core/fxcodec/gif/cfx_gifcontext.cpp b/core/fxcodec/gif/cfx_gifcontext.cpp
index 6ce9570..a377998 100644
--- a/core/fxcodec/gif/cfx_gifcontext.cpp
+++ b/core/fxcodec/gif/cfx_gifcontext.cpp
@@ -213,6 +213,8 @@
std::vector<uint8_t, FxAllocAllocator<uint8_t>> img_data;
size_t read_marker = input_buffer_->GetPosition();
+ // TODO(crbug.com/pdfium/1793): This logic can be simplified a lot, but it
+ // probably makes more sense to switch to a different GIF decoder altogether.
if (decode_status_ == GIF_D_STATUS_IMG_DATA) {
if (!ReadAllOrNone(&img_data_size, sizeof(img_data_size)))
return GifDecoder::Status::kUnfinished;
@@ -224,20 +226,21 @@
return GifDecoder::Status::kUnfinished;
}
- if (!lzw_decompressor_.get()) {
+ if (!lzw_decompressor_) {
lzw_decompressor_ = LZWDecompressor::Create(GetPaletteExp(gif_image),
gif_image->code_exp);
+ if (!lzw_decompressor_) {
+ DecodingFailureAtTailCleanup(gif_image);
+ return GifDecoder::Status::kError;
+ }
}
+ lzw_decompressor_->SetSource(img_data.data(), img_data_size);
+
SaveDecodingStatus(GIF_D_STATUS_IMG_DATA);
img_row_offset_ += img_row_avail_size_;
img_row_avail_size_ = gif_img_row_bytes - img_row_offset_;
- LZWDecompressor::Status ret =
- lzw_decompressor_.get()
- ? lzw_decompressor_->Decode(
- img_data.data(), img_data_size,
- gif_image->row_buffer.data() + img_row_offset_,
- &img_row_avail_size_)
- : LZWDecompressor::Status::kError;
+ LZWDecompressor::Status ret = lzw_decompressor_->Decode(
+ gif_image->row_buffer.data() + img_row_offset_, &img_row_avail_size_);
if (ret == LZWDecompressor::Status::kError) {
DecodingFailureAtTailCleanup(gif_image);
return GifDecoder::Status::kError;
@@ -263,19 +266,14 @@
return GifDecoder::Status::kUnfinished;
}
- if (!lzw_decompressor_.get()) {
- lzw_decompressor_ = LZWDecompressor::Create(
- GetPaletteExp(gif_image), gif_image->code_exp);
- }
+ lzw_decompressor_->SetSource(img_data.data(), img_data_size);
+
SaveDecodingStatus(GIF_D_STATUS_IMG_DATA);
img_row_offset_ += img_row_avail_size_;
img_row_avail_size_ = gif_img_row_bytes - img_row_offset_;
- ret = lzw_decompressor_.get()
- ? lzw_decompressor_->Decode(
- img_data.data(), img_data_size,
- gif_image->row_buffer.data() + img_row_offset_,
- &img_row_avail_size_)
- : LZWDecompressor::Status::kError;
+ ret = lzw_decompressor_->Decode(
+ gif_image->row_buffer.data() + img_row_offset_,
+ &img_row_avail_size_);
}
}
@@ -295,18 +293,15 @@
} else {
ReadScanline(gif_image->row_num++, gif_image->row_buffer.data());
}
+
img_row_offset_ = 0;
img_row_avail_size_ = gif_img_row_bytes;
- ret = lzw_decompressor_.get()
- ? lzw_decompressor_->Decode(
- img_data.data(), img_data_size,
- gif_image->row_buffer.data() + img_row_offset_,
- &img_row_avail_size_)
- : LZWDecompressor::Status::kError;
+ ret = lzw_decompressor_->Decode(
+ gif_image->row_buffer.data() + img_row_offset_,
+ &img_row_avail_size_);
}
- if (ret == LZWDecompressor::Status::kInsufficientDestSize ||
- ret == LZWDecompressor::Status::kError) {
+ if (ret == LZWDecompressor::Status::kError) {
DecodingFailureAtTailCleanup(gif_image);
return GifDecoder::Status::kError;
}
diff --git a/core/fxcodec/gif/lzw_decompressor.cpp b/core/fxcodec/gif/lzw_decompressor.cpp
index 50469aa..8325ce2 100644
--- a/core/fxcodec/gif/lzw_decompressor.cpp
+++ b/core/fxcodec/gif/lzw_decompressor.cpp
@@ -33,25 +33,28 @@
: code_size_(code_exp),
code_color_end_(static_cast<uint16_t>(1 << (color_exp + 1))),
code_clear_(static_cast<uint16_t>(1 << code_exp)),
- code_end_(static_cast<uint16_t>((1 << code_exp) + 1)) {}
+ code_end_(static_cast<uint16_t>((1 << code_exp) + 1)) {
+ ClearTable();
+}
LZWDecompressor::~LZWDecompressor() = default;
-LZWDecompressor::Status LZWDecompressor::Decode(const uint8_t* src_buf,
- uint32_t src_size,
- uint8_t* dest_buf,
+void LZWDecompressor::SetSource(const uint8_t* src_buf, uint32_t src_size) {
+ next_in_ = src_buf;
+ avail_in_ = src_size;
+}
+
+LZWDecompressor::Status LZWDecompressor::Decode(uint8_t* dest_buf,
uint32_t* dest_size) {
- if (!src_buf || src_size == 0 || !dest_buf || !dest_size)
+ if (!next_in_ || !dest_buf || !dest_size)
return Status::kError;
+ if (avail_in_ == 0)
+ return Status::kUnfinished;
+
if (*dest_size == 0)
return Status::kInsufficientDestSize;
- next_in_ = src_buf;
- avail_in_ = src_size;
-
- ClearTable();
-
uint32_t i = 0;
if (decompressed_next_ != 0) {
uint32_t extracted_size = ExtractData(dest_buf, *dest_size);
diff --git a/core/fxcodec/gif/lzw_decompressor.h b/core/fxcodec/gif/lzw_decompressor.h
index 390cedb..b99b473 100644
--- a/core/fxcodec/gif/lzw_decompressor.h
+++ b/core/fxcodec/gif/lzw_decompressor.h
@@ -34,10 +34,8 @@
uint8_t code_exp);
~LZWDecompressor();
- Status Decode(const uint8_t* src_buf,
- uint32_t src_size,
- uint8_t* dest_buf,
- uint32_t* dest_size);
+ void SetSource(const uint8_t* src_buf, uint32_t src_size);
+ Status Decode(uint8_t* dest_buf, uint32_t* dest_size);
// Used by unittests, should not be called in production code.
uint32_t ExtractDataForTest(uint8_t* dest_buf, uint32_t dest_size) {
diff --git a/core/fxcodec/gif/lzw_decompressor_unittest.cpp b/core/fxcodec/gif/lzw_decompressor_unittest.cpp
index 99e186e..f14fa14 100644
--- a/core/fxcodec/gif/lzw_decompressor_unittest.cpp
+++ b/core/fxcodec/gif/lzw_decompressor_unittest.cpp
@@ -4,9 +4,15 @@
#include "core/fxcodec/gif/lzw_decompressor.h"
+#include <stdint.h>
+#include <string.h>
+
+#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/base/cxx17_backports.h"
+using ::testing::ElementsAreArray;
+
TEST(LZWDecompressor, CreateBadParams) {
EXPECT_EQ(nullptr, LZWDecompressor::Create(0x10, 0x02));
EXPECT_EQ(nullptr, LZWDecompressor::Create(0x04, 0x0F));
@@ -91,21 +97,23 @@
uint8_t output_data[10];
uint32_t output_size = pdfium::size(output_data);
- EXPECT_EQ(
- LZWDecompressor::Status::kError,
- decompressor->Decode(nullptr, image_size, output_data, &output_size));
+ decompressor->SetSource(nullptr, image_size);
EXPECT_EQ(LZWDecompressor::Status::kError,
- decompressor->Decode(image_data, 0, output_data, &output_size));
- EXPECT_EQ(
- LZWDecompressor::Status::kError,
- decompressor->Decode(image_data, image_size, nullptr, &output_size));
+ decompressor->Decode(output_data, &output_size));
+
+ decompressor->SetSource(image_data, 0);
+ EXPECT_EQ(LZWDecompressor::Status::kUnfinished,
+ decompressor->Decode(output_data, &output_size));
+
+ decompressor->SetSource(image_data, image_size);
EXPECT_EQ(LZWDecompressor::Status::kError,
- decompressor->Decode(image_data, image_size, output_data, nullptr));
+ decompressor->Decode(nullptr, &output_size));
+ EXPECT_EQ(LZWDecompressor::Status::kError,
+ decompressor->Decode(output_data, nullptr));
output_size = 0;
- EXPECT_EQ(
- LZWDecompressor::Status::kInsufficientDestSize,
- decompressor->Decode(image_data, image_size, output_data, &output_size));
+ EXPECT_EQ(LZWDecompressor::Status::kInsufficientDestSize,
+ decompressor->Decode(output_data, &output_size));
}
TEST(LZWDecompressor, Decode1x1SingleColour) {
@@ -122,9 +130,9 @@
memset(output_data, 0, sizeof(output_data));
uint32_t output_size = pdfium::size(output_data);
- EXPECT_EQ(
- LZWDecompressor::Status::kSuccess,
- decompressor->Decode(image_data, image_size, output_data, &output_size));
+ decompressor->SetSource(image_data, image_size);
+ EXPECT_EQ(LZWDecompressor::Status::kSuccess,
+ decompressor->Decode(output_data, &output_size));
EXPECT_EQ(pdfium::size(output_data), output_size);
EXPECT_TRUE(0 == memcmp(expected_data, output_data, sizeof(expected_data)));
@@ -154,9 +162,9 @@
memset(output_data, 0, sizeof(output_data));
uint32_t output_size = pdfium::size(output_data);
- EXPECT_EQ(
- LZWDecompressor::Status::kSuccess,
- decompressor->Decode(kImageData, image_size, output_data, &output_size));
+ decompressor->SetSource(kImageData, image_size);
+ EXPECT_EQ(LZWDecompressor::Status::kSuccess,
+ decompressor->Decode(output_data, &output_size));
EXPECT_EQ(pdfium::size(output_data), output_size);
EXPECT_TRUE(0 == memcmp(kExpectedData, output_data, sizeof(kExpectedData)));
@@ -188,14 +196,39 @@
memset(output_data, 0, sizeof(output_data));
uint32_t output_size = pdfium::size(output_data);
- EXPECT_EQ(
- LZWDecompressor::Status::kSuccess,
- decompressor->Decode(kImageData, image_size, output_data, &output_size));
+ decompressor->SetSource(kImageData, image_size);
+ EXPECT_EQ(LZWDecompressor::Status::kSuccess,
+ decompressor->Decode(output_data, &output_size));
EXPECT_EQ(pdfium::size(output_data), output_size);
EXPECT_TRUE(0 == memcmp(kExpectedData, output_data, sizeof(kExpectedData)));
}
+TEST(LZWDecompressor, MultipleDecodes) {
+ auto decompressor = LZWDecompressor::Create(/*color_exp=*/0, /*code_exp=*/2);
+ ASSERT_NE(nullptr, decompressor);
+
+ static constexpr uint8_t kImageData[] = {0x84, 0x6f, 0x05};
+ decompressor->SetSource(kImageData, pdfium::size(kImageData));
+
+ static constexpr uint8_t kExpectedScanline[] = {0x00, 0x00, 0x00, 0x00};
+ uint8_t output_data[pdfium::size(kExpectedScanline)];
+
+ memset(output_data, 0xFF, sizeof(output_data));
+ uint32_t output_size = pdfium::size(output_data);
+ EXPECT_EQ(LZWDecompressor::Status::kInsufficientDestSize,
+ decompressor->Decode(output_data, &output_size));
+ EXPECT_EQ(pdfium::size(kExpectedScanline), output_size);
+ EXPECT_THAT(output_data, ElementsAreArray(kExpectedScanline));
+
+ memset(output_data, 0xFF, sizeof(output_data));
+ output_size = pdfium::size(output_data);
+ EXPECT_EQ(LZWDecompressor::Status::kSuccess,
+ decompressor->Decode(output_data, &output_size));
+ EXPECT_EQ(pdfium::size(kExpectedScanline), output_size);
+ EXPECT_THAT(output_data, ElementsAreArray(kExpectedScanline));
+}
+
TEST(LZWDecompressor, HandleColourCodeOutOfPalette) {
uint8_t palette_exp = 0x2; // Image uses 10 colours, so the palette exp
// should be 3, 2^(3+1) = 16 colours.
@@ -213,7 +246,7 @@
memset(output_data, 0, sizeof(output_data));
uint32_t output_size = pdfium::size(output_data);
- EXPECT_EQ(
- LZWDecompressor::Status::kError,
- decompressor->Decode(kImageData, image_size, output_data, &output_size));
+ decompressor->SetSource(kImageData, image_size);
+ EXPECT_EQ(LZWDecompressor::Status::kError,
+ decompressor->Decode(output_data, &output_size));
}
diff --git a/core/fxcodec/progressive_decoder_unittest.cpp b/core/fxcodec/progressive_decoder_unittest.cpp
index 4c1b2c4..8902f6b 100644
--- a/core/fxcodec/progressive_decoder_unittest.cpp
+++ b/core/fxcodec/progressive_decoder_unittest.cpp
@@ -105,6 +105,117 @@
EXPECT_THAT(bitmap->GetScanline(0), ElementsAre(0xc0, 0x80, 0x40, 0xff));
}
+TEST(ProgressiveDecoder, GifInsufficientCodeSize) {
+ // This GIF causes `LZWDecompressor::Create()` to fail because the minimum
+ // code size is too small for the palette.
+ static constexpr uint8_t kInput[] = {
+ 0x47, 0x49, 0x46, 0x38, 0x37, 0x61, 0x01, 0x00, 0x01, 0x00, 0x82,
+ 0x01, 0x00, 0x40, 0x80, 0xc0, 0x80, 0x80, 0x80, 0x81, 0x81, 0x81,
+ 0x82, 0x82, 0x82, 0x83, 0x83, 0x83, 0x84, 0x84, 0x84, 0x85, 0x85,
+ 0x85, 0x86, 0x86, 0x86, 0x2c, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00,
+ 0x01, 0x00, 0x00, 0x02, 0x2, 0x44, 0x01, 0x00, 0x3b};
+
+ auto decoder = std::make_unique<ProgressiveDecoder>();
+
+ auto source = pdfium::MakeRetain<CFX_ReadOnlyMemoryStream>(kInput);
+ CFX_DIBAttribute attr;
+ FXCODEC_STATUS status =
+ decoder->LoadImageInfo(source, FXCODEC_IMAGE_GIF, &attr, true);
+ ASSERT_EQ(FXCODEC_STATUS::kFrameReady, status);
+
+ ASSERT_EQ(1, decoder->GetWidth());
+ ASSERT_EQ(1, decoder->GetHeight());
+
+ auto bitmap = pdfium::MakeRetain<CFX_DIBitmap>();
+ bitmap->Create(decoder->GetWidth(), decoder->GetHeight(),
+ FXDIB_Format::kArgb);
+
+ size_t frames;
+ std::tie(status, frames) = decoder->GetFrames();
+ ASSERT_EQ(FXCODEC_STATUS::kDecodeReady, status);
+ ASSERT_EQ(1u, frames);
+
+ status = DecodeToBitmap(*decoder, bitmap);
+ EXPECT_EQ(FXCODEC_STATUS::kError, status);
+}
+
+TEST(ProgressiveDecoder, GifDecodeAcrossScanlines) {
+ // This GIF contains an LZW code unit split across 2 scanlines. The decoder
+ // must continue decoding the second scanline using the residual data.
+ static constexpr uint8_t kInput[] = {
+ 0x47, 0x49, 0x46, 0x38, 0x37, 0x61, 0x04, 0x00, 0x02, 0x00, 0x80, 0x01,
+ 0x00, 0x40, 0x80, 0xc0, 0x80, 0x80, 0x80, 0x2c, 0x00, 0x00, 0x00, 0x00,
+ 0x04, 0x00, 0x02, 0x00, 0x00, 0x02, 0x03, 0x84, 0x6f, 0x05, 0x00, 0x3b};
+
+ auto decoder = std::make_unique<ProgressiveDecoder>();
+
+ auto source = pdfium::MakeRetain<CFX_ReadOnlyMemoryStream>(kInput);
+ CFX_DIBAttribute attr;
+ FXCODEC_STATUS status =
+ decoder->LoadImageInfo(source, FXCODEC_IMAGE_GIF, &attr, true);
+ ASSERT_EQ(FXCODEC_STATUS::kFrameReady, status);
+
+ ASSERT_EQ(4, decoder->GetWidth());
+ ASSERT_EQ(2, decoder->GetHeight());
+
+ auto bitmap = pdfium::MakeRetain<CFX_DIBitmap>();
+ bitmap->Create(decoder->GetWidth(), decoder->GetHeight(),
+ FXDIB_Format::kArgb);
+
+ size_t frames;
+ std::tie(status, frames) = decoder->GetFrames();
+ ASSERT_EQ(FXCODEC_STATUS::kDecodeReady, status);
+ ASSERT_EQ(1u, frames);
+
+ status = DecodeToBitmap(*decoder, bitmap);
+ EXPECT_EQ(FXCODEC_STATUS::kDecodeFinished, status);
+ EXPECT_THAT(bitmap->GetScanline(0),
+ ElementsAre(0xc0, 0x80, 0x40, 0xff, 0xc0, 0x80, 0x40, 0xff, 0xc0,
+ 0x80, 0x40, 0xff, 0xc0, 0x80, 0x40, 0xff));
+ EXPECT_THAT(bitmap->GetScanline(1),
+ ElementsAre(0xc0, 0x80, 0x40, 0xff, 0xc0, 0x80, 0x40, 0xff, 0xc0,
+ 0x80, 0x40, 0xff, 0xc0, 0x80, 0x40, 0xff));
+}
+
+TEST(ProgressiveDecoder, GifDecodeAcrossSubblocks) {
+ // This GIF contains a scanline split across 2 data sub-blocks. The decoder
+ // must continue decoding in the second sub-block.
+ static constexpr uint8_t kInput[] = {
+ 0x47, 0x49, 0x46, 0x38, 0x37, 0x61, 0x04, 0x00, 0x02, 0x00,
+ 0x80, 0x01, 0x00, 0x40, 0x80, 0xc0, 0x80, 0x80, 0x80, 0x2c,
+ 0x00, 0x00, 0x00, 0x00, 0x04, 0x00, 0x02, 0x00, 0x00, 0x02,
+ 0x02, 0x84, 0x6f, 0x01, 0x05, 0x00, 0x3b};
+
+ auto decoder = std::make_unique<ProgressiveDecoder>();
+
+ auto source = pdfium::MakeRetain<CFX_ReadOnlyMemoryStream>(kInput);
+ CFX_DIBAttribute attr;
+ FXCODEC_STATUS status =
+ decoder->LoadImageInfo(source, FXCODEC_IMAGE_GIF, &attr, true);
+ ASSERT_EQ(FXCODEC_STATUS::kFrameReady, status);
+
+ ASSERT_EQ(4, decoder->GetWidth());
+ ASSERT_EQ(2, decoder->GetHeight());
+
+ auto bitmap = pdfium::MakeRetain<CFX_DIBitmap>();
+ bitmap->Create(decoder->GetWidth(), decoder->GetHeight(),
+ FXDIB_Format::kArgb);
+
+ size_t frames;
+ std::tie(status, frames) = decoder->GetFrames();
+ ASSERT_EQ(FXCODEC_STATUS::kDecodeReady, status);
+ ASSERT_EQ(1u, frames);
+
+ status = DecodeToBitmap(*decoder, bitmap);
+ EXPECT_EQ(FXCODEC_STATUS::kDecodeFinished, status);
+ EXPECT_THAT(bitmap->GetScanline(0),
+ ElementsAre(0xc0, 0x80, 0x40, 0xff, 0xc0, 0x80, 0x40, 0xff, 0xc0,
+ 0x80, 0x40, 0xff, 0xc0, 0x80, 0x40, 0xff));
+ EXPECT_THAT(bitmap->GetScanline(1),
+ ElementsAre(0xc0, 0x80, 0x40, 0xff, 0xc0, 0x80, 0x40, 0xff, 0xc0,
+ 0x80, 0x40, 0xff, 0xc0, 0x80, 0x40, 0xff));
+}
+
TEST(ProgressiveDecoder, BUG_895009) {
static constexpr uint8_t kInput[] = {
0x47, 0x49, 0x46, 0x38, 0x39, 0x61, 0x62, 0x00, 0x21, 0x1b, 0x27, 0x01,
@@ -478,7 +589,7 @@
ASSERT_EQ(1u, frames);
status = DecodeToBitmap(*decoder, bitmap);
- EXPECT_EQ(FXCODEC_STATUS::kDecodeFinished, status);
+ EXPECT_EQ(FXCODEC_STATUS::kError, status);
}
#endif // PDF_ENABLE_XFA_GIF
diff --git a/testing/SUPPRESSIONS b/testing/SUPPRESSIONS
index 24ca5c0..0784cb3 100644
--- a/testing/SUPPRESSIONS
+++ b/testing/SUPPRESSIONS
@@ -386,8 +386,6 @@
xfa_example.in win * * *
# TODO(pdfium:1354): Remove after associated bug is fixed
xfa_example.in * * * skia,skiapaths
-# TODO(pdfium:1167): Remove after associated bug is fixed
-xfa_gif_image.in * * * *
# TODO(pdfium:1743): Remove after associated bug is fixed
xfa_node_caption.pdf * * * skia,skiapaths
# TODO(pdfium:1095): Remove after associated bug is fixed
diff --git a/testing/fuzzers/pdf_lzw_fuzzer.cc b/testing/fuzzers/pdf_lzw_fuzzer.cc
index f019b66..1292ec0 100644
--- a/testing/fuzzers/pdf_lzw_fuzzer.cc
+++ b/testing/fuzzers/pdf_lzw_fuzzer.cc
@@ -2,6 +2,9 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include <stddef.h>
+#include <stdint.h>
+
#include <vector>
#include "core/fxcodec/gif/lzw_decompressor.h"
@@ -15,7 +18,7 @@
static constexpr size_t kMaxFuzzBytes = 1024 * 1024 * 1024; // 1 GB.
void LZWFuzz(const uint8_t* src_buf,
- size_t src_size,
+ uint32_t src_size,
uint8_t color_exp,
uint8_t code_exp) {
std::unique_ptr<LZWDecompressor> decompressor =
@@ -29,9 +32,9 @@
// This cast should be safe since the caller is checking for overflow on
// the initial data.
uint32_t dest_size = static_cast<uint32_t>(dest_buf.size());
+ decompressor->SetSource(src_buf, src_size);
if (LZWDecompressor::Status::kInsufficientDestSize !=
- decompressor->Decode(src_buf, static_cast<uint32_t>(src_size),
- dest_buf.data(), &dest_size)) {
+ decompressor->Decode(dest_buf.data(), &dest_size)) {
return;
}
}
diff --git a/testing/resources/pixel/xfa_specific/bug_1337_expected.pdf.0.png b/testing/resources/pixel/xfa_specific/bug_1337_expected.pdf.0.png
index 08c11b0..c0d8380 100644
--- a/testing/resources/pixel/xfa_specific/bug_1337_expected.pdf.0.png
+++ b/testing/resources/pixel/xfa_specific/bug_1337_expected.pdf.0.png
Binary files differ
