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 =