From e563e8352139e4852a955e319023b09f2844aee9 Mon Sep 17 00:00:00 2001 From: Tom Sepez Date: Tue, 30 Jan 2018 17:38:00 +0000 Subject: Use UnownedPtr instead of T* in MaybeOwned. Always check the liftime in the unowned case. Doing so unearthed the following issues: Transient lifetime issue in jbig2_image when doing realloc(). Stale (but unused) dictionary pointer in CPDF_Image. Destruction order in error branch in cpdf_dibsource.cpp Change-Id: I12b758aafeefedc7abe1e8b21a18db959929e95f Reviewed-on: https://pdfium-review.googlesource.com/24552 Commit-Queue: Tom Sepez Reviewed-by: dsinclair --- core/fpdfapi/page/cpdf_image.cpp | 26 ++++++++++---------------- core/fpdfapi/page/cpdf_image.h | 4 +--- core/fpdfapi/render/cpdf_dibsource.cpp | 2 +- core/fxcodec/jbig2/JBig2_Image.cpp | 4 ++-- core/fxcrt/maybe_owned.h | 22 ++++++++++++++++------ 5 files changed, 30 insertions(+), 28 deletions(-) diff --git a/core/fpdfapi/page/cpdf_image.cpp b/core/fpdfapi/page/cpdf_image.cpp index 65ca78e08e..5f82886a9b 100644 --- a/core/fpdfapi/page/cpdf_image.cpp +++ b/core/fpdfapi/page/cpdf_image.cpp @@ -35,33 +35,27 @@ CPDF_Image::CPDF_Image(CPDF_Document* pDoc) : m_pDocument(pDoc) {} CPDF_Image::CPDF_Image(CPDF_Document* pDoc, std::unique_ptr pStream) - : m_bIsInline(true), - m_pDocument(pDoc), - m_pStream(std::move(pStream)), - m_pDict(ToDictionary(m_pStream->GetDict()->Clone())) { + : m_bIsInline(true), m_pDocument(pDoc), m_pStream(std::move(pStream)) { ASSERT(m_pStream.IsOwned()); - ASSERT(m_pDict.IsOwned()); - FinishInitialization(); + FinishInitialization(m_pStream->GetDict()); } CPDF_Image::CPDF_Image(CPDF_Document* pDoc, uint32_t dwStreamObjNum) : m_pDocument(pDoc), - m_pStream(ToStream(pDoc->GetIndirectObject(dwStreamObjNum))), - m_pDict(m_pStream->GetDict()) { + m_pStream(ToStream(pDoc->GetIndirectObject(dwStreamObjNum))) { ASSERT(!m_pStream.IsOwned()); - ASSERT(!m_pDict.IsOwned()); - FinishInitialization(); + FinishInitialization(m_pStream->GetDict()); } CPDF_Image::~CPDF_Image() {} -void CPDF_Image::FinishInitialization() { - m_pOC = m_pDict->GetDictFor("OC"); +void CPDF_Image::FinishInitialization(CPDF_Dictionary* pDict) { + m_pOC = pDict->GetDictFor("OC"); m_bIsMask = - !m_pDict->KeyExist("ColorSpace") || m_pDict->GetIntegerFor("ImageMask"); - m_bInterpolate = !!m_pDict->GetIntegerFor("Interpolate"); - m_Height = m_pDict->GetIntegerFor("Height"); - m_Width = m_pDict->GetIntegerFor("Width"); + !pDict->KeyExist("ColorSpace") || pDict->GetIntegerFor("ImageMask"); + m_bInterpolate = !!pDict->GetIntegerFor("Interpolate"); + m_Height = pDict->GetIntegerFor("Height"); + m_Width = pDict->GetIntegerFor("Width"); } void CPDF_Image::ConvertStreamToIndirectObject() { diff --git a/core/fpdfapi/page/cpdf_image.h b/core/fpdfapi/page/cpdf_image.h index 23864bbf6c..57cbe94ca2 100644 --- a/core/fpdfapi/page/cpdf_image.h +++ b/core/fpdfapi/page/cpdf_image.h @@ -29,7 +29,6 @@ class CPDF_Image : public Retainable { void ConvertStreamToIndirectObject(); - CPDF_Dictionary* GetInlineDict() const { return m_pDict.Get(); } CPDF_Stream* GetStream() const { return m_pStream.Get(); } CPDF_Dictionary* GetDict() const; CPDF_Dictionary* GetOC() const { return m_pOC.Get(); } @@ -68,7 +67,7 @@ class CPDF_Image : public Retainable { CPDF_Image(CPDF_Document* pDoc, uint32_t dwStreamObjNum); ~CPDF_Image() override; - void FinishInitialization(); + void FinishInitialization(CPDF_Dictionary* pStreamDict); std::unique_ptr InitJPEG(uint8_t* pData, uint32_t size); int32_t m_Height = 0; @@ -78,7 +77,6 @@ class CPDF_Image : public Retainable { bool m_bInterpolate = false; UnownedPtr const m_pDocument; MaybeOwned m_pStream; - MaybeOwned m_pDict; UnownedPtr m_pOC; }; diff --git a/core/fpdfapi/render/cpdf_dibsource.cpp b/core/fpdfapi/render/cpdf_dibsource.cpp index aff63d102d..48715d2ecb 100644 --- a/core/fpdfapi/render/cpdf_dibsource.cpp +++ b/core/fpdfapi/render/cpdf_dibsource.cpp @@ -341,9 +341,9 @@ int CPDF_DIBSource::ContinueLoadDIBSource(IFX_PauseIndicator* pPause) { } if (iDecodeStatus < 0) { + m_pJbig2Context.reset(); m_pCachedBitmap.Reset(); m_pGlobalStream.Reset(); - m_pJbig2Context.reset(); return 0; } if (iDecodeStatus == FXCODEC_STATUS_DECODE_TOBECONTINUE) diff --git a/core/fxcodec/jbig2/JBig2_Image.cpp b/core/fxcodec/jbig2/JBig2_Image.cpp index b0d75d4d96..d229e0ca01 100644 --- a/core/fxcodec/jbig2/JBig2_Image.cpp +++ b/core/fxcodec/jbig2/JBig2_Image.cpp @@ -234,8 +234,8 @@ void CJBig2_Image::expand(int32_t h, bool v) { return; if (m_pData.IsOwned()) { - m_pData.Reset(std::unique_ptr( - FX_Realloc(uint8_t, m_pData.Release().release(), h * m_nStride))); + m_pData.Reset(std::unique_ptr(FX_Realloc( + uint8_t, m_pData.ReleaseAndClear().release(), h * m_nStride))); } else { uint8_t* pExternalBuffer = data(); m_pData.Reset(std::unique_ptr( diff --git a/core/fxcrt/maybe_owned.h b/core/fxcrt/maybe_owned.h index 11dd68642d..130d2bdc3c 100644 --- a/core/fxcrt/maybe_owned.h +++ b/core/fxcrt/maybe_owned.h @@ -11,6 +11,7 @@ #include "core/fxcrt/fx_memory.h" #include "core/fxcrt/fx_system.h" +#include "core/fxcrt/unowned_ptr.h" namespace fxcrt { @@ -33,25 +34,34 @@ class MaybeOwned { } void Reset(std::unique_ptr ptr) { + m_pObj = ptr.get(); m_pOwnedObj = std::move(ptr); - m_pObj = m_pOwnedObj.get(); } void Reset(T* ptr = nullptr) { - m_pOwnedObj.reset(); m_pObj = ptr; + m_pOwnedObj.reset(); } + T* Get() const { return m_pObj.Get(); } bool IsOwned() const { return !!m_pOwnedObj; } - T* Get() const { return m_pObj; } + + // Downgrades to unowned, caller takes ownership. std::unique_ptr Release() { ASSERT(IsOwned()); return std::move(m_pOwnedObj); } + // Downgrades to empty, caller takes ownership. + std::unique_ptr ReleaseAndClear() { + ASSERT(IsOwned()); + m_pObj = nullptr; + return std::move(m_pOwnedObj); + } + MaybeOwned& operator=(const MaybeOwned& that) = delete; MaybeOwned& operator=(MaybeOwned&& that) { - m_pOwnedObj = std::move(that.m_pOwnedObj); m_pObj = that.m_pObj; + m_pOwnedObj = std::move(that.m_pOwnedObj); that.m_pObj = nullptr; return *this; } @@ -78,11 +88,11 @@ class MaybeOwned { explicit operator bool() const { return !!m_pObj; } T& operator*() const { return *m_pObj; } - T* operator->() const { return m_pObj; } + T* operator->() const { return m_pObj.Get(); } private: std::unique_ptr m_pOwnedObj; - T* m_pObj; + UnownedPtr m_pObj; }; } // namespace fxcrt -- cgit v1.2.3