commit ca03f7a1c654709152a411f665ce38c3626582e8
author Dan Sinclair <dsinclair@chromium.org> Thu Oct 06 14:53:13 2016 -0400
committer Dan Sinclair <dsinclair@chromium.org> Thu Oct 06 14:53:13 2016 -0400
tree 21219202db1f20f84e8bc625662278091f856094
parent f7ca7302bed831801a0b81ff4e1222c75833d2d6

Fixup MSan embeddertests

The embeddertests were closing the document before the formfill environment.
This caused a use-after-free as we try to use the document during formfill
destruction.

This Cl fixes the destruction order in the embedder tests. As well, a few guards
are put in place to keep the system from crashing if the wrong destruction
order is called.

R=tsepez@chromium.org

Review URL: https://codereview.chromium.org/2398063002 .

fpdfsdk/cpdfsdk_document.h

diff --git a/fpdfsdk/cpdfsdk_document.h b/fpdfsdk/cpdfsdk_document.h
index 3da229c..ee140d8 100644
--- a/fpdfsdk/cpdfsdk_document.h
+++ b/fpdfsdk/cpdfsdk_document.h
@@ -54,6 +54,7 @@
 #ifdef PDF_ENABLE_XFA
   // Gets the XFA document directly (XFA-only).
   CPDFXFA_Document* GetXFADocument() const { return m_pDoc; }
+  void ResetXFADocument() { m_pDoc = nullptr; }
 
   int GetPageViewCount() const { return m_pageMap.size(); }
 #endif  // PDF_ENABLE_XFA

fpdfsdk/fpdfformfill.cpp

diff --git a/fpdfsdk/fpdfformfill.cpp b/fpdfsdk/fpdfformfill.cpp
index 8be9d5f..fd083c9 100644
--- a/fpdfsdk/fpdfformfill.cpp
+++ b/fpdfsdk/fpdfformfill.cpp
@@ -277,7 +277,10 @@
   // XFA document.
   if (CPDFSDK_Document* pSDKDoc = pEnv->GetSDKDocument()) {
     pSDKDoc->ClearAllFocusedAnnots();
-    pSDKDoc->GetXFADocument()->SetSDKDoc(nullptr);
+    // If the document was closed first, it's possible the XFA document
+    // is now a nullptr.
+    if (pSDKDoc->GetXFADocument())
+      pSDKDoc->GetXFADocument()->SetSDKDoc(nullptr);
   }
 #endif  // PDF_ENABLE_XFA
 

fpdfsdk/fpdfxfa/cpdfxfa_document.cpp

diff --git a/fpdfsdk/fpdfxfa/cpdfxfa_document.cpp b/fpdfsdk/fpdfxfa/cpdfxfa_document.cpp
index bd7f931..90a762d 100644
--- a/fpdfsdk/fpdfxfa/cpdfxfa_document.cpp
+++ b/fpdfsdk/fpdfxfa/cpdfxfa_document.cpp
@@ -45,6 +45,9 @@
 
   if (m_pSDKDoc) {
     m_pSDKDoc->ClearAllFocusedAnnots();
+    // Once we're deleted the SDKDocument will point at a bad underlying
+    // doc so we need to reset it ...
+    m_pSDKDoc->ResetXFADocument();
     m_pSDKDoc = nullptr;
   }
 

testing/embedder_test.cpp

diff --git a/testing/embedder_test.cpp b/testing/embedder_test.cpp
index 1ce0f36..c23b5c8 100644
--- a/testing/embedder_test.cpp
+++ b/testing/embedder_test.cpp
@@ -99,16 +99,8 @@
 void EmbedderTest::TearDown() {
   if (document_) {
     FORM_DoDocumentAAction(form_handle_, FPDFDOC_AACTION_WC);
-#ifdef PDF_ENABLE_XFA
-    // Note: The shut down order here is the reverse of the non-XFA branch
-    // order. Need to work out if this is required, and if it is, the lifetimes
-    // of objects owned by |doc| that |form| reference.
-    FPDF_CloseDocument(document_);
-    FPDFDOC_ExitFormFillEnvironment(form_handle_);
-#else   // PDF_ENABLE_XFA
     FPDFDOC_ExitFormFillEnvironment(form_handle_);
     FPDF_CloseDocument(document_);
-#endif  // PDF_ENABLE_XFA
   }
 
   FPDFAvail_Destroy(avail_);