From 009019152edb1aace33472a6858fe39f9b44b67b Mon Sep 17 00:00:00 2001 From: Michal Klocek Date: Thu, 10 May 2018 23:46:39 +0000 Subject: [Backport] Security Bug 838886 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- chromium/pdf/BUILD.gn | 1 + chromium/pdf/pdfium/pdfium_engine.cc | 122 +++++++++++++---------- chromium/pdf/pdfium/pdfium_engine.h | 35 +++++-- chromium/third_party/pdfium/fpdfsdk/fpdfview.cpp | 4 +- 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(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(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(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(this)); - progressive_paints_[progressive_index].bitmap = bitmap; + + std::unique_ptr 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(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(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(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(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(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 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 PDFiumEngine::CreateBitmap(const pp::Rect& rect, + pp::ImageData* image_data) const { void* region; int stride; GetRegion(rect.point(), image_data, ®ion, &stride); if (!region) return nullptr; - return FPDFBitmap_CreateEx(rect.width(), rect.height(), FPDFBitmap_BGRx, - region, stride); + return std::unique_ptr(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 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 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 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 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 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 pChecker(static_cast(first_scan)); auto pBitmap = pdfium::MakeRetain(); - pBitmap->Create(width, height, fx_format, static_cast(first_scan), + pBitmap->Create(width, height, fx_format, pChecker.Get(), stride); return pBitmap.Leak(); } -- cgit v1.2.3