From ad178855775da18d7abfadce4b64825f3e32adbd Mon Sep 17 00:00:00 2001 From: Ryan Harrison Date: Thu, 3 May 2018 20:40:35 +0000 Subject: Invalidate GIF input buffer when moving file cursor backwards The current implementation of the GIF codec does not handle the file cursor moving backwards correctly. Specifically the input buffer that the data is being read into is not invalidated, so if the entirity of the buffer hasn't been consumed, a chunk of it will be moved to the front before reading in more data, which is just incorrect. Additionally, depending on the specific series of operations, it is possible that the buffer was allocated for more space then had been read into it and the uninitialized portion at the end is being copied to the beginning. The file cursor may move backwards when dealing with an animated gif or other image with multiple frames, since all of the control data is read in on load, and future calls specify what frame to fetch. The code has been changed to treat the input buffer as invalid when moving the cursor to a frame location, which will bypass any of the problematic unused saving behaviour. A call to std::min has been added to prevent allocation of an input buffer larger then the file size. Additionally this CL refactors GifReadMoreData to be clearer about what calculations are occuring, since the existing code reuses a number of vaguely named variables, making it difficult to follow. BUG=chromium:839348, chromium:839361 Change-Id: I2865658187bdf30bcad13ef4cac4f51a8966db11 Reviewed-on: https://pdfium-review.googlesource.com/32054 Reviewed-by: Henrique Nakashima Commit-Queue: Ryan Harrison --- core/fxcodec/codec/ccodec_progressivedecoder.h | 1 + core/fxcodec/codec/fx_codec_progress.cpp | 53 ++++++++++++++++---------- 2 files changed, 33 insertions(+), 21 deletions(-) diff --git a/core/fxcodec/codec/ccodec_progressivedecoder.h b/core/fxcodec/codec/ccodec_progressivedecoder.h index 4a67bbb335..a14ec0db29 100644 --- a/core/fxcodec/codec/ccodec_progressivedecoder.h +++ b/core/fxcodec/codec/ccodec_progressivedecoder.h @@ -242,6 +242,7 @@ class CCodec_ProgressiveDecoder : public CCodec_BmpModule::Delegate, FX_RECT m_GifFrameRect; bool m_BmpIsTopBottom; FXCODEC_STATUS m_status; + bool m_InvalidateGifBuffer; }; #endif // CORE_FXCODEC_CODEC_CCODEC_PROGRESSIVEDECODER_H_ diff --git a/core/fxcodec/codec/fx_codec_progress.cpp b/core/fxcodec/codec/fx_codec_progress.cpp index 9d25d3f140..e1aec22150 100644 --- a/core/fxcodec/codec/fx_codec_progress.cpp +++ b/core/fxcodec/codec/fx_codec_progress.cpp @@ -283,6 +283,7 @@ CCodec_ProgressiveDecoder::CCodec_ProgressiveDecoder( m_GifTransIndex = -1; m_GifFrameRect = FX_RECT(0, 0, 0, 0); m_BmpIsTopBottom = false; + m_InvalidateGifBuffer = true; } CCodec_ProgressiveDecoder::~CCodec_ProgressiveDecoder() { @@ -564,38 +565,45 @@ void CCodec_ProgressiveDecoder::PngFillScanlineBufCompleted(int pass, bool CCodec_ProgressiveDecoder::GifReadMoreData(CCodec_GifModule* pGifModule, FXCODEC_STATUS& err_status) { - uint32_t dwSize = (uint32_t)m_pFile->GetSize(); - if (dwSize <= m_offSet) { + if (static_cast(m_pFile->GetSize()) <= m_offSet) return false; - } - dwSize = dwSize - m_offSet; - uint32_t dwAvail = pGifModule->GetAvailInput(m_pGifContext.get(), nullptr); - if (dwAvail == m_SrcSize) { - if (dwSize > FXCODEC_BLOCK_SIZE) { - dwSize = FXCODEC_BLOCK_SIZE; - } - m_SrcSize = (dwSize + dwAvail + FXCODEC_BLOCK_SIZE - 1) / - FXCODEC_BLOCK_SIZE * FXCODEC_BLOCK_SIZE; + + uint32_t dwFileRemaining = m_pFile->GetSize() - m_offSet; + uint32_t dwUnusedBuffer = + !m_InvalidateGifBuffer + ? pGifModule->GetAvailInput(m_pGifContext.get(), nullptr) + : 0; + uint32_t dwAmountToFetchFromFile = dwFileRemaining; + if (dwUnusedBuffer == m_SrcSize) { + if (dwFileRemaining > FXCODEC_BLOCK_SIZE) + dwAmountToFetchFromFile = FXCODEC_BLOCK_SIZE; + m_SrcSize = std::min( + (dwAmountToFetchFromFile + dwUnusedBuffer + FXCODEC_BLOCK_SIZE - 1) / + FXCODEC_BLOCK_SIZE * FXCODEC_BLOCK_SIZE, + static_cast(m_pFile->GetSize())); m_pSrcBuf = FX_TryRealloc(uint8_t, m_pSrcBuf, m_SrcSize); if (!m_pSrcBuf) { err_status = FXCODEC_STATUS_ERR_MEMORY; return false; } } else { - uint32_t dwConsume = m_SrcSize - dwAvail; - if (dwAvail) { - memmove(m_pSrcBuf, m_pSrcBuf + dwConsume, dwAvail); - } - if (dwSize > dwConsume) { - dwSize = dwConsume; - } + uint32_t dwConsumed = m_SrcSize - dwUnusedBuffer; + if (dwUnusedBuffer) + memmove(m_pSrcBuf, m_pSrcBuf + dwConsumed, dwUnusedBuffer); + if (dwFileRemaining > dwConsumed) + dwAmountToFetchFromFile = dwConsumed; } - if (!m_pFile->ReadBlock(m_pSrcBuf + dwAvail, m_offSet, dwSize)) { + + if (!m_pFile->ReadBlock(m_pSrcBuf + dwUnusedBuffer, m_offSet, + dwAmountToFetchFromFile)) { err_status = FXCODEC_STATUS_ERR_READ; return false; } - m_offSet += dwSize; - pGifModule->Input(m_pGifContext.get(), m_pSrcBuf, dwSize + dwAvail); + + m_offSet += dwAmountToFetchFromFile; + pGifModule->Input(m_pGifContext.get(), m_pSrcBuf, + dwAmountToFetchFromFile + dwUnusedBuffer); + m_InvalidateGifBuffer = false; return true; } @@ -616,6 +624,8 @@ bool CCodec_ProgressiveDecoder::GifInputRecordPositionBuf( int32_t disposal_method, bool interlace) { m_offSet = rcd_pos; + m_InvalidateGifBuffer = true; + FXCODEC_STATUS error_status = FXCODEC_STATUS_ERROR; if (!GifReadMoreData(m_pCodecMgr->GetGifModule(), error_status)) { return false; @@ -1196,6 +1206,7 @@ bool CCodec_ProgressiveDecoder::PngDetectImageType(CFX_DIBAttribute* pAttribute, m_status = FXCODEC_STATUS_ERR_READ; return false; } + m_offSet += size; bResult = pPngModule->Input(m_pPngContext.get(), m_pSrcBuf, size, pAttribute); while (bResult) { -- cgit v1.2.3