0

Fix dangling pointer in safe_browsing::LastDownloadFinder

Use a non-dereferenceable type as the key in the map holding the data
for Profiles with pending queries. Also use ScopedObservation to make
managing the Profile observations easier.

Bug: 339322848
Change-Id: If537fbe8c49c751b19480a97eb206b4b996e5886
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5544688
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Commit-Queue: Lily Chen <chlily@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1302865}
This commit is contained in:
Lily Chen
2024-05-17 21:33:46 +00:00
committed by Chromium LUCI CQ
parent 99fe5ddf4e
commit 78842ebdf8
2 changed files with 102 additions and 52 deletions
chrome/browser/safe_browsing/incident_reporting

@ -242,8 +242,6 @@ void PopulateNonBinaryDetailsFromRow(
LastDownloadFinder::~LastDownloadFinder() {
g_browser_process->profile_manager()->RemoveObserver(this);
for (const auto& state : profile_states_)
state.first->RemoveObserver(this);
}
// static
@ -254,8 +252,9 @@ std::unique_ptr<LastDownloadFinder> LastDownloadFinder::Create(
base::WrapUnique(new LastDownloadFinder(
std::move(download_details_getter), std::move(callback))));
// Return NULL if there is no work to do.
if (finder->profile_states_.empty())
if (finder->pending_profiles_.empty()) {
return nullptr;
}
return finder;
}
@ -276,35 +275,50 @@ LastDownloadFinder::LastDownloadFinder(
g_browser_process->profile_manager()->AddObserver(this);
}
LastDownloadFinder::PendingProfileData::PendingProfileData(
LastDownloadFinder* finder,
Profile* profile,
State state)
: state(state), observation(finder) {
observation.Observe(profile);
}
LastDownloadFinder::PendingProfileData::~PendingProfileData() = default;
// static
LastDownloadFinder::ProfileKey LastDownloadFinder::KeyForProfile(
Profile* profile) {
return ProfileKey(reinterpret_cast<std::uintptr_t>(profile));
}
void LastDownloadFinder::SearchInProfile(Profile* profile) {
// Do not look in OTR profiles or in profiles that do not participate in
// safe browsing extended reporting.
if (!IncidentReportingService::IsEnabledForProfile(profile))
return;
// Exit early if already processing this profile. This could happen if, for
// example, OnProfileAdded is called after construction while waiting for
// OnHistoryServiceLoaded.
if (profile_states_.count(profile))
// Try to initiate a metadata search.
ProfileKey profile_key = KeyForProfile(profile);
auto [iter, inserted] = pending_profiles_.try_emplace(
profile_key, this, profile, PendingProfileData::WAITING_FOR_METADATA);
// If the profile was already being processed, do nothing.
if (!inserted) {
return;
profile->AddObserver(this);
// Initiate a metadata search. As with IncidentReportingService, it's assumed
// that all passed profiles will outlive |this|.
profile_states_[profile] = WAITING_FOR_METADATA;
}
download_details_getter_.Run(
profile, base::BindOnce(&LastDownloadFinder::OnMetadataQuery,
weak_ptr_factory_.GetWeakPtr(), profile));
weak_ptr_factory_.GetWeakPtr(), profile_key));
}
void LastDownloadFinder::OnMetadataQuery(
Profile* profile,
ProfileKey profile_key,
std::unique_ptr<ClientIncidentReport_DownloadDetails> details) {
auto iter = profile_states_.find(profile);
auto iter = pending_profiles_.find(profile_key);
// Early-exit if the search for this profile was abandoned.
if (iter == profile_states_.end())
if (iter == pending_profiles_.end()) {
return;
}
if (details) {
if (IsMostInterestingBinary(*details, details_.get(),
@ -312,14 +326,14 @@ void LastDownloadFinder::OnMetadataQuery(
details_ = std::move(details);
most_recent_binary_row_.end_time = base::Time();
}
iter->second = WAITING_FOR_NON_BINARY_HISTORY;
iter->second.state = PendingProfileData::WAITING_FOR_NON_BINARY_HISTORY;
} else {
iter->second = WAITING_FOR_HISTORY;
iter->second.state = PendingProfileData::WAITING_FOR_HISTORY;
}
// Initiate a history search
history::HistoryService* history_service =
HistoryServiceFactory::GetForProfile(profile,
HistoryServiceFactory::GetForProfile(iter->second.profile(),
ServiceAccessType::IMPLICIT_ACCESS);
// No history service is returned for profiles that do not save history.
if (!history_service) {
@ -329,7 +343,7 @@ void LastDownloadFinder::OnMetadataQuery(
if (history_service->BackendLoaded()) {
history_service->QueryDownloads(
base::BindOnce(&LastDownloadFinder::OnDownloadQuery,
weak_ptr_factory_.GetWeakPtr(), profile));
weak_ptr_factory_.GetWeakPtr(), profile_key));
} else {
// else wait until history is loaded.
history_service_observations_.AddObservation(history_service);
@ -337,15 +351,16 @@ void LastDownloadFinder::OnMetadataQuery(
}
void LastDownloadFinder::OnDownloadQuery(
Profile* profile,
ProfileKey profile_key,
std::vector<history::DownloadRow> downloads) {
// Early-exit if the history search for this profile was abandoned.
auto iter = profile_states_.find(profile);
if (iter == profile_states_.end())
auto iter = pending_profiles_.find(profile_key);
if (iter == pending_profiles_.end()) {
return;
}
// Don't overwrite the download from metadata if it came from this profile.
if (iter->second == WAITING_FOR_HISTORY) {
if (iter->second.state == PendingProfileData::WAITING_FOR_HISTORY) {
// Find the most recent from this profile and use it if it's better than
// anything else found so far.
const history::DownloadRow* profile_best_binary =
@ -370,19 +385,19 @@ void LastDownloadFinder::OnDownloadQuery(
}
void LastDownloadFinder::RemoveProfileAndReportIfDone(
std::map<Profile*, ProfileWaitState>::iterator iter) {
DCHECK(iter != profile_states_.end());
iter->first->RemoveObserver(this);
profile_states_.erase(iter);
PendingProfilesMap::iterator iter) {
CHECK(iter != pending_profiles_.end());
pending_profiles_.erase(iter);
// Finish processing if all results are in.
if (profile_states_.empty())
if (pending_profiles_.empty()) {
ReportResults();
}
// Do not touch this LastDownloadFinder after reporting results.
}
void LastDownloadFinder::ReportResults() {
DCHECK(profile_states_.empty());
CHECK(pending_profiles_.empty());
std::unique_ptr<ClientIncidentReport_DownloadDetails> binary_details;
std::unique_ptr<ClientIncidentReport_NonBinaryDownloadDetails>
@ -414,25 +429,27 @@ void LastDownloadFinder::OnProfileAdded(Profile* profile) {
}
void LastDownloadFinder::OnProfileWillBeDestroyed(Profile* profile) {
// |profile| may not be present in the set of profiles.
auto iter = profile_states_.find(profile);
DCHECK(iter != profile_states_.end());
// If a Profile is about to be destroyed while we are observing it, the
// `profile` must be present in the map of pending queries.
auto iter = pending_profiles_.find(KeyForProfile(profile));
CHECK(iter != pending_profiles_.end());
RemoveProfileAndReportIfDone(iter);
}
void LastDownloadFinder::OnHistoryServiceLoaded(
history::HistoryService* history_service) {
for (const auto& pair : profile_states_) {
for (auto& [profile_key, profile_data] : pending_profiles_) {
history::HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
pair.first, ServiceAccessType::EXPLICIT_ACCESS);
profile_data.profile(), ServiceAccessType::EXPLICIT_ACCESS);
if (hs == history_service) {
// Start the query in the history service if the finder was waiting for
// the service to load.
if (pair.second == WAITING_FOR_HISTORY ||
pair.second == WAITING_FOR_NON_BINARY_HISTORY) {
if (profile_data.state == PendingProfileData::WAITING_FOR_HISTORY ||
profile_data.state ==
PendingProfileData::WAITING_FOR_NON_BINARY_HISTORY) {
history_service->QueryDownloads(
base::BindOnce(&LastDownloadFinder::OnDownloadQuery,
weak_ptr_factory_.GetWeakPtr(), pair.first));
weak_ptr_factory_.GetWeakPtr(), profile_key));
}
return;
}

@ -5,6 +5,8 @@
#ifndef CHROME_BROWSER_SAFE_BROWSING_INCIDENT_REPORTING_LAST_DOWNLOAD_FINDER_H_
#define CHROME_BROWSER_SAFE_BROWSING_INCIDENT_REPORTING_LAST_DOWNLOAD_FINDER_H_
#include <stdint.h>
#include <map>
#include <memory>
#include <vector>
@ -12,6 +14,8 @@
#include "base/functional/callback.h"
#include "base/memory/weak_ptr.h"
#include "base/scoped_multi_source_observation.h"
#include "base/scoped_observation.h"
#include "base/types/strong_alias.h"
#include "chrome/browser/profiles/profile_manager_observer.h"
#include "chrome/browser/profiles/profile_observer.h"
#include "chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.h"
@ -65,15 +69,43 @@ class LastDownloadFinder : public ProfileManagerObserver,
LastDownloadFinder();
private:
enum ProfileWaitState {
WAITING_FOR_METADATA,
WAITING_FOR_HISTORY,
WAITING_FOR_NON_BINARY_HISTORY,
// Holds state for a Profile for which a query is currently pending. The State
// describes what we are waiting for. The observation is a mechanism to ensure
// that we do not attempt to process the results of a search for a Profile
// that is no longer valid. This guarantees that a PendingProfileData exists
// iff the Profile is alive and we are waiting on a query for it.
struct PendingProfileData {
enum State {
WAITING_FOR_METADATA,
WAITING_FOR_HISTORY,
WAITING_FOR_NON_BINARY_HISTORY,
};
// Makes the `finder` start observing `profile`.
PendingProfileData(LastDownloadFinder* finder,
Profile* profile,
State state);
~PendingProfileData();
Profile* profile() { return observation.GetSource(); }
State state;
base::ScopedObservation<Profile, LastDownloadFinder> observation;
};
// We identify Profiles by their addresses, in the form of a uintptr_t value,
// which is derived from a Profile* but is not to be dereferenced.
using ProfileKey = base::StrongAlias<class ProfileKeyTag, uintptr_t>;
using PendingProfilesMap = std::map<ProfileKey, PendingProfileData>;
LastDownloadFinder(DownloadDetailsGetter download_details_getter,
LastDownloadCallback callback);
// Returns the address of the Profile in a safe form to be used as a map key.
static inline ProfileKey KeyForProfile(Profile* profile);
// Adds |profile| to the set of profiles to be searched if it is an
// on-the-record profile with history that participates in safe browsing
// extended reporting. A search for metadata is initiated immediately.
@ -84,19 +116,19 @@ class LastDownloadFinder : public ProfileManagerObserver,
// begins a search in history. Reports results if there are no more pending
// queries.
void OnMetadataQuery(
Profile* profile,
ProfileKey profile_key,
std::unique_ptr<ClientIncidentReport_DownloadDetails> details);
// HistoryService::DownloadQueryCallback. Retrieves the most recent completed
// executable download from |downloads| and reports results if there are no
// more pending queries.
void OnDownloadQuery(Profile* profile,
void OnDownloadQuery(ProfileKey profile_key,
std::vector<history::DownloadRow> downloads);
// Removes the profile pointed to by |it| from profile_states_ and reports
// results if there are no more pending queries.
void RemoveProfileAndReportIfDone(
std::map<Profile*, ProfileWaitState>::iterator iter);
// Severs ties with the Profile whose state is pointed at by `iter` within
// `pending_profiles_`, either after a query finishes or to abandon an ongoing
// query. Also reports results if there are no more pending queries.
void RemoveProfileAndReportIfDone(PendingProfilesMap::iterator iter);
// Invokes the caller-supplied callback with the download found.
void ReportResults();
@ -120,9 +152,10 @@ class LastDownloadFinder : public ProfileManagerObserver,
// found.
LastDownloadCallback callback_;
// A mapping of profiles for which a download query is pending to their
// respective states.
std::map<Profile*, ProfileWaitState> profile_states_;
// A mapping of profiles for which a query is pending to their respective
// states. Items are removed from here when a query finishes, or when we
// abandon a query.
PendingProfilesMap pending_profiles_;
// The most interesting download details retrieved from download metadata.
std::unique_ptr<ClientIncidentReport_DownloadDetails> details_;