[Patch] Some more leaks and memory corruptions

2006-11-07 Thread Tambet Ingo
Hey,

Here's a quite long patch for 0.6 branch. In addition to fixes to simple
leaks which don't need any explanation, I'll try to comment the bigger
changes and why I did them.

NetworkManager.c (nm_data_free):

I added a comment there why it's necessary, but in short, removing all
devices most likely triggers a state change, which gets scheduled, but
never has a chance to run. It wouldn't be a big deal, but the scheduled
signal emitters have references to devices which prevents them getting
cleaned properly. Also, since these signals now actually do run, the
order of clean up needed some changes.

nm-dbus-nmi.c:
vpn-manager/nm-dbus-vpn.c

Remove 'dbus_pending_call_ref (pcall)' calls from dbus callbacks - we
already own them, it just leaks all the pcalls.

NetworkManagerPolicy.c:
nm-device.c

For all the scheduled functions which pass NMActRequest, increment the
reference count when scheduling and release the reference in callback.
This is mostly needed to avoid problems when activation is canceled (or
fails) at unfortunate moments and the NMActRequest gets freed before the
callback has a chance to run.

nm-device-802-11-wireless.c:

There was some unusual reference counting patterns there: in some cases,
when a callback is scheduled (g_source_attach), the reference count of
the source was balanced at the callback - which works fine if the
callback actually has the chance to run. Removing a device frees it's
GMainContext, leaving all scheduled callbacks (and their user_data!)
alive. So to fix that, the reference of the source is given to
GMainContext, so when it exits, it can cleanly free the sources. Also,
instead of freeing callback' user_data in callback, use the
GDestroyNotify of the source to free them - again, so the the
GMainContext can clean things up without calling the callback.

Tambet
? nm-leaks.diff
Index: NetworkManager.c
===
RCS file: /cvs/gnome/NetworkManager/src/NetworkManager.c,v
retrieving revision 1.104.2.7
diff -u -r1.104.2.7 NetworkManager.c
--- NetworkManager.c	3 Nov 2006 11:39:15 -	1.104.2.7
+++ NetworkManager.c	7 Nov 2006 13:14:34 -
@@ -485,8 +485,8 @@
 {
 	g_return_if_fail (dev != NULL);
 
-	nm_device_set_removed (dev, TRUE);
-	nm_device_deactivate (dev);
+	nm_device_set_removed (dev, TRUE);	
+	nm_device_stop (dev);
 	g_object_unref (G_OBJECT (dev));
 }
 
@@ -507,20 +507,28 @@
 	if ((req = nm_vpn_manager_get_vpn_act_request (data-vpn_manager)))
 		nm_vpn_manager_deactivate_vpn_connection (data-vpn_manager, nm_vpn_act_request_get_parent_dev (req));
 
-	if (data-netlink_monitor)
-	{
-		g_object_unref (G_OBJECT (data-netlink_monitor));
-		data-netlink_monitor = NULL;
-	}
-
 	/* Stop and destroy all devices */
 	nm_lock_mutex (data-dev_list_mutex, __FUNCTION__);
 	g_slist_foreach (data-dev_list, (GFunc) device_stop_and_free, NULL);
 	g_slist_free (data-dev_list);
+	data-dev_list = NULL;
 	nm_unlock_mutex (data-dev_list_mutex, __FUNCTION__);
 
+	/* device_stop_and_free() deactivates devices, which triggers state change
+	   signals. Without cleaning the queue, they never get the chance to fire,
+	   keeping the device references alive.
+	*/
+	while (g_main_context_pending (data-main_context))
+		g_main_context_iteration (data-main_context, TRUE);
+
 	g_mutex_free (data-dev_list_mutex);
 
+	if (data-netlink_monitor)
+	{
+		g_object_unref (G_OBJECT (data-netlink_monitor));
+		data-netlink_monitor = NULL;
+	}
+
 	nm_ap_list_unref (data-allowed_ap_list);
 	nm_ap_list_unref (data-invalid_ap_list);
 
@@ -536,7 +544,7 @@
 	nm_dbus_method_list_free (data-net_methods);
 	nm_dbus_method_list_free (data-vpn_methods);
 
