[xfa] Fix image data positions in the GIF decoder
Fixes GIF decoder bugs related to image data positions. The following
issues are addressed:
1. The image data position should not add in the input buffer position.
ProgressiveDecoder::GifCurrentPosition() already accounts for the
input buffer position, so the defective code is double-counting it.
2. A GIF-specific code path in ProgressiveDecoder::ReadMoreData() tries
to replace the existing buffer contents with the bytes at the image
data position, but should also update the associated buffer pointers.
The fixed code moves the GIF-specific logic out of the common code
path as well.
Bug: pdfium:904, pdfium:1337
Change-Id: I9fdf844e49844c4850f56b9feaba90e92b1c3189
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/92410
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: K. Moon <kmoon@chromium.org>
diff --git a/core/fxcodec/gif/cfx_gifcontext.cpp b/core/fxcodec/gif/cfx_gifcontext.cpp
index 01babf6..6ce9570 100644
--- a/core/fxcodec/gif/cfx_gifcontext.cpp
+++ b/core/fxcodec/gif/cfx_gifcontext.cpp
@@ -27,10 +27,6 @@
CFX_GifContext::~CFX_GifContext() = default;
-uint32_t CFX_GifContext::CurrentPosition() const {
- return delegate_->GifCurrentPosition();
-}
-
void CFX_GifContext::ReadScanline(int32_t row_num, uint8_t* row_buf) {
delegate_->GifReadScanline(row_num, row_buf);
}
@@ -496,8 +492,7 @@
}
gif_image->code_exp = code_size;
- gif_image->data_pos = CurrentPosition();
- gif_image->data_pos += input_buffer_->GetPosition();
+ gif_image->data_pos = delegate_->GifCurrentPosition();
gif_image->image_GCE = nullptr;
if (graphic_control_extension_.get()) {
if (graphic_control_extension_->gce_flags.transparency) {
diff --git a/core/fxcodec/gif/cfx_gifcontext.h b/core/fxcodec/gif/cfx_gifcontext.h
index 6894da6..2130c8c 100644
--- a/core/fxcodec/gif/cfx_gifcontext.h
+++ b/core/fxcodec/gif/cfx_gifcontext.h
@@ -25,7 +25,6 @@
explicit CFX_GifContext(GifDecoder::Delegate* delegate);
~CFX_GifContext() override;
- uint32_t CurrentPosition() const;
void ReadScanline(int32_t row_num, uint8_t* row_buf);
bool GetRecordPosition(uint32_t cur_pos,
int32_t left,
diff --git a/core/fxcodec/progressive_decoder.cpp b/core/fxcodec/progressive_decoder.cpp
index 9612025..ad52068 100644
--- a/core/fxcodec/progressive_decoder.cpp
+++ b/core/fxcodec/progressive_decoder.cpp
@@ -371,11 +371,12 @@
int32_t trans_index,
bool interlace) {
m_offSet = rcd_pos;
- m_InvalidateGifBuffer = true;
FXCODEC_STATUS error_status = FXCODEC_STATUS::kError;
+ m_pCodecMemory->Seek(m_pCodecMemory->GetSize());
if (!GifReadMoreData(&error_status))
return false;
+ m_pCodecMemory->Seek(0);
CFX_GifPalette* pPalette = nullptr;
if (pal_num != 0 && pal_ptr) {
@@ -719,7 +720,7 @@
bool ProgressiveDecoder::BmpReadMoreData(
ProgressiveDecoderIface::Context* pContext,
FXCODEC_STATUS* err_status) {
- return ReadMoreData(BmpProgressiveDecoder::GetInstance(), pContext, false,
+ return ReadMoreData(BmpProgressiveDecoder::GetInstance(), pContext,
err_status);
}
@@ -761,12 +762,8 @@
#ifdef PDF_ENABLE_XFA_GIF
bool ProgressiveDecoder::GifReadMoreData(FXCODEC_STATUS* err_status) {
- if (!ReadMoreData(GifProgressiveDecoder::GetInstance(), m_pGifContext.get(),
- m_InvalidateGifBuffer, err_status)) {
- return false;
- }
- m_InvalidateGifBuffer = false;
- return true;
+ return ReadMoreData(GifProgressiveDecoder::GetInstance(), m_pGifContext.get(),
+ err_status);
}
bool ProgressiveDecoder::GifDetectImageTypeInBuffer() {
@@ -929,7 +926,7 @@
bool ProgressiveDecoder::JpegReadMoreData(FXCODEC_STATUS* err_status) {
return ReadMoreData(JpegProgressiveDecoder::GetInstance(),
- m_pJpegContext.get(), false, err_status);
+ m_pJpegContext.get(), err_status);
}
bool ProgressiveDecoder::JpegDetectImageTypeInBuffer(
@@ -1449,7 +1446,6 @@
bool ProgressiveDecoder::ReadMoreData(
ProgressiveDecoderIface* pModule,
ProgressiveDecoderIface::Context* pContext,
- bool invalidate_buffer,
FXCODEC_STATUS* err_status) {
// Check for EOF.
if (m_offSet >= static_cast<uint32_t>(m_pFile->GetSize()))
@@ -1460,13 +1456,10 @@
pdfium::base::checked_cast<uint32_t>(m_pFile->GetSize() - m_offSet);
// Figure out if the codec stopped processing midway through the buffer.
- size_t dwUnconsumed = 0;
- if (!invalidate_buffer) {
- FX_SAFE_SIZE_T avail_input = pModule->GetAvailInput(pContext);
- if (!avail_input.IsValid())
- return false;
- dwUnconsumed = avail_input.ValueOrDie();
- }
+ size_t dwUnconsumed;
+ FX_SAFE_SIZE_T avail_input = pModule->GetAvailInput(pContext);
+ if (!avail_input.AssignIfValid(&dwUnconsumed))
+ return false;
if (dwUnconsumed == m_pCodecMemory->GetSize()) {
// Codec couldn't make any progress against the bytes in the buffer.
diff --git a/core/fxcodec/progressive_decoder.h b/core/fxcodec/progressive_decoder.h
index f7bee03..72a5041 100644
--- a/core/fxcodec/progressive_decoder.h
+++ b/core/fxcodec/progressive_decoder.h
@@ -197,7 +197,6 @@
CFX_DIBAttribute* pAttribute);
bool ReadMoreData(ProgressiveDecoderIface* pModule,
ProgressiveDecoderIface::Context* pContext,
- bool invalidate_buffer,
FXCODEC_STATUS* err_status);
int GetDownScale();
@@ -265,7 +264,6 @@
int32_t m_GifPltNumber = 0;
int m_GifTransIndex = -1;
FX_RECT m_GifFrameRect;
- bool m_InvalidateGifBuffer = true;
#endif // PDF_ENABLE_XFA_GIF
#ifdef PDF_ENABLE_XFA_BMP
bool m_BmpIsTopBottom = false;
diff --git a/core/fxcodec/progressive_decoder_unittest.cpp b/core/fxcodec/progressive_decoder_unittest.cpp
index 7de25bf..4c1b2c4 100644
--- a/core/fxcodec/progressive_decoder_unittest.cpp
+++ b/core/fxcodec/progressive_decoder_unittest.cpp
@@ -4,9 +4,19 @@
#include "core/fxcodec/progressive_decoder.h"
+#include <stddef.h>
+#include <stdint.h>
+
+#include <memory>
+#include <tuple>
+
#include "core/fxcodec/fx_codec.h"
+#include "core/fxcodec/fx_codec_def.h"
#include "core/fxcrt/cfx_readonlymemorystream.h"
+#include "core/fxcrt/retain_ptr.h"
#include "core/fxge/dib/cfx_dibitmap.h"
+#include "core/fxge/dib/fx_dib.h"
+#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/base/span.h"
@@ -16,7 +26,85 @@
namespace fxcodec {
+namespace {
+
+using ::testing::ElementsAre;
+
+FXCODEC_STATUS DecodeToBitmap(ProgressiveDecoder& decoder,
+ const fxcrt::RetainPtr<CFX_DIBitmap>& bitmap) {
+ FXCODEC_STATUS status = decoder.StartDecode(bitmap, 0, 0, bitmap->GetWidth(),
+ bitmap->GetHeight());
+ while (status == FXCODEC_STATUS::kDecodeToBeContinued)
+ status = decoder.ContinueDecode();
+ return status;
+}
+
+} // namespace
+
#ifdef PDF_ENABLE_XFA_GIF
+TEST(ProgressiveDecoder, Gif87a) {
+ static constexpr uint8_t kInput[] = {
+ 0x47, 0x49, 0x46, 0x38, 0x37, 0x61, 0x01, 0x00, 0x01, 0x00, 0x80, 0x01,
+ 0x00, 0x40, 0x80, 0xc0, 0x80, 0x80, 0x80, 0x2c, 0x00, 0x00, 0x00, 0x00,
+ 0x01, 0x00, 0x01, 0x00, 0x00, 0x02, 0x02, 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::kDecodeFinished, status);
+ EXPECT_THAT(bitmap->GetScanline(0), ElementsAre(0xc0, 0x80, 0x40, 0xff));
+}
+
+TEST(ProgressiveDecoder, Gif89a) {
+ static constexpr uint8_t kInput[] = {
+ 0x47, 0x49, 0x46, 0x38, 0x39, 0x61, 0x01, 0x00, 0x01, 0x00, 0x80,
+ 0x01, 0x00, 0x40, 0x80, 0xc0, 0x80, 0x80, 0x80, 0x21, 0xf9, 0x04,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x2c, 0x00, 0x00, 0x00, 0x00, 0x01,
+ 0x00, 0x01, 0x00, 0x00, 0x02, 0x02, 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::kDecodeFinished, status);
+ EXPECT_THAT(bitmap->GetScanline(0), ElementsAre(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,
@@ -369,33 +457,28 @@
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
- {
- auto decoder = std::make_unique<ProgressiveDecoder>();
+ 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);
+ 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(98, decoder->GetWidth());
- ASSERT_EQ(6945, decoder->GetHeight());
+ ASSERT_EQ(98, decoder->GetWidth());
+ ASSERT_EQ(6945, decoder->GetHeight());
- auto bitmap = pdfium::MakeRetain<CFX_DIBitmap>();
- bitmap->Create(decoder->GetWidth(), decoder->GetHeight(),
- FXDIB_Format::kArgb);
+ 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);
+ size_t frames;
+ std::tie(status, frames) = decoder->GetFrames();
+ ASSERT_EQ(FXCODEC_STATUS::kDecodeReady, status);
+ ASSERT_EQ(1u, frames);
- status = decoder->StartDecode(bitmap, 0, 0, bitmap->GetWidth(),
- bitmap->GetHeight());
- while (status == FXCODEC_STATUS::kDecodeToBeContinued)
- status = decoder->ContinueDecode();
- EXPECT_EQ(FXCODEC_STATUS::kDecodeFinished, status);
- }
+ status = DecodeToBitmap(*decoder, bitmap);
+ EXPECT_EQ(FXCODEC_STATUS::kDecodeFinished, status);
}
#endif // PDF_ENABLE_XFA_GIF
