From 9b7c8db16a0f8e4f87c793412fd519d944a7ee7e Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 18 Feb 2015 12:41:49 +0100 Subject: [PATCH 01/10] settings-connection: memleak: free filename on dispose ==4203== 97 (+97) bytes in 2 (+2) blocks are definitely lost in loss record 4,586 of 5,632 ==4203== at 0x4C29BCF: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==4203== by 0x7F4A6F5: g_malloc (gmem.c:97) ==4203== by 0x7F6301E: g_strdup (gstrfuncs.c:356) ==4203== by 0x47E4C8: nm_settings_connection_set_filename (nm-settings-connection.c:2228) ==4203== by 0x7CBF6EC: object_set_property (gobject.c:1415) ==4203== by 0x7CBF6EC: g_object_new_internal (gobject.c:1828) ==4203== by 0x7CC1194: g_object_new_valist (gobject.c:2034) ==4203== by 0x7CC14D0: g_object_new (gobject.c:1617) ==4203== by 0x12A08193: nm_ifcfg_connection_new (nm-ifcfg-connection.c:229) ==4203== by 0x12A0542B: update_connection (plugin.c:225) ==4203== by 0x12A0696A: add_connection (plugin.c:715) ==4203== by 0x4814BB: nm_settings_add_connection (nm-settings.c:1030) ==4203== by 0x4817DE: pk_add_cb (nm-settings.c:1136) --- src/settings/nm-settings-connection.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index ef993c39c..9285dfbfa 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -2314,6 +2314,8 @@ dispose (GObject *object) } g_clear_object (&priv->agent_mgr); + g_clear_pointer (&priv->filename, g_free); + G_OBJECT_CLASS (nm_settings_connection_parent_class)->dispose (object); } From 76430f9ccad641d162c0ec79133bac56c55fe755 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 18 Feb 2015 13:11:34 +0100 Subject: [PATCH 02/10] dhcp-client: memleak: free hostname on dispose ==7745== 11 (+11) bytes in 1 (+1) blocks are definitely lost in loss record 408 of 5,735 ==7745== at 0x4C29BCF: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==7745== by 0x7F4A6F5: g_malloc (gmem.c:97) ==7745== by 0x7F6301E: g_strdup (gstrfuncs.c:356) ==7745== by 0x45C188: nm_dhcp_client_start_ip4 (nm-dhcp-client.c:417) ==7745== by 0x460147: client_start (nm-dhcp-manager.c:268) ==7745== by 0x460393: nm_dhcp_manager_start_ip4 (nm-dhcp-manager.c:308) ==7745== by 0x44EB16: dhcp4_start (nm-device.c:3168) ==7745== by 0x44EE15: act_stage3_ip4_config_start (nm-device.c:3440) ==7745== by 0x455C9F: nm_device_activate_stage3_ip4_start (nm-device.c:4657) ==7745== by 0x456467: nm_device_activate_stage3_ip_config_start (nm-device.c:4801) ==7745== by 0x7F44AEA: g_main_dispatch (gmain.c:3111) ==7745== by 0x7F44AEA: g_main_context_dispatch (gmain.c:3710) ==7745== by 0x7F44E87: g_main_context_iterate.isra.29 (gmain.c:3781) --- src/dhcp-manager/nm-dhcp-client.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/dhcp-manager/nm-dhcp-client.c b/src/dhcp-manager/nm-dhcp-client.c index 951674953..d8c77878f 100644 --- a/src/dhcp-manager/nm-dhcp-client.c +++ b/src/dhcp-manager/nm-dhcp-client.c @@ -884,6 +884,7 @@ dispose (GObject *object) timeout_cleanup (self); g_clear_pointer (&priv->iface, g_free); + g_clear_pointer (&priv->hostname, g_free); if (priv->hwaddr) { g_byte_array_free (priv->hwaddr, TRUE); From c26ef29a47546e14fc5ef7f56470615ee488ce50 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 18 Feb 2015 13:12:39 +0100 Subject: [PATCH 03/10] dhcp-client: memleak: free uuid on dispose ==7745== 37 (+37) bytes in 1 (+1) blocks are definitely lost in loss record 2,679 of 5,735 ==7745== at 0x4C29BCF: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==7745== by 0x7F4A6F5: g_malloc (gmem.c:97) ==7745== by 0x7F6301E: g_strdup (gstrfuncs.c:356) ==7745== by 0x45B097: set_property (nm-dhcp-client.c:851) ==7745== by 0x7CBF688: object_set_property (gobject.c:1415) ==7745== by 0x7CBF688: g_object_new_internal (gobject.c:1808) ==7745== by 0x7CC1194: g_object_new_valist (gobject.c:2034) ==7745== by 0x7CC14D0: g_object_new (gobject.c:1617) ==7745== by 0x45FF9F: client_start (nm-dhcp-manager.c:253) ==7745== by 0x460393: nm_dhcp_manager_start_ip4 (nm-dhcp-manager.c:308) ==7745== by 0x44EB16: dhcp4_start (nm-device.c:3168) ==7745== by 0x44EE15: act_stage3_ip4_config_start (nm-device.c:3440) ==7745== by 0x455C9F: nm_device_activate_stage3_ip4_start (nm-device.c:4657) --- src/dhcp-manager/nm-dhcp-client.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/dhcp-manager/nm-dhcp-client.c b/src/dhcp-manager/nm-dhcp-client.c index d8c77878f..a39ffcca0 100644 --- a/src/dhcp-manager/nm-dhcp-client.c +++ b/src/dhcp-manager/nm-dhcp-client.c @@ -885,6 +885,7 @@ dispose (GObject *object) g_clear_pointer (&priv->iface, g_free); g_clear_pointer (&priv->hostname, g_free); + g_clear_pointer (&priv->uuid, g_free); if (priv->hwaddr) { g_byte_array_free (priv->hwaddr, TRUE); From 5d9f9febfb9895d9cd5b59ad1ee79b027fc6dcaa Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 18 Feb 2015 13:20:59 +0100 Subject: [PATCH 04/10] ndp: memleak: unregister router advertisement handler on dispose ndp_close() does not do that -- it only closes the socket. It's safe to call even if we didn't start solicitation as it has a NULL-check. ==7745== 80 (+80) bytes in 2 (+2) blocks are definitely lost in loss record 3,983 of 5,735 ==7745== at 0x4C29BCF: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==7745== by 0x6F57A2D: ndp_msgrcv_handler_register (libndp.c:1697) ==7745== by 0x47572E: start (nm-lndp-rdisc.c:691) ==7745== by 0x44A457: addrconf6_start_with_link_ready (nm-device.c:4280) ==7745== by 0x44C1E7: linklocal6_complete (nm-device.c:3931) ==7745== by 0x44C1E7: update_ip_config (nm-device.c:6667) ==7745== by 0x44C2F8: queued_ip_config_change (nm-device.c:6688) ==7745== by 0x7F44AEA: g_main_dispatch (gmain.c:3111) ==7745== by 0x7F44AEA: g_main_context_dispatch (gmain.c:3710) ==7745== by 0x7F44E87: g_main_context_iterate.isra.29 (gmain.c:3781) ==7745== by 0x7F451B1: g_main_loop_run (gmain.c:3975) ==7745== by 0x432F74: main (main.c:460) --- src/rdisc/nm-lndp-rdisc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/rdisc/nm-lndp-rdisc.c b/src/rdisc/nm-lndp-rdisc.c index ff1bd33de..922cb3dd7 100644 --- a/src/rdisc/nm-lndp-rdisc.c +++ b/src/rdisc/nm-lndp-rdisc.c @@ -720,6 +720,7 @@ dispose (GObject *object) g_clear_pointer (&priv->event_channel, g_io_channel_unref); if (priv->ndp) { + ndp_msgrcv_handler_unregister (priv->ndp, receive_ra, NDP_MSG_RA, NM_RDISC (rdisc)->ifindex, rdisc); ndp_close (priv->ndp); priv->ndp = NULL; } From f93f0e0b15830e4e265ad7b6877407e64f0185f2 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 18 Feb 2015 16:04:43 +0100 Subject: [PATCH 05/10] linux-platform: dont use-after-free the driver string ==1345== Invalid read of size 1 ==1345== at 0x827DC15: vfprintf (vfprintf.c:1642) ==1345== by 0x8345D04: __vasprintf_chk (vasprintf_chk.c:66) ==1345== by 0x7F882DB: vasprintf (stdio2.h:210) ==1345== by 0x7F882DB: g_vasprintf (gprintf.c:316) ==1345== by 0x7F6319C: g_strdup_vprintf (gstrfuncs.c:507) ==1345== by 0x7F63258: g_strdup_printf (gstrfuncs.c:533) ==1345== by 0x472833: nm_platform_link_to_string (nm-platform.c:2337) ==1345== by 0x472A05: log_link (nm-platform.c:2754) ==1345== by 0x9DC5D5F: ffi_call_unix64 (unix64.S:76) ==1345== by 0x9DC57D0: ffi_call (ffi64.c:525) ==1345== by 0x7CBA553: g_cclosure_marshal_generic (gclosure.c:1448) ==1345== by 0x7CB9D34: g_closure_invoke (gclosure.c:768) ==1345== by 0x7CCB34B: signal_emit_unlocked_R (gsignal.c:3483) ==1345== Address 0xa91b5a0 is 0 bytes inside a block of size 5 free'd ==1345== at 0x4C2ACE9: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==1345== by 0x68E7D6D: link_free_data (link.c:223) ==1345== by 0x6D47B1F: nl_object_free (object.c:186) ==1345== by 0x46C31C: put_nl_object (nm-linux-platform.c:222) ==1345== by 0x46C31C: link_change (nm-linux-platform.c:2354) ==1345== by 0x46C87F: link_set_user_ipv6ll_enabled (nm-linux-platform.c:2583) ==1345== by 0x4476C4: set_nm_ipv6ll (nm-device.c:4418) ==1345== by 0x4476C4: ip6_managed_setup (nm-device.c:7515) ==1345== by 0x453F12: _set_state_full (nm-device.c:7665) ==1345== by 0x4B6609: add_device (nm-manager.c:1885) ==1345== by 0x4B6880: system_create_virtual_device (nm-manager.c:1126) ==1345== by 0x4B6B40: system_create_virtual_devices (nm-manager.c:1163) ==1345== by 0x4B6E00: platform_link_added (nm-manager.c:2213) ==1345== by 0x4B6E00: platform_link_cb (nm-manager.c:2228) ==1345== by 0x9DC5D5F: ffi_call_unix64 (unix64.S:76) --- src/platform/nm-linux-platform.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 091791f4b..e1887e5a4 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -1008,7 +1008,7 @@ init_link (NMPlatform *platform, NMPlatformLink *info, struct rtnl_link *rtnllin if (udev_device) { info->driver = udev_get_driver (platform, udev_device, info->ifindex); if (!info->driver) - info->driver = rtnl_link_get_type (rtnllink); + info->driver = g_intern_string (rtnl_link_get_type (rtnllink)); if (!info->driver) info->driver = ethtool_get_driver (info->name); if (!info->driver) From e5499c558f47733c2753c202a7010ae2ab4c1a6f Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 18 Feb 2015 16:13:57 +0100 Subject: [PATCH 06/10] connectivity: memleak: drop async result reference on complete ==5177== 104 (+104) bytes in 1 (+1) blocks are definitely lost in loss record 5,502 of 6,581 ==5177== at 0x4C29BCF: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==5177== by 0x7F4A6F5: g_malloc (gmem.c:97) ==5177== by 0x7F6159F: g_slice_alloc (gslice.c:1007) ==5177== by 0x7F61B6D: g_slice_alloc0 (gslice.c:1032) ==5177== by 0x7CDB9A3: g_type_create_instance (gtype.c:1847) ==5177== by 0x7CBF356: g_object_new_internal (gobject.c:1774) ==5177== by 0x7CC0D4C: g_object_newv (gobject.c:1922) ==5177== by 0x7CC14E3: g_object_new (gobject.c:1614) ==5177== by 0x79A4C57: g_simple_async_result_new (gsimpleasyncresult.c:319) ==5177== by 0x515B34: nm_connectivity_check_async (nm-connectivity.c:289) ==5177== by 0x515D74: run_check (nm-connectivity.c:217) ==5177== by 0x515DBF: idle_start_periodic_checks (nm-connectivity.c:229) --- src/nm-connectivity.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/nm-connectivity.c b/src/nm-connectivity.c index 804e3935f..34465aad0 100644 --- a/src/nm-connectivity.c +++ b/src/nm-connectivity.c @@ -186,6 +186,7 @@ nm_connectivity_check_cb (SoupSession *session, SoupMessage *msg, gpointer user_ g_simple_async_result_set_op_res_gssize (simple, new_state); g_simple_async_result_complete (simple); + g_object_unref (simple); g_free (cb_data->uri); g_free (cb_data->response); @@ -321,6 +322,7 @@ nm_connectivity_check_async (NMConnectivity *self, g_simple_async_result_set_op_res_gssize (simple, priv->state); g_simple_async_result_complete_in_idle (simple); + g_object_unref (simple); } NMConnectivityState From da5c332151b61eb8037008b4e4dd9150982b8783 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 18 Feb 2015 16:42:39 +0100 Subject: [PATCH 07/10] auth-utils: memleak: free the key when we steal data ==5177== 6 (+6) bytes in 1 (+1) blocks are definitely lost in loss record 118 of 6,581 ==5177== at 0x4C29BCF: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==5177== by 0x7F4A6F5: g_malloc (gmem.c:97) ==5177== by 0x7F6301E: g_strdup (gstrfuncs.c:356) ==5177== by 0x4AD902: nm_auth_chain_set_data (nm-auth-utils.c:194) ==5177== by 0x50919E: impl_agent_manager_register_with_capabilities (nm-agent-manager.c:323) ==5177== by 0x62649BE: invoke_object_method (dbus-gobject.c:1899) ==5177== by 0x62649BE: object_registration_message (dbus-gobject.c:2161) ==5177== by 0x649D5CE: _dbus_object_tree_dispatch_and_unlock (dbus-object-tree.c:1018) ==5177== by 0x648F193: dbus_connection_dispatch (dbus-connection.c:4718) ==5177== by 0x6261DB4: message_queue_dispatch (dbus-gmain.c:90) ==5177== by 0x7F44AEA: g_main_dispatch (gmain.c:3111) ==5177== by 0x7F44AEA: g_main_context_dispatch (gmain.c:3710) ==5177== by 0x7F44E87: g_main_context_iterate.isra.29 (gmain.c:3781) ==5177== by 0x7F451B1: g_main_loop_run (gmain.c:3975) --- src/nm-auth-utils.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index 12924e789..1e14c7bab 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -158,17 +158,18 @@ nm_auth_chain_steal_data (NMAuthChain *self, const char *tag) { ChainData *tmp; gpointer value = NULL; + void *orig_key; g_return_val_if_fail (self != NULL, NULL); g_return_val_if_fail (tag != NULL, NULL); - tmp = g_hash_table_lookup (self->data, tag); - if (tmp) { + if (g_hash_table_lookup_extended (self->data, tag, &orig_key, (gpointer)&tmp)) { g_hash_table_steal (self->data, tag); value = tmp->data; /* Make sure the destroy handler isn't called when freeing */ tmp->destroy = NULL; free_data (tmp); + g_free (orig_key); } return value; } From 7d706daf3ef003e0e7cb5a30296df2e1a1c65ee3 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 18 Feb 2015 17:06:46 +0100 Subject: [PATCH 08/10] agent-manager: memleak: unref the last agent for a request too ==29353== 620 (+620) (32 (+32) direct, 588 (+588) indirect) bytes in 1 (+1) blocks are definitely lost in loss record 6,905 of 7,076 ==29353== at 0x7CDBAC8: g_type_create_instance (gtype.c:1844) ==29353== by 0x7CBF356: g_object_new_internal (gobject.c:1774) ==29353== by 0x7CC0D4C: g_object_newv (gobject.c:1922) ==29353== by 0x7CC14E3: g_object_new (gobject.c:1614) ==29353== by 0x50B58A: nm_secret_agent_new (nm-secret-agent.c:489) ==29353== by 0x50915F: impl_agent_manager_register_with_capabilities (nm-agent-manager.c:309) ==29353== by 0x62649BE: invoke_object_method (dbus-gobject.c:1899) ==29353== by 0x62649BE: object_registration_message (dbus-gobject.c:2161) ==29353== by 0x649D5CE: _dbus_object_tree_dispatch_and_unlock (dbus-object-tree.c:1018) ==29353== by 0x648F193: dbus_connection_dispatch (dbus-connection.c:4718) ==29353== by 0x6261DB4: message_queue_dispatch (dbus-gmain.c:90) ==29353== by 0x7F44AEA: g_main_dispatch (gmain.c:3111) ==29353== by 0x7F44AEA: g_main_context_dispatch (gmain.c:3710) ==29353== by 0x7F44E87: g_main_context_iterate.isra.29 (gmain.c:3781) --- src/settings/nm-agent-manager.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 6228dd5ad..99602d948 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -318,6 +318,7 @@ impl_agent_manager_register_with_capabilities (NMAgentManager *self, priv->chains = g_slist_append (priv->chains, chain); } else { + g_object_unref (agent); error = g_error_new_literal (NM_AGENT_MANAGER_ERROR, NM_AGENT_MANAGER_ERROR_FAILED, "Unable to start agent authentication."); @@ -585,11 +586,12 @@ request_next_agent (Request *req) { GError *error = NULL; + req->current_call_id = NULL; + if (req->current) + g_object_unref (req->current); + if (req->pending) { /* Send the request to the next agent */ - req->current_call_id = NULL; - if (req->current) - g_object_unref (req->current); req->current = req->pending->data; req->pending = g_slist_remove (req->pending, req->current); @@ -599,7 +601,6 @@ request_next_agent (Request *req) req->next_callback (req); } else { - req->current_call_id = NULL; req->current = NULL; /* No more secret agents are available to fulfill this secrets request */ From b1e6c9be91159ee487dadfed1a8c839688594c7b Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 18 Feb 2015 17:34:31 +0100 Subject: [PATCH 09/10] manager: memleak: free the state file name on dispose ==12663== 45 bytes in 1 blocks are definitely lost in loss record 2,464 of 4,708 ==12663== at 0x4C29BCF: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==12663== by 0x7F4A6F5: g_malloc (gmem.c:97) ==12663== by 0x7F6301E: g_strdup (gstrfuncs.c:356) ==12663== by 0x4B8AE5: nm_manager_new (nm-manager.c:4793) ==12663== by 0x432F3D: main (main.c:413) --- src/nm-manager.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/nm-manager.c b/src/nm-manager.c index d16a91288..c6f87cb98 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -5112,6 +5112,7 @@ dispose (GObject *object) } g_clear_object (&priv->settings); + g_free (priv->state_file); g_clear_object (&priv->vpn_manager); /* Unregister property filter */ From be6ce67213066c9348d60894cb59546d74407096 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 18 Feb 2015 18:02:52 +0100 Subject: [PATCH 10/10] build: don't run tests with valgrind by default Only enable it when user requested it. The surpressions might not work for everyone. --- configure.ac | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/configure.ac b/configure.ac index 883a2e297..a6780114a 100644 --- a/configure.ac +++ b/configure.ac @@ -873,13 +873,13 @@ VAPIGEN_CHECK(0.17.1.24) # Tests, utilities and documentation AC_ARG_ENABLE(tests, AS_HELP_STRING([--enable-tests=root|yes|no], [Build NetworkManager tests (default: yes)])) -AC_ARG_WITH(valgrind, AS_HELP_STRING([--with-valgrind=yes|no|path], [Use valgrind to memory-check the tests (default: yes)])) +AC_ARG_WITH(valgrind, AS_HELP_STRING([--with-valgrind=yes|no|path], [Use valgrind to memory-check the tests (default: no)])) # Fallback to --with-tests AC_ARG_WITH(tests, AS_HELP_STRING([--with-tests], [Build NetworkManager tests (deprecated)])) AS_IF([test -n "$with_tests"], enable_tests="$with_tests") -# Default to --enable-tests --with-valgrind +# Default to --enable-tests --with-valgrind=no AS_IF([test -z "$enable_tests"], enable_tests="yes") -AS_IF([test -z "$with_valgrind"], with_valgrind="yes") +AS_IF([test -z "$with_valgrind"], with_valgrind="no") # Normalize values AS_IF([test "$enable_tests" != "yes" -a "$enable_tests" != "root"], enable_tests="no") # Search for tools