[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 <[email protected]>
Commit-Queue: Akhila Veerapuraju <[email protected]>
Cr-Commit-Position: refs/heads/master@{#877492}
Reviewed-by: Allan Sandfeld Jensen <[email protected]>

Author: dhveerap

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);
   }
 }
 

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_;