[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
