Skip to content

Commit

Permalink
NetworkStateHandler: Protect observer calls
Browse files Browse the repository at this point in the history 
An edge case was recently introduced where a call into
NetworkStateHandler from NetworkConnectionStateChanged or
NetworkPropertiesUpdated might trigger EnsureCellularNetwork
which might delete the network passed to other observers.

We have also seen a handful of crashes in these observers
that may be caused by a different edge case.

This CL fixes the known issue and forces a crash for any
existing or future regressions.

Bug: 774430
Change-Id: I03ecf5ab46c572ee966be1f3c049c6d0906ac9e8
Reviewed-on: https://chromium-review.googlesource.com/798636
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520740}
  • Loading branch information
@stevenjb
stevenjb authored and Commit Bot committed Nov 30, 2017
1 parent a4364d1 commit 9c99751
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 9 deletions.
37 changes: 29 additions & 8 deletions chromeos/network/network_state_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,9 @@ bool ShouldIncludeNetworkInList(const NetworkState* network_state,
const char NetworkStateHandler::kDefaultCheckPortalList[] =
"ethernet,wifi,cellular";

NetworkStateHandler::NetworkStateHandler() = default;
NetworkStateHandler::NetworkStateHandler() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
}

NetworkStateHandler::~NetworkStateHandler() {
// Normally Shutdown() will get called in ~NetworkHandler, however unit
Expand Down Expand Up @@ -311,8 +313,9 @@ const NetworkState* NetworkStateHandler::DefaultNetwork() const {

const NetworkState* NetworkStateHandler::ConnectedNetworkByType(
const NetworkTypePattern& type) {
// Sort to ensure visible networks are listed first.
if (!network_list_sorted_)
SortNetworkList(); // Sort to ensure visible networks are listed first.
SortNetworkList(false /* ensure_cellular */);

const NetworkState* connected_network = nullptr;
for (auto iter = network_list_.begin(); iter != network_list_.end(); ++iter) {
Expand Down Expand Up @@ -387,8 +390,9 @@ const NetworkState* NetworkStateHandler::ConnectingNetworkByType(

const NetworkState* NetworkStateHandler::FirstNetworkByType(
const NetworkTypePattern& type) {
// Sort to ensure visible networks are listed first.
if (!network_list_sorted_)
SortNetworkList(); // Sort to ensure visible networks are listed first.
SortNetworkList(false /* ensure_cellular */);

const NetworkState* first_network = nullptr;
for (auto iter = network_list_.begin(); iter != network_list_.end(); ++iter) {
Expand Down Expand Up @@ -461,6 +465,7 @@ void NetworkStateHandler::GetNetworkListByType(const NetworkTypePattern& type,
bool visible_only,
size_t limit,
NetworkStateList* list) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(list);
list->clear();

Expand All @@ -470,7 +475,7 @@ void NetworkStateHandler::GetNetworkListByType(const NetworkTypePattern& type,
limit = std::numeric_limits<size_t>::max();

if (!network_list_sorted_)
SortNetworkList();
SortNetworkList(false /* ensure_cellular */);

// First, add active Tether networks.
if (type.MatchesPattern(NetworkTypePattern::Tether()))
Expand Down Expand Up @@ -567,6 +572,7 @@ void NetworkStateHandler::AddTetherNetworkState(const std::string& guid,
int battery_percentage,
int signal_strength,
bool has_connected_to_host) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(!guid.empty());
DCHECK(battery_percentage >= 0 && battery_percentage <= 100);
DCHECK(signal_strength >= 0 && signal_strength <= 100);
Expand Down Expand Up @@ -654,6 +660,8 @@ bool NetworkStateHandler::SetTetherNetworkHasConnectedToHost(
}

bool NetworkStateHandler::RemoveTetherNetworkState(const std::string& guid) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
CHECK(!notifying_network_observers_);
for (auto iter = tether_network_list_.begin();
iter != tether_network_list_.end(); ++iter) {
if (iter->get()->AsNetworkState()->guid() == guid) {
Expand Down Expand Up @@ -996,6 +1004,7 @@ void NetworkStateHandler::SetLastErrorForTest(const std::string& service_path,

void NetworkStateHandler::UpdateManagedList(ManagedState::ManagedType type,
const base::ListValue& entries) {
CHECK(!notifying_network_observers_);
ManagedStateList* managed_list = GetManagedList(type);
NET_LOG_DEBUG("UpdateManagedList: " + ManagedState::TypeToString(type),
base::StringPrintf("%" PRIuS, entries.GetSize()));
Expand Down Expand Up @@ -1264,7 +1273,7 @@ void NetworkStateHandler::ManagedStateListChanged(
ManagedState::ManagedType type) {
SCOPED_NET_LOG_IF_SLOW();
if (type == ManagedState::MANAGED_TYPE_NETWORK) {
SortNetworkList();
SortNetworkList(true /* ensure_cellular */);
UpdateNetworkStats();
NotifyNetworkListChanged();
} else if (type == ManagedState::MANAGED_TYPE_DEVICE) {
Expand All @@ -1281,7 +1290,8 @@ void NetworkStateHandler::ManagedStateListChanged(
}
}

void NetworkStateHandler::SortNetworkList() {
void NetworkStateHandler::SortNetworkList(bool ensure_cellular) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (tether_sort_delegate_)
tether_sort_delegate_->SortTetherNetworkList(&tether_network_list_);

Expand Down Expand Up @@ -1317,7 +1327,8 @@ void NetworkStateHandler::SortNetworkList() {
hidden.push_back(std::move(*iter));
}
}
EnsureCellularNetwork(&cellular);
if (ensure_cellular)
EnsureCellularNetwork(&cellular);
// List active non Cellular network first.
network_list_ = std::move(active);
// Ethernet is always active so list any Cellular network next.
Expand Down Expand Up @@ -1448,6 +1459,8 @@ void NetworkStateHandler::UpdateGuid(NetworkState* network) {

void NetworkStateHandler::EnsureCellularNetwork(
ManagedStateList* cellular_networks) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
CHECK(!notifying_network_observers_);
const DeviceState* device =
GetDeviceStateByType(NetworkTypePattern::Cellular());
if (!device) {
Expand Down Expand Up @@ -1538,6 +1551,7 @@ NetworkState* NetworkStateHandler::GetModifiableNetworkStateFromGuid(
ManagedState* NetworkStateHandler::GetModifiableManagedState(
const ManagedStateList* managed_list,
const std::string& path) const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
for (auto iter = managed_list->begin(); iter != managed_list->end(); ++iter) {
ManagedState* managed = iter->get();
if (managed->path() == path)
Expand All @@ -1548,6 +1562,7 @@ ManagedState* NetworkStateHandler::GetModifiableManagedState(

NetworkStateHandler::ManagedStateList* NetworkStateHandler::GetManagedList(
ManagedState::ManagedType type) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
switch (type) {
case ManagedState::MANAGED_TYPE_NETWORK:
return &network_list_;
Expand Down Expand Up @@ -1577,7 +1592,7 @@ bool NetworkStateHandler::OnNetworkConnectionStateChanged(
<< GetLogName(network)
<< "State: " << network->connection_state();
default_network_path_.clear();
SortNetworkList();
SortNetworkList(true /* ensure_cellular */);
NotifyDefaultNetworkChanged(nullptr);
}
}
Expand All @@ -1586,8 +1601,10 @@ bool NetworkStateHandler::OnNetworkConnectionStateChanged(
desc = "Default" + desc;
NET_LOG(EVENT) << "NOTIFY: " << desc << ": " << GetLogName(network) << ": "
<< network->connection_state();
notifying_network_observers_ = true;
for (auto& observer : observers_)
observer.NetworkConnectionStateChanged(network);
notifying_network_observers_ = false;
if (notify_default)
NotifyDefaultNetworkChanged(network);
return notify_default;
Expand All @@ -1597,16 +1614,20 @@ void NetworkStateHandler::NotifyDefaultNetworkChanged(
const NetworkState* default_network) {
SCOPED_NET_LOG_IF_SLOW();
NET_LOG_EVENT("NOTIFY:DefaultNetworkChanged", GetLogName(default_network));
notifying_network_observers_ = true;
for (auto& observer : observers_)
observer.DefaultNetworkChanged(default_network);
notifying_network_observers_ = false;
}

void NetworkStateHandler::NotifyNetworkPropertiesUpdated(
const NetworkState* network) {
SCOPED_NET_LOG_IF_SLOW();
NET_LOG_DEBUG("NOTIFY:NetworkPropertiesUpdated", GetLogName(network));
notifying_network_observers_ = true;
for (auto& observer : observers_)
observer.NetworkPropertiesUpdated(network);
notifying_network_observers_ = false;
}

void NetworkStateHandler::NotifyDevicePropertiesUpdated(
Expand Down
10 changes: 9 additions & 1 deletion chromeos/network/network_state_handler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "base/gtest_prod_util.h"
#include "base/macros.h"
#include "base/observer_list.h"
#include "base/sequence_checker.h"
#include "chromeos/chromeos_export.h"
#include "chromeos/network/managed_state.h"
#include "chromeos/network/network_handler.h"
Expand Down Expand Up @@ -422,7 +423,9 @@ class CHROMEOS_EXPORT NetworkStateHandler
// * Visible non-wifi networks
// * Visible wifi networks
// * Hidden (wifi) networks
void SortNetworkList();
// If |ensure_cellular| is true, call EnsureCellularNetwork (which may
// remove a network from the list).
void SortNetworkList(bool ensure_cellular);

// Updates UMA stats. Called once after all requested networks are updated.
void UpdateNetworkStats();
Expand Down Expand Up @@ -556,6 +559,11 @@ class CHROMEOS_EXPORT NetworkStateHandler
// Ensure that Shutdown() gets called exactly once.
bool did_shutdown_ = false;

// Ensure that we do not delete any networks while notifying observers.
bool notifying_network_observers_ = false;

SEQUENCE_CHECKER(sequence_checker_);

DISALLOW_COPY_AND_ASSIGN(NetworkStateHandler);
};

Expand Down

0 comments on commit 9c99751

Please sign in to comment.