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/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("/");