Skip to content

Commit

Permalink
Fix UAF in PerformanceControlsHatsService
Browse files Browse the repository at this point in the history
Sometimes the UserPerformanceTuningManager is reset before the
PerformanceControlsHatsService is destroyed. In that case, this
causes a UAF error. I was able to reproduce the issue locally
with an asan build and confirmed that with this change, the
error did not occur.

Bug: 1422876
Change-Id: I4368434d5dec20d5e468152c850d284243ddd720
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4330509
Commit-Queue: Alison Gale <agale@chromium.org>
Reviewed-by: Eshwar Stalin <estalin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1118327}
  • Loading branch information
Alison Gale authored and Chromium LUCI CQ committed Mar 16, 2023
1 parent c774694 commit 23097a4
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ PerformanceControlsHatsService::PerformanceControlsHatsService(Profile* profile)
base::FeatureList::IsEnabled(
performance_manager::features::
kPerformanceControlsHighEfficiencyOptOutSurvey)) {
auto* manager = performance_manager::user_tuning::
UserPerformanceTuningManager::GetInstance();
user_performance_tuning_manager_observation_.Observe(manager);
performance_manager::user_tuning::UserPerformanceTuningManager::
GetInstance()
->AddObserver(this);
}

local_pref_registrar_.Init(local_state);
Expand All @@ -47,6 +47,15 @@ PerformanceControlsHatsService::PerformanceControlsHatsService(Profile* profile)

PerformanceControlsHatsService::~PerformanceControlsHatsService() {
local_pref_registrar_.RemoveAll();

// Can't used ScopedObservation because sometimes the
// UserPerformanceTuningManager is destroyed before this service.
if (performance_manager::user_tuning::UserPerformanceTuningManager::
HasInstance()) {
performance_manager::user_tuning::UserPerformanceTuningManager::
GetInstance()
->RemoveObserver(this);
}
}

void PerformanceControlsHatsService::OnBatterySaverModeChange() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
#ifndef CHROME_BROWSER_UI_PERFORMANCE_CONTROLS_PERFORMANCE_CONTROLS_HATS_SERVICE_H_
#define CHROME_BROWSER_UI_PERFORMANCE_CONTROLS_PERFORMANCE_CONTROLS_HATS_SERVICE_H_

#include "base/scoped_observation.h"
#include "chrome/browser/performance_manager/public/user_tuning/user_performance_tuning_manager.h"
#include "chrome/browser/profiles/profile.h"
#include "components/keyed_service/core/keyed_service.h"
Expand Down Expand Up @@ -35,11 +34,6 @@ class PerformanceControlsHatsService
private:
raw_ptr<Profile> profile_;
PrefChangeRegistrar local_pref_registrar_;

base::ScopedObservation<
performance_manager::user_tuning::UserPerformanceTuningManager,
performance_manager::user_tuning::UserPerformanceTuningManager::Observer>
user_performance_tuning_manager_observation_{this};
};

#endif // CHROME_BROWSER_UI_PERFORMANCE_CONTROLS_PERFORMANCE_CONTROLS_HATS_SERVICE_H_

0 comments on commit 23097a4

Please sign in to comment.