diff --git a/core/fpdfapi/page/cpdf_pageobjectholder.cpp b/core/fpdfapi/page/cpdf_pageobjectholder.cpp index 362b988c6..fd93d100d 100644 --- a/core/fpdfapi/page/cpdf_pageobjectholder.cpp +++ b/core/fpdfapi/page/cpdf_pageobjectholder.cpp @@ -186,6 +186,7 @@ CPDF_PageObject* CPDF_PageObjectHolder::GetPageObjectByIndex( void CPDF_PageObjectHolder::AppendPageObject( std::unique_ptr pPageObj) { CHECK(pPageObj); + ++page_object_list_mutation_stamp_; page_object_list_.push_back(std::move(pPageObj)); } @@ -199,6 +200,7 @@ bool CPDF_PageObjectHolder::InsertPageObjectAtIndex( // Unsafe, but the compiler will not complain, because // std::deque::iterator::operator++() has not been marked as unsafe yet. + ++page_object_list_mutation_stamp_; page_object_list_.insert(UNSAFE_TODO(page_object_list_.begin() + index), std::move(page_obj)); return true; @@ -213,6 +215,7 @@ std::unique_ptr CPDF_PageObjectHolder::RemovePageObject( } std::unique_ptr result = std::move(*it); + ++page_object_list_mutation_stamp_; page_object_list_.erase(it); int32_t content_stream = pPageObj->GetContentStream(); @@ -230,6 +233,7 @@ bool CPDF_PageObjectHolder::ErasePageObjectAtIndex(size_t index) { // Unsafe, but the compiler will not complain, because // std::deque::iterator::operator++() has not been marked as unsafe yet. + ++page_object_list_mutation_stamp_; page_object_list_.erase(UNSAFE_TODO(page_object_list_.begin() + index)); return true; } diff --git a/core/fpdfapi/page/cpdf_pageobjectholder.h b/core/fpdfapi/page/cpdf_pageobjectholder.h index 7ca2bbffc..69946690f 100644 --- a/core/fpdfapi/page/cpdf_pageobjectholder.h +++ b/core/fpdfapi/page/cpdf_pageobjectholder.h @@ -114,6 +114,14 @@ class CPDF_PageObjectHolder { iterator end() { return page_object_list_.end(); } const_iterator end() const { return page_object_list_.end(); } + // SECURITY: Incremented whenever `page_object_list_` is mutated. Any code + // that persists an iterator into `page_object_list_` across calls into the + // embedder must snapshot this value and CHECK it before reusing the + // iterator, since std::deque mutations invalidate all iterators. + uint64_t page_object_list_mutation_stamp() const { + return page_object_list_mutation_stamp_; + } + const CFX_FloatRect& GetBBox() const { return bbox_; } const CPDF_Transparency& GetTransparency() const { return transparency_; } @@ -164,6 +172,7 @@ class CPDF_PageObjectHolder { std::vector mask_bounding_boxes_; std::unique_ptr parser_; std::deque> page_object_list_; + uint64_t page_object_list_mutation_stamp_ = 0; CTMMap all_ctms_; diff --git a/core/fpdfapi/render/cpdf_progressiverenderer.cpp b/core/fpdfapi/render/cpdf_progressiverenderer.cpp index 3139b9374..7c732a68a 100644 --- a/core/fpdfapi/render/cpdf_progressiverenderer.cpp +++ b/core/fpdfapi/render/cpdf_progressiverenderer.cpp @@ -15,6 +15,7 @@ #include "core/fpdfapi/render/cpdf_renderoptions.h" #include "core/fpdfapi/render/cpdf_renderstatus.h" #include "core/fxcrt/check.h" +#include "core/fxcrt/check_op.h" #include "core/fxcrt/pauseindicator_iface.h" #include "core/fxge/cfx_renderdevice.h" @@ -52,6 +53,8 @@ void CPDF_ProgressiveRenderer::Continue(PauseIndicatorIface* pPause) { } current_layer_ = context_->GetLayer(layer_index_); last_object_rendered_ = current_layer_->GetObjectHolder()->end(); + holder_mutation_stamp_ = + current_layer_->GetObjectHolder()->page_object_list_mutation_stamp(); render_status_ = std::make_unique(context_, device_); if (options_) { render_status_->SetOptions(*options_); @@ -63,6 +66,15 @@ void CPDF_ProgressiveRenderer::Continue(PauseIndicatorIface* pPause) { clip_rect_ = current_layer_->GetMatrix().GetInverse().TransformRect( CFX_FloatRect(device_->GetClipBox())); } + // SECURITY: `last_object_rendered_` is a std::deque iterator persisted + // across yields to the embedder. Any mutation of the holder's + // page_object_list_ (e.g. via FPDFPage_InsertObject / + // FPDFPage_RemoveObject between FPDF_RenderPageBitmap_Start and + // FPDF_RenderPage_Continue) invalidates it. CHECK rather than risk a + // use-after-free / wild virtual call. + CHECK_EQ( + holder_mutation_stamp_, + current_layer_->GetObjectHolder()->page_object_list_mutation_stamp()); CPDF_PageObjectHolder::const_iterator iter; CPDF_PageObjectHolder::const_iterator iterEnd = current_layer_->GetObjectHolder()->end(); @@ -133,6 +145,13 @@ void CPDF_ProgressiveRenderer::Continue(PauseIndicatorIface* pPause) { return; } else { current_layer_->GetObjectHolder()->ContinueParse(pPause); + // ContinueParse() may itself append page objects; this is an internal + // mutation that does not go through the embedder. Re-snapshot so the + // CHECK above only guards against external mutation between yields. + // (Note: this branch is unreachable from Chrome's PDF viewer, which + // always renders fully-parsed holders.) + holder_mutation_stamp_ = + current_layer_->GetObjectHolder()->page_object_list_mutation_stamp(); if (current_layer_->GetObjectHolder()->GetParseState() != CPDF_PageObjectHolder::ParseState::kParsed) { return; diff --git a/core/fpdfapi/render/cpdf_progressiverenderer.h b/core/fpdfapi/render/cpdf_progressiverenderer.h index c30dd2f5f..24b220a72 100644 --- a/core/fpdfapi/render/cpdf_progressiverenderer.h +++ b/core/fpdfapi/render/cpdf_progressiverenderer.h @@ -55,6 +55,11 @@ class CPDF_ProgressiveRenderer { uint32_t layer_index_ = 0; UnownedPtr current_layer_; CPDF_PageObjectHolder::const_iterator last_object_rendered_; + // SECURITY: Snapshot of the holder's mutation stamp taken when + // `last_object_rendered_` was last assigned. CHECKed before reuse to detect + // page-object-list mutation between progressive yields, which invalidates + // the std::deque iterator and would otherwise lead to use-after-free. + uint64_t holder_mutation_stamp_ = 0; }; #endif // CORE_FPDFAPI_RENDER_CPDF_PROGRESSIVERENDERER_H_