From 5353de15a14144cd72c630925fe28abad566f6a2 Mon Sep 17 00:00:00 2001 From: Akhila Veerapuraju <dhveerap@microsoft.com> Date: Thu, 29 Apr 2021 15:52:19 +0000 Subject: [PATCH] [Backport] CVE-2021-30518: Heap buffer overflow in Reader Mode Cherry-pick of patch originally reviewed on https://chromium-review.googlesource.com/c/chromium/src/+/2856118: Replace std::vector with base::ObserverList to support container modification while iterating TaskTracker saves list of viewers in vector, that needs to be notified when distillation is completed. At the time of notifying the viewers, we are indirectly erasing viewers from vector while iterating. This is causing container-overflow in asan build when vector has more than one viewer while notifying. This change is to replace vector with ObserverList that can be modified during iteration without invalidating the iterator. Bug: 1203590 Change-Id: I7c7b8237584c48c9ebc2639b9268a6a78c2db4b2 Reviewed-by: Matt Jones <mdjones@chromium.org> Commit-Queue: Akhila Veerapuraju <dhveerap@microsoft.com> Cr-Commit-Position: refs/heads/master@{#877492} Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io> --- .../components/dom_distiller/core/task_tracker.cc | 12 ++++++------ .../components/dom_distiller/core/task_tracker.h | 7 ++++--- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/chromium/components/dom_distiller/core/task_tracker.cc b/chromium/components/dom_distiller/core/task_tracker.cc index e66a62c4091..f22c88967bc 100644 --- a/chromium/components/dom_distiller/core/task_tracker.cc +++ b/chromium/components/dom_distiller/core/task_tracker.cc @@ -85,7 +85,7 @@ void TaskTracker::AddSaveCallback(SaveCallback callback) { std::unique_ptr<ViewerHandle> TaskTracker::AddViewer( ViewRequestDelegate* delegate) { - viewers_.push_back(delegate); + viewers_.AddObserver(delegate); if (content_ready_) { // Distillation for this task has already completed, and so the delegate can // be immediately told of the result. @@ -115,7 +115,7 @@ bool TaskTracker::HasUrl(const GURL& url) const { } void TaskTracker::RemoveViewer(ViewRequestDelegate* delegate) { - base::Erase(viewers_, delegate); + viewers_.RemoveObserver(delegate); if (viewers_.empty()) { MaybeCancel(); } @@ -219,8 +219,8 @@ void TaskTracker::DistilledArticleReady( } void TaskTracker::NotifyViewersAndCallbacks() { - for (auto* viewer : viewers_) { - NotifyViewer(viewer); + for (auto& viewer : viewers_) { + NotifyViewer(&viewer); } // Already inside a callback run SaveCallbacks directly. @@ -242,8 +242,8 @@ void TaskTracker::DoSaveCallbacks(bool success) { void TaskTracker::OnArticleDistillationUpdated( const ArticleDistillationUpdate& article_update) { - for (auto* viewer : viewers_) { - viewer->OnArticleUpdated(article_update); + for (auto& viewer : viewers_) { + viewer.OnArticleUpdated(article_update); } } diff --git a/chromium/components/dom_distiller/core/task_tracker.h b/chromium/components/dom_distiller/core/task_tracker.h index 484145cf7d1..cc13e727292 100644 --- a/chromium/components/dom_distiller/core/task_tracker.h +++ b/chromium/components/dom_distiller/core/task_tracker.h @@ -11,6 +11,7 @@ #include "base/bind.h" #include "base/callback.h" #include "base/memory/weak_ptr.h" +#include "base/observer_list.h" #include "components/dom_distiller/core/article_distillation_update.h" #include "components/dom_distiller/core/article_entry.h" #include "components/dom_distiller/core/distiller.h" @@ -40,9 +41,9 @@ class ViewerHandle { // Interface for a DOM distiller entry viewer. Implement this to make a view // request and receive the data for an entry when it becomes available. -class ViewRequestDelegate { +class ViewRequestDelegate : public base::CheckedObserver { public: - virtual ~ViewRequestDelegate() = default; + ~ViewRequestDelegate() override = default; // Called when the distilled article contents are available. The // DistilledArticleProto is owned by a TaskTracker instance and is invalidated @@ -140,7 +141,7 @@ class TaskTracker { std::vector<SaveCallback> save_callbacks_; // A ViewRequestDelegate will be added to this list when a view request is // made and removed when the corresponding ViewerHandle is destroyed. - std::vector<ViewRequestDelegate*> viewers_; + base::ObserverList<ViewRequestDelegate> viewers_; std::unique_ptr<Distiller> distiller_; bool blob_fetcher_running_;