commit 31dd5e3153cff586092453312a64dae7b29a8ad5
author Lei Zhang <thestig@chromium.org> Sat Feb 02 00:37:25 2019 +0000
committer Chromium commit bot <commit-bot@chromium.org> Sat Feb 02 00:37:25 2019 +0000
tree 6718fb0395a6da3826584719a671fef4aeb142c9
parent 301b83141c48399f481d4bb5966436dd7894ce9d

Change |CPDF_Annot::m_pPopupAnnot| to UnownedPtr.

Fix destruction order to prevent dangling pointers.

Change-Id: I460e51681388a629cbd75afe994b8957b9922ab7
Reviewed-on: https://pdfium-review.googlesource.com/c/49690
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>

core/fpdfdoc/cpdf_annot.h

diff --git a/core/fpdfdoc/cpdf_annot.h b/core/fpdfdoc/cpdf_annot.h
index c6999db..d4c2dd1 100644
--- a/core/fpdfdoc/cpdf_annot.h
+++ b/core/fpdfdoc/cpdf_annot.h
@@ -104,7 +104,7 @@
                   const CPDF_RenderOptions* pOptions);
   CPDF_Form* GetAPForm(const CPDF_Page* pPage, AppearanceMode mode);
   void SetOpenState(bool bOpenState) { m_bOpenState = bOpenState; }
-  CPDF_Annot* GetPopupAnnot() const { return m_pPopupAnnot; }
+  CPDF_Annot* GetPopupAnnot() const { return m_pPopupAnnot.Get(); }
   void SetPopupAnnot(CPDF_Annot* pAnnot) { m_pPopupAnnot = pAnnot; }
 
  private:
@@ -119,13 +119,12 @@
   UnownedPtr<CPDF_Document> const m_pDocument;
   CPDF_Annot::Subtype m_nSubtype;
   std::map<CPDF_Stream*, std::unique_ptr<CPDF_Form>> m_APMap;
+  // If non-null, then this is not a popup annotation.
+  UnownedPtr<CPDF_Annot> m_pPopupAnnot;
   // |m_bOpenState| is only set for popup annotations.
   bool m_bOpenState = false;
   bool m_bHasGeneratedAP;
   bool m_bIsTextMarkupAnnotation;
-  // Not owned. If there is a valid pointer in |m_pPopupAnnot|,
-  // then this annot is never a popup.
-  CPDF_Annot* m_pPopupAnnot = nullptr;
 };
 
 // Get the AP in an annotation dict for a given appearance mode.

core/fpdfdoc/cpdf_annotlist.cpp

diff --git a/core/fpdfdoc/cpdf_annotlist.cpp b/core/fpdfdoc/cpdf_annotlist.cpp
index c37c9ef..5d22beb 100644
--- a/core/fpdfdoc/cpdf_annotlist.cpp
+++ b/core/fpdfdoc/cpdf_annotlist.cpp
@@ -196,8 +196,8 @@
     }
   }
 
-  size_t nAnnotListSize = m_AnnotList.size();
-  for (size_t i = 0; i < nAnnotListSize; ++i) {
+  m_nAnnotCount = m_AnnotList.size();
+  for (size_t i = 0; i < m_nAnnotCount; ++i) {
     std::unique_ptr<CPDF_Annot> pPopupAnnot =
         CreatePopupAnnot(m_pDocument.Get(), pPage, m_AnnotList[i].get());
     if (pPopupAnnot)
@@ -205,7 +205,16 @@
   }
 }
 
-CPDF_AnnotList::~CPDF_AnnotList() = default;
+CPDF_AnnotList::~CPDF_AnnotList() {
+  // Move the pop-up annotations out of |m_AnnotList| into |popups|. Then
+  // destroy |m_AnnotList| first. This prevents dangling pointers to the pop-up
+  // annotations.
+  size_t nPopupCount = m_AnnotList.size() - m_nAnnotCount;
+  std::vector<std::unique_ptr<CPDF_Annot>> popups(nPopupCount);
+  for (size_t i = 0; i < nPopupCount; ++i)
+    popups[i] = std::move(m_AnnotList[m_nAnnotCount + i]);
+  m_AnnotList.clear();
+}
 
 void CPDF_AnnotList::DisplayPass(CPDF_Page* pPage,
                                  CFX_RenderDevice* pDevice,

core/fpdfdoc/cpdf_annotlist.h

diff --git a/core/fpdfdoc/cpdf_annotlist.h b/core/fpdfdoc/cpdf_annotlist.h
index 26cd96c..49a00e5 100644
--- a/core/fpdfdoc/cpdf_annotlist.h
+++ b/core/fpdfdoc/cpdf_annotlist.h
@@ -59,7 +59,11 @@
                    FX_RECT* clip_rect);
 
   UnownedPtr<CPDF_Document> const m_pDocument;
+
+  // The first |m_nAnnotCount| elements are from the PDF itself. The rest are
+  // generated pop-up annotations.
   std::vector<std::unique_ptr<CPDF_Annot>> m_AnnotList;
+  size_t m_nAnnotCount = 0;
 };
 
 #endif  // CORE_FPDFDOC_CPDF_ANNOTLIST_H_