[go: up one dir, main page]

Fix a possible out-of-bound access in components/page_load_metrics

`new_timings` is iterated at
`DispatchEventsAfterBackForwardCacheRestore` and the index is used for
`back_forward_cache_restores_` at
`PageLoadTracker::GetBackForwardCacheRestore`.  On the other hand,
`new_timings` can be updated via mojo message
`PageLoadMetrics.UpdateTiming`, then in theory it is possible to make
`new_timings` larger than `back_forward_cache_restores_`. This could
cause an out-of-bound access.

This change fixes this out-of-bound access to
`back_forward_cache_restores_` by adding boundary checks.

(cherry picked from commit 1059bf191cc8c7a8a10fd667522b6949e8de0f35)

Bug: 1272403
Change-Id: I9eae03f55b63f353f36d8372930f16034dcea7a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3299751
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
Reviewed-by: Nicolás Peña Moreno <npm@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#944966}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3307157
Commit-Queue: Hajime Hoshi <hajimehoshi@chromium.org>
Cr-Commit-Position: refs/branch-heads/4664@{#1216}
Cr-Branched-From: 24dc4ee75e01a29d390d43c9c264372a169273a7-refs/heads/main@{#929512}
diff --git a/components/page_load_metrics/browser/observers/back_forward_cache_page_load_metrics_observer.cc b/components/page_load_metrics/browser/observers/back_forward_cache_page_load_metrics_observer.cc
index c1b857ae..8e0a3de5 100644
--- a/components/page_load_metrics/browser/observers/back_forward_cache_page_load_metrics_observer.cc
+++ b/components/page_load_metrics/browser/observers/back_forward_cache_page_load_metrics_observer.cc
@@ -178,6 +178,8 @@
     OnFirstPaintAfterBackForwardCacheRestoreInPage(
         const page_load_metrics::mojom::BackForwardCacheTiming& timing,
         size_t index) {
+  if (index >= back_forward_cache_navigation_ids_.size())
+    return;
   auto first_paint = timing.first_paint_after_back_forward_cache_restore;
   DCHECK(!first_paint.is_zero());
   if (page_load_metrics::
@@ -210,6 +212,8 @@
     OnRequestAnimationFramesAfterBackForwardCacheRestoreInPage(
         const page_load_metrics::mojom::BackForwardCacheTiming& timing,
         size_t index) {
+  if (index >= back_forward_cache_navigation_ids_.size())
+    return;
   auto request_animation_frames =
       timing.request_animation_frames_after_back_forward_cache_restore;
   DCHECK_EQ(request_animation_frames.size(), 3u);
@@ -244,6 +248,8 @@
     OnFirstInputAfterBackForwardCacheRestoreInPage(
         const page_load_metrics::mojom::BackForwardCacheTiming& timing,
         size_t index) {
+  if (index >= back_forward_cache_navigation_ids_.size())
+    return;
   auto first_input_delay =
       timing.first_input_delay_after_back_forward_cache_restore;
   DCHECK(first_input_delay.has_value());
diff --git a/components/page_load_metrics/browser/page_load_tracker.cc b/components/page_load_metrics/browser/page_load_tracker.cc
index e2c97d6..cc4af1a 100644
--- a/components/page_load_metrics/browser/page_load_tracker.cc
+++ b/components/page_load_metrics/browser/page_load_tracker.cc
@@ -14,6 +14,7 @@
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/notreached.h"
+#include "base/strings/stringprintf.h"
 #include "base/time/default_tick_clock.h"
 #include "base/trace_event/trace_event.h"
 #include "components/page_load_metrics/browser/page_load_metrics_embedder_interface.h"
@@ -131,7 +132,13 @@
         last_timings,
     const std::vector<mojo::StructPtr<mojom::BackForwardCacheTiming>>&
         new_timings) {
-  DCHECK_GE(new_timings.size(), last_timings.size());
+  if (new_timings.size() < last_timings.size()) {
+    mojo::ReportBadMessage(base::StringPrintf(
+        "`new_timings.size()` (%zu) must be equal to or greater than "
+        "`last_timings.size()` (%zu) but is not",
+        new_timings.size(), last_timings.size()));
+    return;
+  }
 
   for (size_t i = 0; i < new_timings.size(); i++) {
     auto first_paint =