Fix heap use after free in CPDFSDK_Annot::GetPDFAnnot.
authorTom Sepez <tsepez@chromium.org>
Wed, 9 Sep 2015 16:58:10 +0000 (09:58 -0700)
committerTom Sepez <tsepez@chromium.org>
Wed, 9 Sep 2015 16:58:10 +0000 (09:58 -0700)
Use two seperate loops to kill current focus annot and to release annots
in current page. Loop to kill current focus annot is run first, so it
will not access deleted annots.

BUG=507316

R=tsepez@chromium.org

TEST=Reproduction steps mentioned in issue 507316 should not crash
     chrome.
     Unit test added to pdfium.
     Run pdfium_embeddertests.exe.

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

fpdfsdk/src/fpdfformfill_embeddertest.cpp
fpdfsdk/src/fsdk_mgr.cpp
testing/embedder_test.cpp
testing/embedder_test.h

index 6baad11..56710b9 100644 (file)
@@ -40,6 +40,18 @@ TEST_F(FPDFFormFillEmbeddertest, BUG_487928) {
   UnloadPage(page);
 }
 
+TEST_F(FPDFFormFillEmbeddertest, BUG_507316) {
+  EmbedderTestTimerHandlingDelegate delegate;
+  SetDelegate(&delegate);
+
+  EXPECT_TRUE(OpenDocument("testing/resources/bug_507316.pdf"));
+  FPDF_PAGE page = LoadAndCachePage(2);
+  EXPECT_NE(nullptr, page);
+  DoOpenActions();
+  delegate.AdvanceTime(4000);
+  UnloadPage(page);
+}
+
 TEST_F(FPDFFormFillEmbeddertest, BUG_514690) {
   EXPECT_TRUE(OpenDocument("testing/resources/hello_world.pdf"));
   FPDF_PAGE page = LoadPage(0);
index 3eca559..9cbb9de 100644 (file)
@@ -634,16 +634,22 @@ CPDFSDK_PageView::CPDFSDK_PageView(CPDFSDK_Document* pSDKDoc, CPDF_Page* page)
 }
 
 CPDFSDK_PageView::~CPDFSDK_PageView() {
-  CPDFDoc_Environment* pEnv = m_pSDKDoc->GetEnv();
-  int nAnnotCount = m_fxAnnotArray.GetSize();
+  // if there is a focused annot on the page, we should kill the focus first.
+  if (CPDFSDK_Annot* focusedAnnot = m_pSDKDoc->GetFocusAnnot()) {
+    for (int i = 0, count = m_fxAnnotArray.GetSize(); i < count; i++) {
+      CPDFSDK_Annot* pAnnot = (CPDFSDK_Annot*)m_fxAnnotArray.GetAt(i);
+      if (pAnnot == focusedAnnot) {
+        KillFocusAnnot();
+        break;
+      }
+    }
+  }
 
-  for (int i = 0; i < nAnnotCount; i++) {
+  CPDFDoc_Environment* pEnv = m_pSDKDoc->GetEnv();
+  CPDFSDK_AnnotHandlerMgr* pAnnotHandlerMgr = pEnv->GetAnnotHandlerMgr();
+  ASSERT(pAnnotHandlerMgr);
+  for (int i = 0, count = m_fxAnnotArray.GetSize(); i < count; 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();
index 1476522..eb16bc7 100644 (file)
@@ -221,6 +221,7 @@ bool EmbedderTest::OpenDocument(const std::string& filename) {
   formfillinfo->version = 1;
   formfillinfo->FFI_SetTimer = SetTimerTrampoline;
   formfillinfo->FFI_KillTimer = KillTimerTrampoline;
+  formfillinfo->FFI_GetPage = GetPageTrampoline;
   formfillinfo->m_pJsPlatform = platform;
 
   form_handle_ = FPDFDOC_InitFormFillEnvironment(document_, formfillinfo);
@@ -259,6 +260,15 @@ FPDF_PAGE EmbedderTest::LoadPage(int page_number) {
   return page;
 }
 
+FPDF_PAGE EmbedderTest::LoadAndCachePage(int page_number) {
+  FPDF_PAGE page = delegate_->GetPage(form_handle_, document_, page_number);
+  if (!page) {
+    return nullptr;
+  }
+  FORM_DoPageAAction(page, form_handle_, FPDFPAGE_AACTION_OPEN);
+  return page;
+}
+
 FPDF_BITMAP EmbedderTest::RenderPage(FPDF_PAGE page) {
   int width = static_cast<int>(FPDF_GetPageWidth(page));
   int height = static_cast<int>(FPDF_GetPageHeight(page));
@@ -275,6 +285,22 @@ void EmbedderTest::UnloadPage(FPDF_PAGE page) {
   FPDF_ClosePage(page);
 }
 
+FPDF_PAGE EmbedderTest::Delegate::GetPage(FPDF_FORMHANDLE form_handle,
+                                          FPDF_DOCUMENT document,
+                                          int page_index) {
+  auto it = m_pageMap.find(page_index);
+  if (it != m_pageMap.end()) {
+    return it->second;
+  }
+  FPDF_PAGE page = FPDF_LoadPage(document, page_index);
+  if (!page) {
+    return nullptr;
+  }
+  m_pageMap[page_index] = page;
+  FORM_OnAfterLoadPage(page, form_handle);
+  return page;
+}
+
 // static
 void EmbedderTest::UnsupportedHandlerTrampoline(UNSUPPORT_INFO* info,
                                                 int type) {
@@ -306,6 +332,14 @@ void EmbedderTest::KillTimerTrampoline(FPDF_FORMFILLINFO* info, int id) {
   return test->delegate_->KillTimer(id);
 }
 
+// static
+FPDF_PAGE EmbedderTest::GetPageTrampoline(FPDF_FORMFILLINFO* info,
+                                          FPDF_DOCUMENT document,
+                                          int page_index) {
+  EmbedderTest* test = static_cast<EmbedderTest*>(info);
+  return test->delegate_->GetPage(test->m_pFormfillinfo, document, page_index);
+}
+
 // Can't use gtest-provided main since we need to stash the path to the
 // executable in order to find the external V8 binary data files.
 int main(int argc, char** argv) {
index 4bd9d97..f490f82 100644 (file)
@@ -5,6 +5,7 @@
 #ifndef TESTING_EMBEDDER_TEST_H_
 #define TESTING_EMBEDDER_TEST_H_
 
+#include <map>
 #include <string>
 
 #include "../public/fpdf_dataavail.h"
@@ -43,6 +44,14 @@ class EmbedderTest : public ::testing::Test,
 
     // Equivalent to FPDF_FORMFILLINFO::FFI_KillTimer().
     virtual void KillTimer(int id) {}
+
+    // Equivalent to FPDF_FORMFILLINFO::FFI_GetPage().
+    virtual FPDF_PAGE GetPage(FPDF_FORMHANDLE form_handle,
+                              FPDF_DOCUMENT document,
+                              int page_index);
+
+   private:
+    std::map<int, FPDF_PAGE> m_pageMap;
   };
 
   EmbedderTest();
@@ -72,6 +81,10 @@ class EmbedderTest : public ::testing::Test,
   // Load a specific page of the open document.
   virtual FPDF_PAGE LoadPage(int page_number);
 
+  // Load a specific page of the open document using delegate_->GetPage.
+  // delegate_->GetPage also caches loaded page.
+  virtual FPDF_PAGE LoadAndCachePage(int page_number);
+
   // Convert a loaded page into a bitmap.
   virtual FPDF_BITMAP RenderPage(FPDF_PAGE page);
 
@@ -106,6 +119,9 @@ class EmbedderTest : public ::testing::Test,
                                 int msecs,
                                 TimerCallback fn);
   static void KillTimerTrampoline(FPDF_FORMFILLINFO* info, int id);
+  static FPDF_PAGE GetPageTrampoline(FPDF_FORMFILLINFO* info,
+                                     FPDF_DOCUMENT document,
+                                     int page_index);
 };
 
 #endif  // TESTING_EMBEDDER_TEST_H_