-	g_io_channel_unref(data-sigterm_iochannel);
+	g_io_channel_unref (data-sigterm_iochannel);
 
 	nm_hal_deinit (data);
 
Index: NetworkManagerAP.c
===
RCS file: /cvs/gnome/NetworkManager/src/NetworkManagerAP.c,v
retrieving revision 1.53.2.4
diff -u -r1.53.2.4 NetworkManagerAP.c
--- NetworkManagerAP.c	21 Oct 2006 03:41:19 -	1.53.2.4
+++ NetworkManagerAP.c	7 Nov 2006 13:14:34 -
@@ -537,6 +537,7 @@
 
 	/* Free existing list */
 	g_slist_foreach (ap-user_addresses, (GFunc) g_free, NULL);
+	g_slist_free (ap-user_addresses);
 
 	/* Copy new list and set as our own */
 	for (elt = list; elt; elt = g_slist_next (elt))
Index: NetworkManagerDbus.c
===
RCS file: /cvs/gnome/NetworkManager/src/NetworkManagerDbus.c,v
retrieving revision 1.106.2.3
diff -u -r1.106.2.3 NetworkManagerDbus.c
--- NetworkManagerDbus.c	21 May 2006 17:28:02 -	1.106.2.3
+++ NetworkManagerDbus.c	7 Nov 2006 13:14:34 -
@@ -207,9 +207,9 @@
 static gboolean nm_dbus_signal_device_status_change (gpointer user_data)
 {
 	NMStatusChangeData *cb_data = (NMStatusChangeData *)user_data;
-	DBusMessage *		message;
-	char *			dev_path;
-	const char *		sig = NULL;
+	DBusMessage *		message = NULL;
+	char *			dev_path = NULL;

Re: [Patch] Some more leaks and memory corruptions

2006-11-07 Thread Dan Williams
On Tue, 2006-11-07 at 16:02 +0200, Tambet Ingo wrote:
 Hey,
 
 Here's a quite long patch for 0.6 branch. In addition to fixes to simple
 leaks which don't need any explanation, I'll try to comment the bigger
 changes and why I did them.
 
 NetworkManager.c (nm_data_free):
 
 I added a comment there why it's necessary, but in short, removing all
 devices most likely triggers a state change, which gets scheduled, but
 never has a chance to run. It wouldn't be a big deal, but the scheduled
 signal emitters have references to devices which prevents them getting
 cleaned properly. Also, since these signals now actually do run, the
 order of clean up needed some changes.
 
 nm-dbus-nmi.c:
 vpn-manager/nm-dbus-vpn.c
 
 Remove 'dbus_pending_call_ref (pcall)' calls from dbus callbacks - we
 already own them, it just leaks all the pcalls.
 
 NetworkManagerPolicy.c:
 nm-device.c
 
 For all the scheduled functions which pass NMActRequest, increment the
 reference count when scheduling and release the reference in callback.
 This is mostly needed to avoid problems when activation is canceled (or
 fails) at unfortunate moments and the NMActRequest gets freed before the
 callback has a chance to run.
 
 nm-device-802-11-wireless.c:
 
 There was some unusual reference counting patterns there: in some cases,
 when a callback is scheduled (g_source_attach), the reference count of
 the source was balanced at the callback - which works fine if the
 callback actually has the chance to run. Removing a device frees it's
 GMainContext, leaving all scheduled callbacks (and their user_data!)
 alive. So to fix that, the reference of the source is given to
 GMainContext, so when it exits, it can cleanly free the sources. Also,
 instead of freeing callback' user_data in callback, use the
 GDestroyNotify of the source to free them - again, so the the
 GMainContext can clean things up without calling the callback.

Sure; looks good.

Dan

 Tambet
 ___
 NetworkManager-list mailing list
 NetworkManager-list@gnome.org
 http://mail.gnome.org/mailman/listinfo/networkmanager-list

___
NetworkManager-list mailing list
NetworkManager-list@gnome.org
http://mail.gnome.org/mailman/listinfo/networkmanager-list