[Backport] Security Bug 838886

Retain pp::ImageData while there are pending paints against it.

The ImageData might get destroyed while the paints are still
pending. Typically, the paints are then cancelled thereafter so
no harm comes from the dangling references, but this patch avoids
creating them in the first place.

The remaining changes are consequences of ProgressivePaint
becoming non-POD, and converting to protected members. Also
use scoped FPDF classes while we're at it.

Bug: 838886

Reviewed-on: https://chromium-review.googlesource.com/1054502

----------------------------------------------------------------

Prove that the memory was good at FPDFBitmap_CreateEx() create time.

Diagnostic for the associated bug, not a bugfix. Helps rule out one
possible scenario.

Bug: chromium:838886

Reviewed-on: https://pdfium-review.googlesource.com/32055

----------------------------------------------------------------

Change-Id: If8c4c2bd87b8b77e1111f27acf3276a7ebd6698f
Reviewed-by: Michael Brüning <michael.bruning@qt.io>

4 files changed, 104 insertions, 58 deletions

diff --git a/chromium/pdf/BUILD.gn b/chromium/pdf/BUILD.gn
index 34c33a4c378..31c0a34567c 100644
--- a/chromium/pdf/BUILD.gn
+++ b/chromium/pdf/BUILD.gn
@@ -16,6 +16,7 @@ buildflag_header("features") {
 
 if (enable_pdf) {
   pdf_engine = 0  # 0 PDFium
+  include_dirs = [ "//third_party/pdfium" ]
 
   static_library("pdf") {
     deps = [
diff --git a/chromium/pdf/pdfium/pdfium_engine.cc b/chromium/pdf/pdfium/pdfium_engine.cc
index 32ddfc20e4f..7778f644614 100644
--- a/chromium/pdf/pdfium/pdfium_engine.cc
+++ b/chromium/pdf/pdfium/pdfium_engine.cc
@@ -1055,7 +1055,7 @@ void PDFiumEngine::ScrolledToYPosition(int position) {
 
 void PDFiumEngine::PrePaint() {
   for (auto& paint : progressive_paints_)
-    paint.painted_ = false;
+    paint.set_painted(false);
 }
 
 void PDFiumEngine::Paint(const pp::Rect& rect,
@@ -1095,7 +1095,7 @@ void PDFiumEngine::Paint(const pp::Rect& rect,
       if (progressive != -1) {
         DCHECK_GE(progressive, 0);
         DCHECK_LT(static_cast<size_t>(progressive), progressive_paints_.size());
-        if (progressive_paints_[progressive].rect != dirty_in_screen) {
+        if (progressive_paints_[progressive].rect() != dirty_in_screen) {
           // The PDFium code can only handle one progressive paint at a time, so
           // queue this up. Previously we used to merge the rects when this
           // happened, but it made scrolling up on complex PDFs very slow since
@@ -1113,7 +1113,7 @@ void PDFiumEngine::Paint(const pp::Rect& rect,
         progressive_paint_timeout_ = kMaxProgressivePaintTime;
       }
 
-      progressive_paints_[progressive].painted_ = true;
+      progressive_paints_[progressive].set_painted(true);
       if (ContinuePaint(progressive, image_data)) {
         FinishPaint(progressive, image_data);
         ready->push_back(dirty_in_screen);
@@ -1129,14 +1129,14 @@ void PDFiumEngine::Paint(const pp::Rect& rect,
 
 void PDFiumEngine::PostPaint() {
   for (size_t i = 0; i < progressive_paints_.size(); ++i) {
-    if (progressive_paints_[i].painted_)
+    if (progressive_paints_[i].painted())
       continue;
 
     // This rectangle must have been merged with another one, that's why we
     // weren't asked to paint it. Remove it or otherwise we'll never finish
     // painting.
-    FPDF_RenderPage_Close(pages_[progressive_paints_[i].page_index]->GetPage());
-    FPDFBitmap_Destroy(progressive_paints_[i].bitmap);
+    FPDF_RenderPage_Close(
+        pages_[progressive_paints_[i].page_index()]->GetPage());
     progressive_paints_.erase(progressive_paints_.begin() + i);
     --i;
   }
@@ -1754,7 +1754,7 @@ PDFiumPage::Area PDFiumEngine::GetCharIndex(const pp::Point& point,
   // If the page hasn't finished rendering, calling into the page sometimes
   // leads to hangs.
   for (const auto& paint : progressive_paints_) {
-    if (paint.page_index == page)
+    if (paint.page_index() == page)
       return PDFiumPage::NONSELECTABLE_AREA;
   }
 
@@ -3228,12 +3228,7 @@ int PDFiumEngine::StartPaint(int page_index, const pp::Rect& dirty) {
   // For the first time we hit paint, do nothing and just record the paint for
   // the next callback.  This keeps the UI responsive in case the user is doing
   // a lot of scrolling.
-  ProgressivePaint progressive;
-  progressive.rect = dirty;
-  progressive.page_index = page_index;
-  progressive.bitmap = nullptr;
-  progressive.painted_ = false;
-  progressive_paints_.push_back(progressive);
+  progressive_paints_.emplace_back(page_index, dirty);
   return progressive_paints_.size() - 1;
 }
 
@@ -3243,29 +3238,35 @@ bool PDFiumEngine::ContinuePaint(int progressive_index,
   DCHECK_LT(static_cast<size_t>(progressive_index), progressive_paints_.size());
   DCHECK(image_data);
 
+  last_progressive_start_time_ = base::Time::Now();
 #if defined(OS_LINUX)
   g_last_instance_id = client_->GetPluginInstance()->pp_instance();
 #endif
 
-  int rv;
-  FPDF_BITMAP bitmap = progressive_paints_[progressive_index].bitmap;
-  int page_index = progressive_paints_[progressive_index].page_index;
+  int page_index = progressive_paints_[progressive_index].page_index();
   DCHECK(PageIndexInBounds(page_index));
-  FPDF_PAGE page = pages_[page_index]->GetPage();
 
-  last_progressive_start_time_ = base::Time::Now();
-  if (bitmap) {
+  int rv;
+  FPDF_PAGE page = pages_[page_index]->GetPage();
+  if (progressive_paints_[progressive_index].bitmap()) {
     rv = FPDF_RenderPage_Continue(page, static_cast<IFSDK_PAUSE*>(this));
   } else {
-    pp::Rect dirty = progressive_paints_[progressive_index].rect;
-    bitmap = CreateBitmap(dirty, image_data);
-    int start_x, start_y, size_x, size_y;
+    int start_x;
+    int start_y;
+    int size_x;
+    int size_y;
+    pp::Rect dirty = progressive_paints_[progressive_index].rect();
     GetPDFiumRect(page_index, dirty, &start_x, &start_y, &size_x, &size_y);
-    FPDFBitmap_FillRect(bitmap, start_x, start_y, size_x, size_y, 0xFFFFFFFF);
-    rv = FPDF_RenderPageBitmap_Start(
-        bitmap, page, start_x, start_y, size_x, size_y, current_rotation_,
-        GetRenderingFlags(), static_cast<IFSDK_PAUSE*>(this));
-    progressive_paints_[progressive_index].bitmap = bitmap;
+
+    std::unique_ptr<void, FPDFBitmapDeleter> new_bitmap = CreateBitmap(dirty, image_data);
+    FPDFBitmap_FillRect(new_bitmap.get(), start_x, start_y, size_x, size_y,
+                        0xFFFFFFFF);
+    rv = FPDF_RenderPageBitmap_Start(new_bitmap.get(), page, start_x, start_y,
+                                     size_x, size_y, current_rotation_,
+                                     GetRenderingFlags(),
+                                     static_cast<IFSDK_PAUSE*>(this));
+    progressive_paints_[progressive_index].SetBitmapAndImageData(
+        std::move(new_bitmap), *image_data);
   }
   return rv != FPDF_RENDER_TOBECOUNTINUED;
 }
@@ -3276,10 +3277,12 @@ void PDFiumEngine::FinishPaint(int progressive_index,
   DCHECK_LT(static_cast<size_t>(progressive_index), progressive_paints_.size());
   DCHECK(image_data);
 
-  int page_index = progressive_paints_[progressive_index].page_index;
-  const pp::Rect& dirty_in_screen = progressive_paints_[progressive_index].rect;
-  FPDF_BITMAP bitmap = progressive_paints_[progressive_index].bitmap;
+  int page_index = progressive_paints_[progressive_index].page_index();
+  const pp::Rect& dirty_in_screen =
+      progressive_paints_[progressive_index].rect();
+
   int start_x, start_y, size_x, size_y;
+  FPDF_BITMAP bitmap = progressive_paints_[progressive_index].bitmap();
   GetPDFiumRect(page_index, dirty_in_screen, &start_x, &start_y, &size_x,
                 &size_y);
 
@@ -3295,17 +3298,15 @@ void PDFiumEngine::FinishPaint(int progressive_index,
   DrawSelections(progressive_index, image_data);
 
   FPDF_RenderPage_Close(pages_[page_index]->GetPage());
-  FPDFBitmap_Destroy(bitmap);
   progressive_paints_.erase(progressive_paints_.begin() + progressive_index);
 
   client_->DocumentPaintOccurred();
 }
 
 void PDFiumEngine::CancelPaints() {
-  for (const auto& paint : progressive_paints_) {
-    FPDF_RenderPage_Close(pages_[paint.page_index]->GetPage());
-    FPDFBitmap_Destroy(paint.bitmap);
-  }
+  for (const auto& paint : progressive_paints_)
+    FPDF_RenderPage_Close(pages_[paint.page_index()]->GetPage());
+
   progressive_paints_.clear();
 }
 
@@ -3313,9 +3314,10 @@ void PDFiumEngine::FillPageSides(int progressive_index) {
   DCHECK_GE(progressive_index, 0);
   DCHECK_LT(static_cast<size_t>(progressive_index), progressive_paints_.size());
 
-  int page_index = progressive_paints_[progressive_index].page_index;
-  const pp::Rect& dirty_in_screen = progressive_paints_[progressive_index].rect;
-  FPDF_BITMAP bitmap = progressive_paints_[progressive_index].bitmap;
+  int page_index = progressive_paints_[progressive_index].page_index();
+  const pp::Rect& dirty_in_screen =
+      progressive_paints_[progressive_index].rect();
+  FPDF_BITMAP bitmap = progressive_paints_[progressive_index].bitmap();
 
   pp::Rect page_rect = pages_[page_index]->rect();
   if (page_rect.x() > 0) {
@@ -3361,8 +3363,9 @@ void PDFiumEngine::PaintPageShadow(int progressive_index,
   DCHECK_LT(static_cast<size_t>(progressive_index), progressive_paints_.size());
   DCHECK(image_data);
 
-  int page_index = progressive_paints_[progressive_index].page_index;
-  const pp::Rect& dirty_in_screen = progressive_paints_[progressive_index].rect;
+  int page_index = progressive_paints_[progressive_index].page_index();
+  const pp::Rect& dirty_in_screen =
+      progressive_paints_[progressive_index].rect();
   pp::Rect page_rect = pages_[page_index]->rect();
   pp::Rect shadow_rect(page_rect);
   shadow_rect.Inset(-kPageShadowLeft, -kPageShadowTop, -kPageShadowRight,
@@ -3389,8 +3392,9 @@ void PDFiumEngine::DrawSelections(int progressive_index,
   DCHECK_LT(static_cast<size_t>(progressive_index), progressive_paints_.size());
   DCHECK(image_data);
 
-  int page_index = progressive_paints_[progressive_index].page_index;
-  const pp::Rect& dirty_in_screen = progressive_paints_[progressive_index].rect;
+  int page_index = progressive_paints_[progressive_index].page_index();
+  const pp::Rect& dirty_in_screen =
+      progressive_paints_[progressive_index].rect();
 
   void* region = nullptr;
   int stride;
@@ -3432,34 +3436,33 @@ void PDFiumEngine::PaintUnavailablePage(int page_index,
                                         pp::ImageData* image_data) {
   int start_x, start_y, size_x, size_y;
   GetPDFiumRect(page_index, dirty, &start_x, &start_y, &size_x, &size_y);
-  FPDF_BITMAP bitmap = CreateBitmap(dirty, image_data);
-  FPDFBitmap_FillRect(bitmap, start_x, start_y, size_x, size_y,
+  std::unique_ptr<void, FPDFBitmapDeleter> bitmap = CreateBitmap(dirty, image_data);
+  FPDFBitmap_FillRect(bitmap.get(), start_x, start_y, size_x, size_y,
                       kPendingPageColor);
 
   pp::Rect loading_text_in_screen(
       pages_[page_index]->rect().width() / 2,
       pages_[page_index]->rect().y() + kLoadingTextVerticalOffset, 0, 0);
   loading_text_in_screen = GetScreenRect(loading_text_in_screen);
-  FPDFBitmap_Destroy(bitmap);
 }
 
 int PDFiumEngine::GetProgressiveIndex(int page_index) const {
   for (size_t i = 0; i < progressive_paints_.size(); ++i) {
-    if (progressive_paints_[i].page_index == page_index)
+    if (progressive_paints_[i].page_index() == page_index)
       return i;
   }
   return -1;
 }
 
-FPDF_BITMAP PDFiumEngine::CreateBitmap(const pp::Rect& rect,
-                                       pp::ImageData* image_data) const {
+std::unique_ptr<void, FPDFBitmapDeleter> PDFiumEngine::CreateBitmap(const pp::Rect& rect,
+                                            pp::ImageData* image_data) const {
   void* region;
   int stride;
   GetRegion(rect.point(), image_data, &region, &stride);
   if (!region)
     return nullptr;
-  return FPDFBitmap_CreateEx(rect.width(), rect.height(), FPDFBitmap_BGRx,
-                             region, stride);
+  return  std::unique_ptr<void, FPDFBitmapDeleter>(FPDFBitmap_CreateEx(rect.width(), rect.height(),
+                                              FPDFBitmap_BGRx, region, stride));
 }
 
 void PDFiumEngine::GetPDFiumRect(int page_index,
@@ -4352,6 +4355,25 @@ PDFiumEngine::FormFillTimerData::FormFillTimerData(base::TimeDelta period,
                                                    TimerCallback callback)
     : timer_period(period), timer_callback(callback) {}
 
+PDFiumEngine::ProgressivePaint::ProgressivePaint(int index,
+                                                 const pp::Rect& rect)
+    : page_index_(index), rect_(rect) {}
+
+PDFiumEngine::ProgressivePaint::ProgressivePaint(ProgressivePaint&& that) =
+    default;
+
+PDFiumEngine::ProgressivePaint::~ProgressivePaint() = default;
+
+PDFiumEngine::ProgressivePaint& PDFiumEngine::ProgressivePaint::operator=(
+    ProgressivePaint&& that) = default;
+
+void PDFiumEngine::ProgressivePaint::SetBitmapAndImageData(
+    std::unique_ptr<void, FPDFBitmapDeleter> bitmap,
+    pp::ImageData image_data) {
+  bitmap_ = std::move(bitmap);
+  image_data_ = std::move(image_data);
+}
+
 ScopedUnsupportedFeature::ScopedUnsupportedFeature(PDFiumEngine* engine)
     : old_engine_(g_engine_for_unsupported) {
   g_engine_for_unsupported = engine;
diff --git a/chromium/pdf/pdfium/pdfium_engine.h b/chromium/pdf/pdfium/pdfium_engine.h
index da9875e5adf..e90fc4c63b1 100644
--- a/chromium/pdf/pdfium/pdfium_engine.h
+++ b/chromium/pdf/pdfium/pdfium_engine.h
@@ -31,6 +31,7 @@
 #include "third_party/pdfium/public/fpdf_formfill.h"
 #include "third_party/pdfium/public/fpdf_progressive.h"
 #include "third_party/pdfium/public/fpdfview.h"
+#include "third_party/pdfium/public/cpp/fpdf_deleters.h"
 
 namespace chrome_pdf {
 
@@ -393,8 +394,8 @@ class PDFiumEngine : public PDFEngine,
   int GetProgressiveIndex(int page_index) const;
 
   // Creates a FPDF_BITMAP from a rectangle in screen coordinates.
-  FPDF_BITMAP CreateBitmap(const pp::Rect& rect,
-                           pp::ImageData* image_data) const;
+  std::unique_ptr<void, FPDFBitmapDeleter> CreateBitmap(const pp::Rect& rect,
+                                pp::ImageData* image_data) const;
 
   // Given a rectangle in screen coordinates, returns the coordinates in the
   // units that PDFium rendering functions expect.
@@ -786,13 +787,33 @@ class PDFiumEngine : public PDFEngine,
   std::string link_under_cursor_;
 
   // Pending progressive paints.
-  struct ProgressivePaint {
-    pp::Rect rect;  // In screen coordinates.
-    FPDF_BITMAP bitmap;
-    int page_index;
+  class ProgressivePaint {
+   public:
+    ProgressivePaint(int page_index, const pp::Rect& rect);
+    ProgressivePaint(ProgressivePaint&& that);
+    ~ProgressivePaint();
+
+    ProgressivePaint& operator=(ProgressivePaint&& that);
+
+    int page_index() const { return page_index_; }
+    const pp::Rect& rect() const { return rect_; }
+    FPDF_BITMAP bitmap() const { return bitmap_.get(); }
+    bool painted() const { return painted_; }
+
+    void set_painted(bool enable) { painted_ = enable; }
+    void SetBitmapAndImageData( std::unique_ptr<void, FPDFBitmapDeleter> bitmap,
+                               pp::ImageData image_data);
+
+   private:
+    int page_index_;
+    pp::Rect rect_;             // In screen coordinates.
+    pp::ImageData image_data_;  // Maintains reference while |bitmap_| exists.
+    std::unique_ptr<void, FPDFBitmapDeleter> bitmap_;   // Must come after |image_data_|.
     // Temporary used to figure out if in a series of Paint() calls whether this
     // pending paint was updated or not.
-    bool painted_;
+    bool painted_ = false;
+
+    DISALLOW_COPY_AND_ASSIGN(ProgressivePaint);
   };
   std::vector<ProgressivePaint> progressive_paints_;
 
diff --git a/chromium/third_party/pdfium/fpdfsdk/fpdfview.cpp b/chromium/third_party/pdfium/fpdfsdk/fpdfview.cpp
index 97fc02a2343..f11515143bf 100644
--- a/chromium/third_party/pdfium/fpdfsdk/fpdfview.cpp
+++ b/chromium/third_party/pdfium/fpdfsdk/fpdfview.cpp
@@ -1198,8 +1198,10 @@ FPDF_EXPORT FPDF_BITMAP FPDF_CALLCONV FPDFBitmap_CreateEx(int width,
     default:
       return nullptr;
   }
+   // Ensure external memory is good at least for the duration of this call.
+  UnownedPtr<uint8_t> pChecker(static_cast<uint8_t*>(first_scan));
   auto pBitmap = pdfium::MakeRetain<CFX_DIBitmap>();
-  pBitmap->Create(width, height, fx_format, static_cast<uint8_t*>(first_scan),
+  pBitmap->Create(width, height, fx_format, pChecker.Get(),
                   stride);
   return pBitmap.Leak();
 }