From 8d8d3bc54593d2d86054d59669b86a959ec0b602 Mon Sep 17 00:00:00 2001 From: Tom Sepez Date: Wed, 10 Oct 2018 17:53:50 +0000 Subject: Fix dangling reference in CFX_CodecMemory. Do this by making CFX_CodecMemory actually own the memory that it is ref-counting. Remove some test cases that are now prohibited, and relax one lifetime restriction in the test because we are now doing one additional copy (in the test, but not in real life). Bug:879512 Change-Id: If030dfcf97fe40155c46a42288fc73192437ce9c Reviewed-on: https://pdfium-review.googlesource.com/c/43670 Commit-Queue: Tom Sepez Reviewed-by: Lei Zhang --- core/fxcodec/codec/ccodec_progressivedecoder.cpp | 67 +++++++----------------- core/fxcodec/codec/ccodec_progressivedecoder.h | 3 +- core/fxcodec/codec/cfx_codec_memory.cpp | 27 ++++++++-- core/fxcodec/codec/cfx_codec_memory.h | 22 +++++--- core/fxcodec/gif/cfx_gifcontext_unittest.cpp | 45 ++++++---------- 5 files changed, 74 insertions(+), 90 deletions(-) (limited to 'core') diff --git a/core/fxcodec/codec/ccodec_progressivedecoder.cpp b/core/fxcodec/codec/ccodec_progressivedecoder.cpp index 07665c1d4a..9d375cbcf1 100644 --- a/core/fxcodec/codec/ccodec_progressivedecoder.cpp +++ b/core/fxcodec/codec/ccodec_progressivedecoder.cpp @@ -718,10 +718,7 @@ bool CCodec_ProgressiveDecoder::BmpDetectImageTypeInBuffer( std::unique_ptr pBmpContext = pBmpModule->Start(this); - pBmpModule->Input(pBmpContext.get(), - pdfium::MakeRetain( - pdfium::make_span(m_pSrcBuf.get(), m_SrcSize)), - nullptr); + pBmpModule->Input(pBmpContext.get(), m_pCodecMemory, nullptr); std::vector palette; int32_t readResult = pBmpModule->ReadHeader( @@ -866,10 +863,7 @@ bool CCodec_ProgressiveDecoder::GifDetectImageTypeInBuffer( return false; } m_pGifContext = pGifModule->Start(this); - pGifModule->Input(m_pGifContext.get(), - pdfium::MakeRetain( - pdfium::make_span(m_pSrcBuf.get(), m_SrcSize)), - nullptr); + pGifModule->Input(m_pGifContext.get(), m_pCodecMemory, nullptr); m_SrcComponents = 1; CFX_GifDecodeStatus readResult = pGifModule->ReadHeader( m_pGifContext.get(), &m_SrcWidth, &m_SrcHeight, &m_GifPltNumber, @@ -1052,10 +1046,8 @@ bool CCodec_ProgressiveDecoder::JpegDetectImageTypeInBuffer( m_status = FXCODEC_STATUS_ERR_MEMORY; return false; } - pJpegModule->Input(m_pJpegContext.get(), - pdfium::MakeRetain( - pdfium::make_span(m_pSrcBuf.get(), m_SrcSize)), - nullptr); + pJpegModule->Input(m_pJpegContext.get(), m_pCodecMemory, nullptr); + // Setting jump marker before calling ReadHeader, since a longjmp to // the marker indicates a fatal error. if (setjmp(*pJpegModule->GetJumpMark(m_pJpegContext.get())) == -1) { @@ -1264,12 +1256,7 @@ bool CCodec_ProgressiveDecoder::PngDetectImageTypeInBuffer( m_status = FXCODEC_STATUS_ERR_MEMORY; return false; } - bool bResult = - pPngModule->Input(m_pPngContext.get(), - pdfium::MakeRetain( - pdfium::make_span(m_pSrcBuf.get(), m_SrcSize)), - pAttribute); - while (bResult) { + while (pPngModule->Input(m_pPngContext.get(), m_pCodecMemory, pAttribute)) { uint32_t remain_size = static_cast(m_pFile->GetSize()) - m_offSet; uint32_t input_size = remain_size > FXCODEC_BLOCK_SIZE ? FXCODEC_BLOCK_SIZE : remain_size; @@ -1278,21 +1265,16 @@ bool CCodec_ProgressiveDecoder::PngDetectImageTypeInBuffer( m_status = FXCODEC_STATUS_ERR_FORMAT; return false; } - if (m_pSrcBuf && input_size > m_SrcSize) { - m_pSrcBuf.reset(FX_Alloc(uint8_t, input_size)); + if (m_pCodecMemory && input_size > m_SrcSize) { + m_pCodecMemory = pdfium::MakeRetain(input_size); m_SrcSize = input_size; } - bResult = m_pFile->ReadBlock(m_pSrcBuf.get(), m_offSet, input_size); - if (!bResult) { + if (!m_pFile->ReadBlock(m_pCodecMemory->GetBuffer(), m_offSet, + input_size)) { m_status = FXCODEC_STATUS_ERR_READ; return false; } m_offSet += input_size; - bResult = - pPngModule->Input(m_pPngContext.get(), - pdfium::MakeRetain( - pdfium::make_span(m_pSrcBuf.get(), input_size)), - pAttribute); } m_pPngContext.reset(); if (m_SrcPassNumber == 0) { @@ -1367,11 +1349,12 @@ FXCODEC_STATUS CCodec_ProgressiveDecoder::PngContinueDecode() { m_status = FXCODEC_STATUS_DECODE_FINISH; return m_status; } - if (m_pSrcBuf && input_size > m_SrcSize) { - m_pSrcBuf.reset(FX_Alloc(uint8_t, input_size)); + if (m_pCodecMemory && input_size > m_SrcSize) { + m_pCodecMemory = pdfium::MakeRetain(input_size); m_SrcSize = input_size; } - bool bResult = m_pFile->ReadBlock(m_pSrcBuf.get(), m_offSet, input_size); + bool bResult = + m_pFile->ReadBlock(m_pCodecMemory->GetBuffer(), m_offSet, input_size); if (!bResult) { m_pDeviceBitmap = nullptr; m_pFile = nullptr; @@ -1379,11 +1362,7 @@ FXCODEC_STATUS CCodec_ProgressiveDecoder::PngContinueDecode() { return m_status; } m_offSet += input_size; - bResult = - pPngModule->Input(m_pPngContext.get(), - pdfium::MakeRetain( - pdfium::make_span(m_pSrcBuf.get(), input_size)), - nullptr); + bResult = pPngModule->Input(m_pPngContext.get(), m_pCodecMemory, nullptr); if (!bResult) { m_pDeviceBitmap = nullptr; m_pFile = nullptr; @@ -1570,10 +1549,9 @@ bool CCodec_ProgressiveDecoder::DetectImageType(FXCODEC_IMAGE_TYPE imageType, size_t size = std::min(m_pFile->GetSize(), FXCODEC_BLOCK_SIZE); m_SrcSize = static_cast(size); - m_pSrcBuf.reset(FX_Alloc(uint8_t, m_SrcSize)); - + m_pCodecMemory = pdfium::MakeRetain(m_SrcSize); m_offSet = 0; - if (!m_pFile->ReadBlock(m_pSrcBuf.get(), m_offSet, m_SrcSize)) { + if (!m_pFile->ReadBlock(m_pCodecMemory->GetBuffer(), m_offSet, m_SrcSize)) { m_status = FXCODEC_STATUS_ERR_READ; return false; } @@ -1629,31 +1607,26 @@ bool CCodec_ProgressiveDecoder::ReadMoreData( dwBytesToFetchFromFile = std::min(dwBytesToFetchFromFile, FXCODEC_BLOCK_SIZE); uint32_t dwNewSize = m_SrcSize + dwBytesToFetchFromFile; - uint8_t* pNewBuf = FX_TryRealloc(uint8_t, m_pSrcBuf.release(), dwNewSize); - if (!pNewBuf) { + if (!m_pCodecMemory->TryResize(dwNewSize)) { err_status = FXCODEC_STATUS_ERR_MEMORY; return false; } m_SrcSize = dwNewSize; - m_pSrcBuf.reset(pNewBuf); } else { uint32_t dwConsumed = m_SrcSize - dwUnconsumed; - memmove(m_pSrcBuf.get(), m_pSrcBuf.get() + dwConsumed, dwUnconsumed); + m_pCodecMemory->Consume(dwConsumed); dwBytesToFetchFromFile = std::min(dwBytesToFetchFromFile, dwConsumed); m_SrcSize = dwBytesToFetchFromFile + dwUnconsumed; } // Append new data past the bytes not yet processed by the codec. - if (!m_pFile->ReadBlock(m_pSrcBuf.get() + dwUnconsumed, m_offSet, + if (!m_pFile->ReadBlock(m_pCodecMemory->GetBuffer() + dwUnconsumed, m_offSet, dwBytesToFetchFromFile)) { err_status = FXCODEC_STATUS_ERR_READ; return false; } m_offSet += dwBytesToFetchFromFile; - return pModule->Input(pContext, - pdfium::MakeRetain( - pdfium::make_span(m_pSrcBuf.get(), m_SrcSize)), - nullptr); + return pModule->Input(pContext, m_pCodecMemory, nullptr); } FXCODEC_STATUS CCodec_ProgressiveDecoder::LoadImageInfo( diff --git a/core/fxcodec/codec/ccodec_progressivedecoder.h b/core/fxcodec/codec/ccodec_progressivedecoder.h index 9821c67f4f..d0419f8268 100644 --- a/core/fxcodec/codec/ccodec_progressivedecoder.h +++ b/core/fxcodec/codec/ccodec_progressivedecoder.h @@ -251,8 +251,7 @@ class CCodec_ProgressiveDecoder : RetainPtr m_pFile; RetainPtr m_pDeviceBitmap; UnownedPtr m_pCodecMgr; - // |m_pSrcBuf| must outlive |m_pGifContext|. - std::unique_ptr m_pSrcBuf; + RetainPtr m_pCodecMemory; std::unique_ptr m_pDecodeBuf; std::unique_ptr m_pSrcPalette; std::unique_ptr m_pJpegContext; diff --git a/core/fxcodec/codec/cfx_codec_memory.cpp b/core/fxcodec/codec/cfx_codec_memory.cpp index b8cf97b098..640db12415 100644 --- a/core/fxcodec/codec/cfx_codec_memory.cpp +++ b/core/fxcodec/codec/cfx_codec_memory.cpp @@ -6,13 +6,13 @@ #include -CFX_CodecMemory::CFX_CodecMemory(pdfium::span buffer) - : buffer_(buffer) {} +CFX_CodecMemory::CFX_CodecMemory(size_t buffer_size) + : buffer_(FX_Alloc(uint8_t, buffer_size)), size_(buffer_size) {} CFX_CodecMemory::~CFX_CodecMemory() = default; bool CFX_CodecMemory::Seek(size_t pos) { - if (pos > buffer_.size()) + if (pos > size_) return false; pos_ = pos; @@ -23,8 +23,25 @@ size_t CFX_CodecMemory::ReadBlock(void* buffer, size_t size) { if (!buffer || !size || IsEOF()) return 0; - size_t bytes_to_read = std::min(size, buffer_.size() - pos_); - memcpy(buffer, &buffer_[pos_], bytes_to_read); + size_t bytes_to_read = std::min(size, size_ - pos_); + memcpy(buffer, buffer_.get() + pos_, bytes_to_read); pos_ += bytes_to_read; return bytes_to_read; } + +bool CFX_CodecMemory::TryResize(size_t new_buffer_size) { + uint8_t* pOldBuf = buffer_.release(); + uint8_t* pNewBuf = FX_TryRealloc(uint8_t, pOldBuf, new_buffer_size); + if (!pNewBuf) { + buffer_.reset(pOldBuf); + return false; + } + buffer_.reset(pNewBuf); + size_ = new_buffer_size; + return true; +} + +void CFX_CodecMemory::Consume(size_t consumed) { + size_t unconsumed = size_ - consumed; + memmove(buffer_.get(), buffer_.get() + consumed, unconsumed); +} diff --git a/core/fxcodec/codec/cfx_codec_memory.h b/core/fxcodec/codec/cfx_codec_memory.h index 0d11d41de8..fa26e240ec 100644 --- a/core/fxcodec/codec/cfx_codec_memory.h +++ b/core/fxcodec/codec/cfx_codec_memory.h @@ -5,6 +5,9 @@ #ifndef CORE_FXCODEC_CODEC_CFX_CODEC_MEMORY_H_ #define CORE_FXCODEC_CODEC_CFX_CODEC_MEMORY_H_ +#include + +#include "core/fxcrt/fx_memory.h" #include "core/fxcrt/retain_ptr.h" #include "third_party/base/span.h" @@ -13,21 +16,28 @@ class CFX_CodecMemory final : public Retainable { template friend RetainPtr pdfium::MakeRetain(Args&&... args); - pdfium::span GetSpan() { return buffer_; } - uint8_t* GetBuffer() { return buffer_.data(); } - size_t GetSize() const { return buffer_.size(); } + pdfium::span GetSpan() { return {buffer_.get(), size_}; } + uint8_t* GetBuffer() { return buffer_.get(); } + size_t GetSize() const { return size_; } size_t GetPosition() const { return pos_; } - bool IsEOF() const { return pos_ >= buffer_.size(); } + bool IsEOF() const { return pos_ >= size_; } size_t ReadBlock(void* buffer, size_t size); // Sets the cursor position to |pos| if possible. bool Seek(size_t pos); + // Try to change the size of the buffer, keep the old one on failure. + bool TryResize(size_t new_buffer_size); + + // Schlep the bytes down the buffer. + void Consume(size_t consumed); + private: - explicit CFX_CodecMemory(pdfium::span buffer); + explicit CFX_CodecMemory(size_t buffer_size); ~CFX_CodecMemory() override; - pdfium::span const buffer_; + std::unique_ptr buffer_; + size_t size_ = 0; size_t pos_ = 0; }; diff --git a/core/fxcodec/gif/cfx_gifcontext_unittest.cpp b/core/fxcodec/gif/cfx_gifcontext_unittest.cpp index 24b8acd769..9b081a4183 100644 --- a/core/fxcodec/gif/cfx_gifcontext_unittest.cpp +++ b/core/fxcodec/gif/cfx_gifcontext_unittest.cpp @@ -4,6 +4,8 @@ #include "core/fxcodec/gif/cfx_gifcontext.h" +#include + #include "testing/gtest/include/gtest/gtest.h" class CFX_GifContextForTest final : public CFX_GifContext { @@ -19,49 +21,33 @@ class CFX_GifContextForTest final : public CFX_GifContext { CFX_CodecMemory* InputBuffer() const { return input_buffer_.Get(); } void SetTestInputBuffer(pdfium::span input) { - SetInputBuffer(pdfium::MakeRetain(input)); + auto pMemory = pdfium::MakeRetain(input.size()); + memcpy(pMemory->GetBuffer(), input.data(), input.size()); + SetInputBuffer(std::move(pMemory)); } }; TEST(CFX_GifContext, SetInputBuffer) { uint8_t buffer[] = {0x00, 0x01, 0x02}; - { - // Context must not outlive its buffers. - CFX_GifContextForTest context(nullptr, nullptr); - - context.SetTestInputBuffer({nullptr, 0}); - EXPECT_EQ(nullptr, context.InputBuffer()->GetBuffer()); - EXPECT_EQ(0u, context.InputBuffer()->GetSize()); - EXPECT_EQ(0u, context.InputBuffer()->GetPosition()); - - context.SetTestInputBuffer({nullptr, 100}); - EXPECT_EQ(nullptr, context.InputBuffer()->GetBuffer()); - EXPECT_EQ(100u, context.InputBuffer()->GetSize()); - EXPECT_EQ(0u, context.InputBuffer()->GetPosition()); + CFX_GifContextForTest context(nullptr, nullptr); - context.SetTestInputBuffer({buffer, 0}); - EXPECT_EQ(buffer, context.InputBuffer()->GetBuffer()); - EXPECT_EQ(0u, context.InputBuffer()->GetSize()); - EXPECT_EQ(0u, context.InputBuffer()->GetPosition()); + context.SetTestInputBuffer({nullptr, 0}); + EXPECT_EQ(0u, context.InputBuffer()->GetSize()); + EXPECT_EQ(0u, context.InputBuffer()->GetPosition()); - context.SetTestInputBuffer({buffer, 3}); - EXPECT_EQ(buffer, context.InputBuffer()->GetBuffer()); - EXPECT_EQ(3u, context.InputBuffer()->GetSize()); - EXPECT_EQ(0u, context.InputBuffer()->GetPosition()); + context.SetTestInputBuffer({buffer, 0}); + EXPECT_EQ(0u, context.InputBuffer()->GetSize()); + EXPECT_EQ(0u, context.InputBuffer()->GetPosition()); - context.SetTestInputBuffer({buffer, 100}); - EXPECT_EQ(buffer, context.InputBuffer()->GetBuffer()); - EXPECT_EQ(100u, context.InputBuffer()->GetSize()); - EXPECT_EQ(0u, context.InputBuffer()->GetPosition()); - } + context.SetTestInputBuffer({buffer, 3}); + EXPECT_EQ(3u, context.InputBuffer()->GetSize()); + EXPECT_EQ(0u, context.InputBuffer()->GetPosition()); } TEST(CFX_GifContext, ReadAllOrNone) { std::vector dest_buffer; uint8_t src_buffer[] = {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09}; - { - // Context must not outlive its buffers. CFX_GifContextForTest context(nullptr, nullptr); context.SetTestInputBuffer({nullptr, 0}); @@ -94,7 +80,6 @@ TEST(CFX_GifContext, ReadAllOrNone) { EXPECT_TRUE(context.ReadAllOrNone(dest_buffer.data(), 1)); EXPECT_EQ(src_buffer[i], dest_buffer[0]); } - } } TEST(CFX_GifContext, ReadGifSignature) { -- cgit v1.2.3