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 <tsepez@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>

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<CPDF_Stream> 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<CPDF_Dictionary> 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<CPDF_Document> const m_pDocument;
   MaybeOwned<CPDF_Stream> m_pStream;
-  MaybeOwned<CPDF_Dictionary> m_pDict;
   UnownedPtr<CPDF_Dictionary> 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<uint8_t, FxFreeDeleter>(
-        FX_Realloc(uint8_t, m_pData.Release().release(), h * m_nStride)));
+    m_pData.Reset(std::unique_ptr<uint8_t, FxFreeDeleter>(FX_Realloc(
+        uint8_t, m_pData.ReleaseAndClear().release(), h * m_nStride)));
   } else {
     uint8_t* pExternalBuffer = data();
     m_pData.Reset(std::unique_ptr<uint8_t, FxFreeDeleter>(
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<T, D> 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<T, D> Release() {
     ASSERT(IsOwned());
     return std::move(m_pOwnedObj);
   }
 
+  // Downgrades to empty, caller takes ownership.
+  std::unique_ptr<T, D> 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<T, D> m_pOwnedObj;
-  T* m_pObj;
+  UnownedPtr<T> m_pObj;
 };
 
 }  // namespace fxcrt