Skip invalid DefaultService object path values.
If shill sends an invalid DefaultService object path value, we need to
explicitly treat since otherwise the DBus library will abort the
program when trying to send a message to the invalid object path.
Bug: chromium:526446
Change-Id: Id91787916b62cd94dab38532b98be0f0a8b6d4c4
Test: Added unittests
diff --git a/connection_manager.cc b/connection_manager.cc
index 99dbcb1..58e2292 100644
--- a/connection_manager.cc
+++ b/connection_manager.cc
@@ -164,16 +164,16 @@
bool ConnectionManager::GetConnectionProperties(
NetworkConnectionType* out_type,
NetworkTethering* out_tethering) {
- string default_service_path;
+ dbus::ObjectPath default_service_path;
TEST_AND_RETURN_FALSE(GetDefaultServicePath(&default_service_path));
- if (default_service_path.empty())
+ if (!default_service_path.IsValid())
return false;
TEST_AND_RETURN_FALSE(
GetServicePathProperties(default_service_path, out_type, out_tethering));
return true;
}
-bool ConnectionManager::GetDefaultServicePath(string* out_path) {
+bool ConnectionManager::GetDefaultServicePath(dbus::ObjectPath* out_path) {
chromeos::VariantDictionary properties;
chromeos::ErrorPtr error;
ManagerProxyInterface* manager_proxy = shill_proxy_->GetManagerProxy();
@@ -186,12 +186,12 @@
if (prop_default_service == properties.end())
return false;
- *out_path = prop_default_service->second.TryGet<dbus::ObjectPath>().value();
- return !out_path->empty();
+ *out_path = prop_default_service->second.TryGet<dbus::ObjectPath>();
+ return out_path->IsValid();
}
bool ConnectionManager::GetServicePathProperties(
- const string& path,
+ const dbus::ObjectPath& path,
NetworkConnectionType* out_type,
NetworkTethering* out_tethering) {
// We create and dispose the ServiceProxyInterface on every request.
@@ -227,8 +227,8 @@
properties.find(shill::kPhysicalTechnologyProperty);
if (prop_physical == properties.end()) {
LOG(ERROR) << "No PhysicalTechnology property found for a VPN"
- << " connection (service: " << path << "). Returning default"
- << " kUnknown value.";
+ " connection (service: "
+ << path.value() << "). Returning default kUnknown value.";
*out_type = NetworkConnectionType::kUnknown;
} else {
*out_type = ParseConnectionType(prop_physical->second.TryGet<string>());
diff --git a/connection_manager.h b/connection_manager.h
index cb638b6..2057f3b 100644
--- a/connection_manager.h
+++ b/connection_manager.h
@@ -20,6 +20,7 @@
#include <string>
#include <base/macros.h>
+#include <dbus/object_path.h>
#include "update_engine/connection_manager_interface.h"
#include "update_engine/shill_proxy_interface.h"
@@ -52,9 +53,9 @@
private:
// Returns (via out_path) the default network path, or empty string if
// there's no network up. Returns true on success.
- bool GetDefaultServicePath(std::string* out_path);
+ bool GetDefaultServicePath(dbus::ObjectPath* out_path);
- bool GetServicePathProperties(const std::string& path,
+ bool GetServicePathProperties(const dbus::ObjectPath& path,
NetworkConnectionType* out_type,
NetworkTethering* out_tethering);
diff --git a/connection_manager_unittest.cc b/connection_manager_unittest.cc
index ba4ad9a..2c1d881 100644
--- a/connection_manager_unittest.cc
+++ b/connection_manager_unittest.cc
@@ -129,7 +129,7 @@
EXPECT_CALL(*service_proxy_mock.get(), GetProperties(_, _, _))
.WillOnce(DoAll(SetArgPointee<0>(reply_dict), Return(true)));
- fake_shill_proxy_.SetServiceForPath(service_path,
+ fake_shill_proxy_.SetServiceForPath(dbus::ObjectPath(service_path),
std::move(service_proxy_mock));
}
@@ -137,8 +137,8 @@
const char* service_type,
const char* physical_technology,
NetworkConnectionType expected_type) {
- SetManagerReply("/service/guest-network", true);
- SetServiceReply("/service/guest-network",
+ SetManagerReply("/service/guest/network", true);
+ SetServiceReply("/service/guest/network",
service_type,
physical_technology,
shill::kTetheringNotDetectedState);
@@ -154,9 +154,9 @@
void ConnectionManagerTest::TestWithServiceTethering(
const char* service_tethering,
NetworkTethering expected_tethering) {
- SetManagerReply("/service/guest-network", true);
+ SetManagerReply("/service/guest/network", true);
SetServiceReply(
- "/service/guest-network", shill::kTypeWifi, nullptr, service_tethering);
+ "/service/guest/network", shill::kTypeWifi, nullptr, service_tethering);
NetworkConnectionType type;
NetworkTethering tethering;
@@ -399,7 +399,7 @@
}
TEST_F(ConnectionManagerTest, MalformedServiceList) {
- SetManagerReply("/service/guest-network", false);
+ SetManagerReply("/service/guest/network", false);
NetworkConnectionType type;
NetworkTethering tethering;
diff --git a/fake_shill_proxy.cc b/fake_shill_proxy.cc
index 7513599..17698cd 100644
--- a/fake_shill_proxy.cc
+++ b/fake_shill_proxy.cc
@@ -29,19 +29,19 @@
}
std::unique_ptr<ServiceProxyInterface> FakeShillProxy::GetServiceForPath(
- const std::string& path) {
- auto it = service_proxy_mocks_.find(path);
+ const dbus::ObjectPath& path) {
+ auto it = service_proxy_mocks_.find(path.value());
CHECK(it != service_proxy_mocks_.end()) << "No ServiceProxyMock set for "
- << path;
+ << path.value();
std::unique_ptr<ServiceProxyInterface> result = std::move(it->second);
service_proxy_mocks_.erase(it);
return result;
}
void FakeShillProxy::SetServiceForPath(
- const std::string& path,
+ const dbus::ObjectPath& path,
std::unique_ptr<ServiceProxyInterface> service_proxy) {
- service_proxy_mocks_[path] = std::move(service_proxy);
+ service_proxy_mocks_[path.value()] = std::move(service_proxy);
}
} // namespace chromeos_update_engine
diff --git a/fake_shill_proxy.h b/fake_shill_proxy.h
index 8daf7c2..ae17eaa 100644
--- a/fake_shill_proxy.h
+++ b/fake_shill_proxy.h
@@ -42,11 +42,11 @@
// with SetServiceForPath().
org::chromium::flimflam::ManagerProxyMock* GetManagerProxy() override;
std::unique_ptr<org::chromium::flimflam::ServiceProxyInterface>
- GetServiceForPath(const std::string& path) override;
+ GetServiceForPath(const dbus::ObjectPath& path) override;
// Sets the service_proxy that will be returned by GetServiceForPath().
void SetServiceForPath(
- const std::string& path,
+ const dbus::ObjectPath& path,
std::unique_ptr<org::chromium::flimflam::ServiceProxyInterface>
service_proxy);
diff --git a/shill_proxy.cc b/shill_proxy.cc
index 99817ea..1c050b4 100644
--- a/shill_proxy.cc
+++ b/shill_proxy.cc
@@ -35,10 +35,9 @@
}
std::unique_ptr<ServiceProxyInterface> ShillProxy::GetServiceForPath(
- const std::string& path) {
+ const dbus::ObjectPath& path) {
DCHECK(bus_.get());
- return std::unique_ptr<ServiceProxyInterface>(
- new ServiceProxy(bus_, dbus::ObjectPath(path)));
+ return std::unique_ptr<ServiceProxyInterface>(new ServiceProxy(bus_, path));
}
} // namespace chromeos_update_engine
diff --git a/shill_proxy.h b/shill_proxy.h
index 1fbcd2b..6d545f6 100644
--- a/shill_proxy.h
+++ b/shill_proxy.h
@@ -22,6 +22,7 @@
#include <base/macros.h>
#include <dbus/bus.h>
+#include <dbus/object_path.h>
#include <shill/dbus-proxies.h>
#include "update_engine/shill_proxy_interface.h"
@@ -41,7 +42,7 @@
// ShillProxyInterface overrides.
org::chromium::flimflam::ManagerProxyInterface* GetManagerProxy() override;
std::unique_ptr<org::chromium::flimflam::ServiceProxyInterface>
- GetServiceForPath(const std::string& path) override;
+ GetServiceForPath(const dbus::ObjectPath& path) override;
private:
// A reference to the main bus for creating new ServiceProxy instances.
diff --git a/shill_proxy_interface.h b/shill_proxy_interface.h
index 0092f12..5f6b44e 100644
--- a/shill_proxy_interface.h
+++ b/shill_proxy_interface.h
@@ -21,6 +21,7 @@
#include <string>
#include <base/macros.h>
+#include <dbus/object_path.h>
#include <shill/dbus-proxies.h>
namespace chromeos_update_engine {
@@ -41,7 +42,7 @@
// Return a ServiceProxy for the given path. The ownership of the returned
// instance is transferred to the caller.
virtual std::unique_ptr<org::chromium::flimflam::ServiceProxyInterface>
- GetServiceForPath(const std::string& path) = 0;
+ GetServiceForPath(const dbus::ObjectPath& path) = 0;
protected:
ShillProxyInterface() = default;
diff --git a/update_manager/real_shill_provider.cc b/update_manager/real_shill_provider.cc
index e3fa8f9..a38c006 100644
--- a/update_manager/real_shill_provider.cc
+++ b/update_manager/real_shill_provider.cc
@@ -20,6 +20,7 @@
#include <base/logging.h>
#include <base/strings/stringprintf.h>
+#include <chromeos/type_name_undecorate.h>
#include <shill/dbus-constants.h>
#include <shill/dbus-proxies.h>
@@ -88,8 +89,17 @@
void RealShillProvider::OnManagerPropertyChanged(const string& name,
const chromeos::Any& value) {
- if (name == shill::kDefaultServiceProperty)
- ProcessDefaultService(value.TryGet<dbus::ObjectPath>().value());
+ if (name == shill::kDefaultServiceProperty) {
+ dbus::ObjectPath service_path = value.TryGet<dbus::ObjectPath>();
+ if (!service_path.IsValid()) {
+ LOG(WARNING) << "Got an invalid DefaultService path. The property value "
+ "contains a "
+ << chromeos::UndecorateTypeName(value.GetType().name())
+ << ", read as the object path: '" << service_path.value()
+ << "'";
+ }
+ ProcessDefaultService(service_path);
+ }
}
void RealShillProvider::OnSignalConnected(const string& interface_name,
@@ -102,7 +112,7 @@
}
bool RealShillProvider::ProcessDefaultService(
- const string& default_service_path) {
+ const dbus::ObjectPath& default_service_path) {
// We assume that if the service path didn't change, then the connection
// type and the tethering status of it also didn't change.
if (default_service_path_ == default_service_path)
@@ -110,7 +120,8 @@
// Update the connection status.
default_service_path_ = default_service_path;
- bool is_connected = (default_service_path_ != "/");
+ bool is_connected = (default_service_path_.IsValid() &&
+ default_service_path_.value() != "/");
var_is_connected_.SetValue(is_connected);
var_conn_last_changed_.SetValue(clock_->GetWallclockTime());
@@ -140,8 +151,8 @@
// error in shill and the policy will handle it, but we will print a log
// message as well for accessing an unused variable.
var_conn_tethering_.UnsetValue();
- LOG(ERROR) << "Could not find connection type (" << default_service_path_
- << ")";
+ LOG(ERROR) << "Could not find connection type (service: "
+ << default_service_path_.value() << ")";
} else {
// If the property doesn't contain a string value, the empty string will
// become kUnknown.
@@ -153,8 +164,8 @@
const auto& prop_type = properties.find(shill::kTypeProperty);
if (prop_type == properties.end()) {
var_conn_type_.UnsetValue();
- LOG(ERROR) << "Could not find connection tethering mode ("
- << default_service_path_ << ")";
+ LOG(ERROR) << "Could not find connection tethering mode (service: "
+ << default_service_path_.value() << ")";
} else {
string type_str = prop_type->second.TryGet<string>();
if (type_str == shill::kTypeVPN) {
@@ -162,7 +173,7 @@
properties.find(shill::kPhysicalTechnologyProperty);
if (prop_physical == properties.end()) {
LOG(ERROR) << "No PhysicalTechnology property found for a VPN"
- << " connection (service: " << default_service_path
+ << " connection (service: " << default_service_path_.value()
<< "). Using default kUnknown value.";
var_conn_type_.SetValue(ConnectionType::kUnknown);
} else {
diff --git a/update_manager/real_shill_provider.h b/update_manager/real_shill_provider.h
index 2184ac2..aca4bae 100644
--- a/update_manager/real_shill_provider.h
+++ b/update_manager/real_shill_provider.h
@@ -24,6 +24,7 @@
#include <string>
#include <base/time/time.h>
+#include <dbus/object_path.h>
#include "update_engine/clock_interface.h"
#include "update_engine/shill_proxy_interface.h"
@@ -77,10 +78,10 @@
// Get the connection and populate the type and tethering status of the given
// default connection.
- bool ProcessDefaultService(const std::string& default_service_path);
+ bool ProcessDefaultService(const dbus::ObjectPath& default_service_path);
- // The current default service path, if connected.
- std::string default_service_path_;
+ // The current default service path, if connected. "/" means not connected.
+ dbus::ObjectPath default_service_path_{"uninitialized"};
// The mockable interface to access the shill DBus proxies, owned by the
// caller.
diff --git a/update_manager/real_shill_provider_unittest.cc b/update_manager/real_shill_provider_unittest.cc
index 4b4d0a7..cab4f60 100644
--- a/update_manager/real_shill_provider_unittest.cc
+++ b/update_manager/real_shill_provider_unittest.cc
@@ -47,13 +47,13 @@
namespace {
// Fake service paths.
-const char* const kFakeEthernetServicePath = "/fake-ethernet-service";
-const char* const kFakeWifiServicePath = "/fake-wifi-service";
-const char* const kFakeWimaxServicePath = "/fake-wimax-service";
-const char* const kFakeBluetoothServicePath = "/fake-bluetooth-service";
-const char* const kFakeCellularServicePath = "/fake-cellular-service";
-const char* const kFakeVpnServicePath = "/fake-vpn-service";
-const char* const kFakeUnknownServicePath = "/fake-unknown-service";
+const char* const kFakeEthernetServicePath = "/fake/ethernet/service";
+const char* const kFakeWifiServicePath = "/fake/wifi/service";
+const char* const kFakeWimaxServicePath = "/fake/wimax/service";
+const char* const kFakeBluetoothServicePath = "/fake/bluetooth/service";
+const char* const kFakeCellularServicePath = "/fake/cellular/service";
+const char* const kFakeVpnServicePath = "/fake/vpn/service";
+const char* const kFakeUnknownServicePath = "/fake/unknown/service";
} // namespace
@@ -259,7 +259,8 @@
.WillOnce(DoAll(SetArgPointee<0>(reply_dict), Return(true)));
fake_shill_proxy_.SetServiceForPath(
- service_path, chromeos::make_unique_ptr(service_proxy_mock));
+ dbus::ObjectPath(service_path),
+ chromeos::make_unique_ptr(service_proxy_mock));
return service_proxy_mock;
}
@@ -275,6 +276,29 @@
provider_->var_conn_last_changed());
}
+// Ensure that invalid DBus paths are ignored.
+TEST_F(UmRealShillProviderTest, InvalidServicePath) {
+ InitWithDefaultService("invalid");
+ UmTestUtils::ExpectVariableHasValue(false, provider_->var_is_connected());
+ UmTestUtils::ExpectVariableNotSet(provider_->var_conn_type());
+ UmTestUtils::ExpectVariableHasValue(InitTime(),
+ provider_->var_conn_last_changed());
+}
+
+// Ensure that a service path property including a different type is ignored.
+TEST_F(UmRealShillProviderTest, InvalidServicePathType) {
+ ManagerProxyMock* manager_proxy_mock = fake_shill_proxy_.GetManagerProxy();
+ chromeos::VariantDictionary reply_dict;
+ reply_dict[shill::kDefaultServiceProperty] = "/not/an/object/path";
+ EXPECT_CALL(*manager_proxy_mock, GetProperties(_, _, _))
+ .WillOnce(DoAll(SetArgPointee<0>(reply_dict), Return(true)));
+
+ EXPECT_TRUE(provider_->Init());
+ EXPECT_TRUE(loop_.RunOnce(false));
+
+ UmTestUtils::ExpectVariableHasValue(false, provider_->var_is_connected());
+}
+
// Test that Ethernet connection is identified correctly.
TEST_F(UmRealShillProviderTest, ReadConnTypeEthernet) {
InitWithDefaultService("/");