From 9af5bdc7d6e1dd2e393e7a8ec2824e83bb516284 Mon Sep 17 00:00:00 2001 From: Florian Leimgruber Date: Thu, 29 Aug 2024 18:21:51 +0000 Subject: [PATCH] Fix dangling pointer in IndependentFlattener IndependentFlattener keeps a raw_ptr to MetricsLog. For it not to become dangling, the MetricsLog needs to outlive the flattener, which is currently not the case. There are a few different problems, as explained below. In all cases, the IndependentFlattener is indirectly owned by a MetricsLogHistogramWriter. - SnapshotUnloggedSamplesAndFinalizeLog( MetricsLogHistogramWriter* log_histogram_writer, std::unique_ptr log, ...) This is problematic from an interface standpoint already, since the log is destroyed at the end of the function's scope, while the unowned log_histogram_writer is kept alive (by whoever owns it on the caller side). - MetricsService::SnapshotDeltasAndFinalizeLog( std::unique_ptr log_histogram_writer, std::unique_ptr log, ...) { ... return FinalizeLog(std::move(log), ...); } This is broken for a similar reason: As ownership is transferred to FinalizeLog(), log is destroyed when FinalizeLog() returns, but log_histogram_writer is only destroyed when SnapshotDeltasAndFinalizeLog() returns slightly afterwards. Both cases are fixed by resetting the MetricsLogHistogramWriter's flattener before FinalizeLog(). Fixed: 41485234 Change-Id: I60d13e8237d90a09b3cc1e82d43d6c3587610993 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5808942 Commit-Queue: Alexei Svitkine Reviewed-by: Alexei Svitkine Cr-Commit-Position: refs/heads/main@{#1348776} --- base/metrics/histogram_snapshot_manager.h | 6 +++++- components/metrics/metrics_service.cc | 16 +++++++++++++--- components/metrics/metrics_service.h | 7 ++++++- 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/base/metrics/histogram_snapshot_manager.h b/base/metrics/histogram_snapshot_manager.h index 33772cc295a1fd..155468cf42bca7 100644 --- a/base/metrics/histogram_snapshot_manager.h +++ b/base/metrics/histogram_snapshot_manager.h @@ -67,6 +67,10 @@ class BASE_EXPORT HistogramSnapshotManager final { void PrepareDelta(HistogramBase* histogram); void PrepareFinalDelta(const HistogramBase* histogram); + // Used to avoid a dangling pointer `histogram_flattener_` if the referenced + // `HistogramFlattener` is destroyed first. + void ResetFlattener() { histogram_flattener_ = nullptr; } + private: FRIEND_TEST_ALL_PREFIXES(HistogramSnapshotManagerTest, CheckMerge); @@ -91,7 +95,7 @@ class BASE_EXPORT HistogramSnapshotManager final { // |histogram_flattener_| handles the logistics of recording the histogram // deltas. - const raw_ptr histogram_flattener_; // Weak. + raw_ptr histogram_flattener_; // Weak. }; } // namespace base diff --git a/components/metrics/metrics_service.cc b/components/metrics/metrics_service.cc index c8cb1395148394..8e5c1684153d5b 100644 --- a/components/metrics/metrics_service.cc +++ b/components/metrics/metrics_service.cc @@ -196,7 +196,7 @@ class IndependentFlattener : public base::HistogramFlattener { } private: - const raw_ptr log_; + const raw_ptr log_; }; // Used to mark histogram samples as reported so that they are not included in @@ -985,13 +985,21 @@ void MetricsService::MetricsLogHistogramWriter:: required_flags_, histogram_snapshot_manager_.get()); } +void MetricsService::MetricsLogHistogramWriter::NotifyLogBeingFinalized() { + // Since the `flattener_` references the `log`, make sure it is destroyed so + // the pointer doesn't become dangling. + histogram_snapshot_manager()->ResetFlattener(); + flattener_.reset(); +} + MetricsService::IndependentMetricsLoader::IndependentMetricsLoader( std::unique_ptr log, std::string app_version, std::string signing_key) : log_(std::move(log)), - flattener_(new IndependentFlattener(log_.get())), - snapshot_manager_(new base::HistogramSnapshotManager(flattener_.get())), + flattener_(std::make_unique(log_.get())), + snapshot_manager_( + std::make_unique(flattener_.get())), app_version_(std::move(app_version)), signing_key_(std::move(signing_key)) { CHECK(log_); @@ -1654,6 +1662,7 @@ MetricsService::FinalizedLog MetricsService::SnapshotDeltasAndFinalizeLog( std::string&& current_app_version, std::string&& signing_key) { log_histogram_writer->SnapshotStatisticsRecorderDeltas(); + log_histogram_writer->NotifyLogBeingFinalized(); return FinalizeLog(std::move(log), truncate_events, std::move(close_time), current_app_version, signing_key); } @@ -1668,6 +1677,7 @@ MetricsService::SnapshotUnloggedSamplesAndFinalizeLog( std::string&& current_app_version, std::string&& signing_key) { log_histogram_writer->SnapshotStatisticsRecorderUnloggedSamples(); + log_histogram_writer->NotifyLogBeingFinalized(); return FinalizeLog(std::move(log), truncate_events, std::move(close_time), current_app_version, signing_key); } diff --git a/components/metrics/metrics_service.h b/components/metrics/metrics_service.h index 97f207c47c883d..38968e637d2f9c 100644 --- a/components/metrics/metrics_service.h +++ b/components/metrics/metrics_service.h @@ -383,13 +383,18 @@ class MetricsService { return snapshot_transaction_id_; } + // Notifies the histogram writer that the `log` passed in through the + // constructor is about to be destroyed. + void NotifyLogBeingFinalized(); + private: // Used to select which histograms to record when calling // SnapshotStatisticsRecorderHistograms() or // SnapshotStatisticsRecorderUnloggedSamples(). const base::HistogramBase::Flags required_flags_; - // Used to write histograms to the log passed in the constructor. + // Used to write histograms to the log passed in the constructor. Null after + // `NotifyLogBeingFinalized()`. std::unique_ptr flattener_; // Used to snapshot histograms.