Fix memory leak on ConnectionManagerTest.
ConnectionManagerTest::SetServiceReply had a memory leak only present
on unittest code. This patch uses g_new0() and g_free() to manage the
memory of GValue objects on GValueNewString and GValueFree, which
allows to use GValueFree to remove any GValue object.
It also replaces the GArray* by a GPtrArray which is what dbus-glib
uses and what is being passed as the GType, as defined on the
DBusWrapperInterface.
BUG=chromium:378548
TEST=`FEATURES="test" USE="clang asan" emerge-link update_engine` doesn't show ConnectionManager failures
TEST=tested on link the update_engine detects the current connection from shill.
Change-Id: I328ba79e2d609ab1cfe3df8b99b82fa24262de08
Reviewed-on: https://chromium-review.googlesource.com/202060
Reviewed-by: Alex Deymo <deymo@chromium.org>
Commit-Queue: Alex Deymo <deymo@chromium.org>
Tested-by: Alex Deymo <deymo@chromium.org>
diff --git a/connection_manager.cc b/connection_manager.cc
index 45c971a..9e3ac1e 100644
--- a/connection_manager.cc
+++ b/connection_manager.cc
@@ -84,12 +84,12 @@
GValue* value = reinterpret_cast<GValue*>(g_hash_table_lookup(hash_table,
"Services"));
- GArray* array = NULL;
+ GPtrArray* array = NULL;
bool success = false;
if (G_VALUE_HOLDS(value, DBUS_TYPE_G_OBJECT_PATH_ARRAY) &&
- (array = reinterpret_cast<GArray*>(g_value_get_boxed(value))) &&
+ (array = reinterpret_cast<GPtrArray*>(g_value_get_boxed(value))) &&
(array->len > 0)) {
- *out_path = g_array_index(array, const char*, 0);
+ *out_path = static_cast<const char*>(g_ptr_array_index(array, 0));
success = true;
}
diff --git a/connection_manager_unittest.cc b/connection_manager_unittest.cc
index 6b3e26b..c495fbf 100644
--- a/connection_manager_unittest.cc
+++ b/connection_manager_unittest.cc
@@ -4,12 +4,14 @@
#include <base/logging.h>
#include <chromeos/dbus/service_constants.h>
+#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include <string>
#include "update_engine/connection_manager.h"
#include "update_engine/fake_system_state.h"
#include "update_engine/mock_dbus_wrapper.h"
+#include "update_engine/test_utils.h"
using std::set;
using std::string;
@@ -33,7 +35,7 @@
protected:
void SetupMocks(const char* service_path);
- void SetManagerReply(gconstpointer value, const GType& type);
+ void SetManagerReply(const char* reply_value, const GType& reply_type);
// Sets the |service_type| Type and the |physical_technology|
// PhysicalTechnology properties in the mocked service. If a NULL
@@ -54,7 +56,7 @@
DBusGProxy* kMockFlimFlamServiceProxy_;
DBusGConnection* kMockSystemBus_;
const char* kServicePath_;
- MockDBusWrapper dbus_iface_;
+ testing::StrictMock<MockDBusWrapper> dbus_iface_;
ConnectionManager cmut_; // ConnectionManager under test.
FakeSystemState fake_system_state_;
};
@@ -77,19 +79,29 @@
.Times(AnyNumber());
}
-void ConnectionManagerTest::SetManagerReply(gconstpointer reply_value,
+void ConnectionManagerTest::SetManagerReply(const char *reply_value,
const GType& reply_type) {
- // Initialize return value for D-Bus call to Manager object.
- // TODO (jaysri): Free the objects allocated here.
- GHashTable* manager_hash_table = g_hash_table_new(g_str_hash, g_str_equal);
+ ASSERT_TRUE(dbus_g_type_is_collection(reply_type));
- GArray* array = g_array_new(FALSE, FALSE, sizeof(const char*));
+ // Create the GPtrArray array holding the |reply_value| pointer. The
+ // |reply_value| string is duplicated because it should be mutable on the
+ // interface and is because dbus-glib collections will g_free() each element
+ // of the GPtrArray automatically when the |array_as_value| GValue is unset.
+ // The g_strdup() is not being leaked.
+ GPtrArray* array = g_ptr_array_new();
ASSERT_TRUE(array != NULL);
+ g_ptr_array_add(array, g_strdup(reply_value));
- EXPECT_EQ(array, g_array_append_val(array, reply_value));
GValue* array_as_value = g_new0(GValue, 1);
EXPECT_EQ(array_as_value, g_value_init(array_as_value, reply_type));
g_value_take_boxed(array_as_value, array);
+
+ // Initialize return value for D-Bus call to Manager object, which is a
+ // hash table of static strings (char*) in GValue* containing a single array.
+ GHashTable* manager_hash_table = g_hash_table_new_full(
+ g_str_hash, g_str_equal,
+ nullptr, // no key_destroy_func because keys are static.
+ GValueFree); // value_destroy_func
g_hash_table_insert(manager_hash_table,
const_cast<char*>("Services"),
array_as_value);
@@ -115,36 +127,26 @@
void ConnectionManagerTest::SetServiceReply(const char* service_type,
const char* physical_technology,
const char* service_tethering) {
- // Initialize return value for D-Bus call to Service object.
- // TODO (jaysri): Free the objects allocated here.
- GHashTable* service_hash_table = g_hash_table_new(g_str_hash, g_str_equal);
-
- GValue* service_type_value = g_new0(GValue, 1);
- EXPECT_EQ(service_type_value,
- g_value_init(service_type_value, G_TYPE_STRING));
- g_value_set_static_string(service_type_value, service_type);
-
+ // Initialize return value for D-Bus call to Service object, which is a
+ // hash table of static strings (char*) in GValue*.
+ GHashTable* service_hash_table = g_hash_table_new_full(
+ g_str_hash, g_str_equal,
+ nullptr, // no key_destroy_func because keys are static.
+ GValueFree); // value_destroy_func
+ GValue* service_type_value = GValueNewString(service_type);
g_hash_table_insert(service_hash_table,
const_cast<char*>("Type"),
service_type_value);
if (physical_technology != NULL) {
- GValue* physical_technology_value = g_new0(GValue, 1);
- EXPECT_EQ(physical_technology_value,
- g_value_init(physical_technology_value, G_TYPE_STRING));
- g_value_set_static_string(physical_technology_value, physical_technology);
-
+ GValue* physical_technology_value = GValueNewString(physical_technology);
g_hash_table_insert(service_hash_table,
const_cast<char*>("PhysicalTechnology"),
physical_technology_value);
}
if (service_tethering != NULL) {
- GValue* service_tethering_value = g_new0(GValue, 1);
- EXPECT_EQ(service_tethering_value,
- g_value_init(service_tethering_value, G_TYPE_STRING));
- g_value_set_static_string(service_tethering_value, service_tethering);
-
+ GValue* service_tethering_value = GValueNewString(service_tethering);
g_hash_table_insert(service_hash_table,
const_cast<char*>("Tethering"),
service_tethering_value);
@@ -182,6 +184,7 @@
NetworkTethering tethering;
EXPECT_TRUE(cmut_.GetConnectionProperties(&dbus_iface_, &type, &tethering));
EXPECT_EQ(expected_type, type);
+ testing::Mock::VerifyAndClearExpectations(&dbus_iface_);
}
void ConnectionManagerTest::TestWithServiceTethering(
@@ -412,8 +415,7 @@
TEST_F(ConnectionManagerTest, MalformedServiceList) {
SetupMocks("/service/guest-network");
- string service_name(kServicePath_);
- SetManagerReply(&service_name, DBUS_TYPE_G_STRING_ARRAY);
+ SetManagerReply(kServicePath_, DBUS_TYPE_G_STRING_ARRAY);
NetworkConnectionType type;
NetworkTethering tethering;
diff --git a/test_utils.cc b/test_utils.cc
index 723281a..7e03ef1 100644
--- a/test_utils.cc
+++ b/test_utils.cc
@@ -343,7 +343,7 @@
}
GValue* GValueNewString(const char* str) {
- GValue* gval = new GValue();
+ GValue* gval = g_new0(GValue, 1);
g_value_init(gval, G_TYPE_STRING);
g_value_set_string(gval, str);
return gval;
@@ -352,7 +352,7 @@
void GValueFree(gpointer arg) {
auto gval = reinterpret_cast<GValue*>(arg);
g_value_unset(gval);
- delete gval;
+ g_free(gval);
}
} // namespace chromeos_update_engine