Lock page in LoadFXAnnot to prevent unintended page closing

fpdfsdk/src/fsdk_mgr.cpp

 // Copyright 2014 PDFium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
  
 // Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
 
 #include "../include/fsdk_define.h"
 #include "../include/fsdk_mgr.h"
 #include "../include/fpdf_ext.h"
 #include "../include/formfiller/FFL_FormFiller.h"
@@ skipping 314 matching lines @@
 }
 
 CPDFSDK_Document::CPDFSDK_Document(CPDF_Document* pDoc,CPDFDoc_Environment* pEnv):m_pDoc(pDoc),
                                                 m_pInterForm(NULL),m_pEnv(pEnv),m_pOccontent(NULL),m_bChangeMask(FALSE)
 {
         m_pFocusAnnot = NULL;
 }
 
 CPDFSDK_Document::~CPDFSDK_Document()
 {
-        m_pageMap.RemoveAll();
+        FX_POSITION pos = m_pageMap.GetStartPosition();
+        while (pos) {
+            CPDF_Page* pPage = NULL;
+            CPDFSDK_PageView* pPageView = NULL;
+            m_pageMap.GetNextAssoc(pos, pPage, pPageView);
+            delete pPageView;
+        }
+        m_pageMap.RemoveAll();
         if(m_pInterForm)
         {
                 m_pInterForm->Destroy();
                 m_pInterForm = NULL;
         }
         if(m_pOccontent)
         {
                 delete m_pOccontent;
                 m_pOccontent = NULL;
         }
@@ skipping 100 matching lines @@
 CPDF_OCContext* CPDFSDK_Document::GetOCContext()
 {
         if(!m_pOccontent)
                 m_pOccontent = new CPDF_OCContext(m_pDoc);
         return m_pOccontent;
 }
 
 void CPDFSDK_Document::ReMovePageView(CPDF_Page* pPDFPage)
 {
         CPDFSDK_PageView* pPageView = (CPDFSDK_PageView*)m_pageMap.GetValueAt(pPDFPage);
-        if(pPageView)
+        if(pPageView && !pPageView->IsLocked())
         {
                 delete pPageView;
                 m_pageMap.RemoveKey(pPDFPage);
         }
 }
 
 CPDF_Page * CPDFSDK_Document::GetPage(int nIndex)
 {
         CPDF_Page * pTempPage = (CPDF_Page*)m_pEnv->FFI_GetPage(m_pDoc,nIndex);
         if(!pTempPage)
@@ skipping 134 matching lines @@
 
 
 CPDFSDK_PageView::CPDFSDK_PageView(CPDFSDK_Document* pSDKDoc,CPDF_Page* page):m_page(page),m_pSDKDoc(pSDKDoc)
 {
         CPDFSDK_InterForm* pInterForm = pSDKDoc->GetInterForm();
         if(pInterForm)
         {
                 CPDF_InterForm* pPDFInterForm = pInterForm->GetInterForm();
                 pPDFInterForm->FixPageFields(page);
         }
-
+        m_page->SetPrivateData((FX_LPVOID)m_page, (FX_LPVOID)this, NULL);
         m_fxAnnotArray.RemoveAll();
 
         m_bEnterWidget = FALSE;
         m_bExitWidget = FALSE;
         m_bOnWidget = FALSE;
         m_CaptureWidget = NULL;
         m_bValid = FALSE;
+        m_bLocked = FALSE;
+        m_bTakeOverPage = FALSE;
 }
 
 CPDFSDK_PageView::~CPDFSDK_PageView()
 {
         CPDFDoc_Environment* pEnv = m_pSDKDoc->GetEnv();        
         int nAnnotCount = m_fxAnnotArray.GetSize();
+
         for (int i=0; i<nAnnotCount; i++)
         {
                 CPDFSDK_Annot* pAnnot = (CPDFSDK_Annot*)m_fxAnnotArray.GetAt(i);
                 //if there is a focused annot on the page, we should kill the focus first.
                 if(pAnnot == m_pSDKDoc->GetFocusAnnot())
                         KillFocusAnnot();
                 CPDFSDK_AnnotHandlerMgr* pAnnotHandlerMgr = pEnv->GetAnnotHandlerMgr();
                 ASSERT(pAnnotHandlerMgr);
                 pAnnotHandlerMgr->ReleaseAnnot(pAnnot);
         }
         m_fxAnnotArray.RemoveAll();
         if(m_pAnnotList)
         {
                 delete m_pAnnotList;
                 m_pAnnotList = NULL;
         }
+        m_page->RemovePrivateData((FX_LPVOID)m_page);
+        if(m_bTakeOverPage) {
+            delete m_page;
+        }
 }
 
 void CPDFSDK_PageView::PageView_OnDraw(CFX_RenderDevice* pDevice, CPDF_Matrix* pUser2Device,CPDF_RenderOptions* pOptions)
 {
         m_curMatrix = *pUser2Device;
         
         //      m_pAnnotList->DisplayAnnots(m_page, pDevice, pUser2Device, FALSE, pOptions);
         CPDFDoc_Environment* pEnv = m_pSDKDoc->GetEnv();
         CPDFSDK_AnnotIterator annotIterator(this, TRUE);
         CPDFSDK_Annot * pSDKAnnot=NULL;
@@ skipping 311 matching lines @@
 
 extern void CheckUnSupportAnnot(CPDF_Document * pDoc, CPDF_Annot* pPDFAnnot);
 
 void CPDFSDK_PageView::LoadFXAnnots()
 {
         CPDFDoc_Environment* pEnv = m_pSDKDoc->GetEnv();
 
         FX_BOOL enableAPUpdate = CPDF_InterForm::UpdatingAPEnabled();
         //Disable the default AP construction.
         CPDF_InterForm::EnableUpdateAP(FALSE);
         m_pAnnotList = new CPDF_AnnotList(m_page);

[Tom Sepez, 2014/09/24 17:02:11]

I saw this function getting re-entered during some tracing.  If this happens,
m_pAnnotList gets overwritten with a new one, and strange things may happen
below.  It looks like this is still possible to be re-entered.  Could you add
some asserts here (e.g. static int depth = 0; if (depth) { /* re-entered */ }
depth++; .... existing code ... depth--;) and see if that is still true when you
run the testcase in the bug?

Even if we don't have a memory safety issue, we'll leak m_pAnnotList if
re-entered.

         CPDF_InterForm::EnableUpdateAP(enableAPUpdate);
         int nCount = m_pAnnotList->Count();
+        SetLock(TRUE);
         for(int i=0; i<nCount; i++)

[Tom Sepez, 2014/09/24 17:02:11]

if re-entered, nCount is from the old m_pAnnotList ...

         {
                 CPDF_Annot* pPDFAnnot = m_pAnnotList->GetAt(i);

[Tom Sepez, 2014/09/24 17:02:11]

... but this is retrieved from the new m_pAnnotList().

                 CPDF_Document * pDoc = this->GetPDFDocument();
                 
                 CheckUnSupportAnnot(pDoc, pPDFAnnot);
 
                 CPDFSDK_AnnotHandlerMgr* pAnnotHandlerMgr = pEnv->GetAnnotHandlerMgr();
                 ASSERT(pAnnotHandlerMgr != NULL);
+
                 if(pAnnotHandlerMgr)
                 {
                         CPDFSDK_Annot* pAnnot = pAnnotHandlerMgr->NewAnnot(pPDFAnnot, this);
                         if(!pAnnot)
                                 continue;
                         m_fxAnnotArray.Add(pAnnot);
 
                         pAnnotHandlerMgr->Annot_OnLoad(pAnnot);

[Tom Sepez, 2014/09/24 17:02:11]

(re-entrancy obseved above ...OnLoad() in this loop.

                 }
 
         }
+        SetLock(FALSE);
 }
 
 void    CPDFSDK_PageView::UpdateRects(CFX_RectArray& rects)
 {
         for(int i=0; i<rects.GetSize(); i++)
         {
                 CPDF_Rect rc = rects.GetAt(i);
                 CPDFDoc_Environment* pEnv = m_pSDKDoc->GetEnv();
                 pEnv->FFI_Invalidate(m_page, rc.left, rc.top, rc.right, rc.bottom);
         }
@@ skipping 46 matching lines @@
         
         for(int i=0; i<m_fxAnnotArray.GetSize(); i++)
         {
                 CPDFSDK_Annot* pAnnot = (CPDFSDK_Annot*)m_fxAnnotArray.GetAt(i);
                 if(pAnnot == pFocusAnnot)
                         return pAnnot;
         }
         return NULL;
 }