XFA: Change the destruction order inside CPDFXFA_Document to avoid UAFs.

fpdfsdk/src/fpdfxfa/fpdfxfa_doc.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/fpdfxfa/fpdfxfa_doc.h"
 #include "../../include/fsdk_mgr.h"
 #include "../../include/fpdfxfa/fpdfxfa_app.h"
@@ skipping 27 matching lines @@
     : m_iDocType(DOCTYPE_PDF),
       m_pPDFDoc(pPDFDoc),
       m_pSDKDoc(nullptr),
       m_pXFADoc(nullptr),
       m_pXFADocView(nullptr),
       m_pApp(pProvider),
       m_pJSContext(nullptr) {
 }
 
 CPDFXFA_Document::~CPDFXFA_Document() {
+  if (m_pJSContext && m_pSDKDoc && m_pSDKDoc->GetEnv()) {
+    m_pSDKDoc->GetEnv()->GetJSRuntime()->ReleaseContext(m_pJSContext);
+    m_pJSContext = nullptr;

[jun_fang, 2016/01/07 04:30:47]

Nit: Do we need to set m_pJSContext as nullptr in the destruction function? 

[Lei Zhang, 2016/01/07 06:54:26]

Done.

+  }
+
+  delete m_pSDKDoc;
+  m_pSDKDoc = nullptr;

[jun_fang, 2016/01/07 04:30:47]

Nit: as above 

[Lei Zhang, 2016/01/07 06:54:26]

Done.

+
   if (m_pPDFDoc) {
     CPDF_Parser* pParser = (CPDF_Parser*)m_pPDFDoc->GetParser();
-    if (pParser == NULL) {
+    if (pParser)
+      delete pParser;
+    else
       delete m_pPDFDoc;
-    } else {
-      delete pParser;
-    }
-    m_pPDFDoc = NULL;
+    m_pPDFDoc = nullptr;

[jun_fang, 2016/01/07 04:30:47]

nit: as above

[Lei Zhang, 2016/01/07 06:54:26]

Done.

   }
   if (m_pXFADoc) {
     IXFA_App* pApp = m_pApp->GetXFAApp();
     if (pApp) {
       IXFA_DocHandler* pDocHandler = pApp->GetDocHandler();
       if (pDocHandler) {
         CloseXFADoc(pDocHandler);
       }
     }
   }
-
-  if (m_pJSContext) {
-    if (m_pSDKDoc && m_pSDKDoc->GetEnv()) {
-      m_pSDKDoc->GetEnv()->GetJSRuntime()->ReleaseContext(m_pJSContext);
-      m_pJSContext = NULL;
-    }
-  }
-
-  if (m_pSDKDoc)
-    delete m_pSDKDoc;
-  m_pSDKDoc = NULL;
 }
 
 FX_BOOL CPDFXFA_Document::LoadXFADoc() {
   if (!m_pPDFDoc)
     return FALSE;
 
   m_XFAPageList.RemoveAll();
 
   IXFA_App* pApp = m_pApp->GetXFAApp();
   if (!pApp)
@@ skipping 110 matching lines @@
   m_XFAPageList.SetAt(page->GetPageIndex(), NULL);
 }
 
 CPDFSDK_Document* CPDFXFA_Document::GetSDKDocument(
     CPDFDoc_Environment* pFormFillEnv) {
   if (!m_pSDKDoc && pFormFillEnv)
     m_pSDKDoc = new CPDFSDK_Document(this, pFormFillEnv);
   return m_pSDKDoc;
 }
 
-void CPDFXFA_Document::ReleaseSDKDoc() {
-  if (m_pSDKDoc)
-    delete m_pSDKDoc;
-
-  m_pSDKDoc = NULL;
-}
-
 void CPDFXFA_Document::FXRect2PDFRect(const CFX_RectF& fxRectF,
                                       CPDF_Rect& pdfRect) {
   pdfRect.left = fxRectF.left;
   pdfRect.top = fxRectF.bottom();
   pdfRect.right = fxRectF.right();
   pdfRect.bottom = fxRectF.top;
 }
 
-//////////////////////////////////////////////////////////////////////////
 void CPDFXFA_Document::SetChangeMark(IXFA_Doc* hDoc) {
   if (hDoc == m_pXFADoc && m_pSDKDoc) {
     m_pSDKDoc->SetChangeMark();
   }
 }
 
 FX_BOOL CPDFXFA_Document::GetChangeMark(IXFA_Doc* hDoc) {
   if (hDoc == m_pXFADoc && m_pSDKDoc)
     return m_pSDKDoc->GetChangeMark();
   return FALSE;
@@ skipping 1034 matching lines @@
   }
 
   return _GetHValueByName(szPropName, hValue,
                           m_pSDKDoc->GetEnv()->GetJSRuntime());
 }
 FX_BOOL CPDFXFA_Document::_GetHValueByName(const CFX_ByteStringC& utf8Name,
                                            FXJSE_HVALUE hValue,
                                            IJS_Runtime* runTime) {
   return runTime->GetHValueByName(utf8Name, hValue);
 }