Lock page in LoadFXAnnot to prevent unintended page closing

fpdfsdk/src/fsdk_mgr.cpp

Index: fpdfsdk/src/fsdk_mgr.cpp
diff --git a/fpdfsdk/src/fsdk_mgr.cpp b/fpdfsdk/src/fsdk_mgr.cpp
index 52173b4c96a29269489979adb901a4e493a05aec..efb21a7efce13f8cd1c866fa1736cf8aa25c2207 100644
--- a/fpdfsdk/src/fsdk_mgr.cpp
+++ b/fpdfsdk/src/fsdk_mgr.cpp
@@ -332,7 +332,14 @@ CPDFSDK_Document::CPDFSDK_Document(CPDF_Document* pDoc,CPDFDoc_Environment* pEnv
 
 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();
@@ -453,7 +460,7 @@ CPDF_OCContext*	CPDFSDK_Document::GetOCContext()
 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);
@@ -608,7 +615,7 @@ CPDFSDK_PageView::CPDFSDK_PageView(CPDFSDK_Document* pSDKDoc,CPDF_Page* page):m_
 		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;
@@ -616,12 +623,15 @@ CPDFSDK_PageView::CPDFSDK_PageView(CPDFSDK_Document* pSDKDoc,CPDF_Page* page):m_
 	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);
@@ -638,6 +648,10 @@ CPDFSDK_PageView::~CPDFSDK_PageView()
 		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)
@@ -972,6 +986,7 @@ void CPDFSDK_PageView::LoadFXAnnots()
 	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().

@@ -981,6 +996,7 @@ void CPDFSDK_PageView::LoadFXAnnots()
 
 		CPDFSDK_AnnotHandlerMgr* pAnnotHandlerMgr = pEnv->GetAnnotHandlerMgr();
 		ASSERT(pAnnotHandlerMgr != NULL);
+
 		if(pAnnotHandlerMgr)
 		{
 			CPDFSDK_Annot* pAnnot = pAnnotHandlerMgr->NewAnnot(pPDFAnnot, this);
@@ -992,6 +1008,7 @@ void CPDFSDK_PageView::LoadFXAnnots()
 		}
 
 	}
+        SetLock(FALSE);
 }
 
 void	CPDFSDK_PageView::UpdateRects(CFX_RectArray& rects)