More fun with valgrind :-)

Fix a bunch of leaks by calling g_object_unref for HalDevice objects that
    goes away
Fix a bunch of leaks by calling g_object_unref for HalDevice objects that
    goes away
Don't leak parent_path
New function; to free values of the sysfs_to_class_in_devices_map hashtable
    (coldplug_synthesize_events): Fix memory leaks
Fix a bunch of leaks by calling g_object_unref for HalDevice objects that
    goes away
Don't leak old value
Fixup error handling (device_property_atomic_update_end): Fix memory leak
Add HALD_MEMLEAK_DBG but uncomment it by default
New function, defined only if HALD_MEMLEAK_DBG is set; should prolly be
    invoked by handler registered with atexit(3); some day in the future
    (osspec_probe_done): Add appropriate timeout if HALD_MEMLEAK_DBG is et
Recognize the HALD_MEMLEAK_DBG define and maintain
    dbg_hal_device_object_delta accordingly. (hal_device_set_udi): Don't
    leak old udi
New file - useful for finding memory leaks

12 files changed, 162 insertions, 30 deletions

diff --git a/ChangeLog b/ChangeLog
index 9acd44be..64f033a3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,47 @@
 2005-02-27  David Zeuthen  <davidz@redhat.com>
 
+	More fun with valgrind :-)
+
+	* hald/linux2/classdev.c: Fix a bunch of leaks by calling
+	g_object_unref for HalDevice objects that goes away
+
+	* hald/linux2/physdev.c: Fix a bunch of leaks by calling
+	g_object_unref for HalDevice objects that goes away
+
+	* hald/linux2/hotplug.c (hotplug_event_begin_sysfs): Don't
+	leak parent_path
+
+	* hald/linux2/coldplug.c (free_hash_sys_to_class_in_dev): New
+	function; to free values of the sysfs_to_class_in_devices_map
+	hashtable
+	(coldplug_synthesize_events): Fix memory leaks
+
+	* hald/linux2/blockdev.c: Fix a bunch of leaks by calling
+	g_object_unref for HalDevice objects that goes away
+
+	* hald/property.c (hal_property_set_string): Don't leak old value
+
+	* hald/hald_dbus.c (sender_has_privileges): Fixup error handling
+	(device_property_atomic_update_end): Fix memory leak
+
+	* hald/hald.h: Add HALD_MEMLEAK_DBG but uncomment it by default
+
+	* hald/hald.c (my_shutdown): New function, defined only if
+	HALD_MEMLEAK_DBG is set; should prolly be invoked by handler
+	registered with atexit(3); some day in the future
+	(osspec_probe_done): Add appropriate timeout if HALD_MEMLEAK_DBG
+	is et
+
+	* hald/device.c (hal_device_finalize, hal_device_new): Recognize
+	the HALD_MEMLEAK_DBG define and maintain dbg_hal_device_object_delta
+	accordingly.
+	(hal_device_set_udi): Don't leak old udi
+
+	* hald/valgrind-hald.sh: New file - useful for finding memory
+	leaks
+
+2005-02-27  David Zeuthen  <davidz@redhat.com>
+
 	Played around with Valgrind on this slow Sunday :-). Before this
 	patch options 'valgrind --show-reachable=yes --leak-check=yes
 	--tool=memcheck ./hald --daemon=no --retain-privileges' - remember
diff --git a/hald/device.c b/hald/device.c
index e17088cc..79195852 100644
--- a/hald/device.c
+++ b/hald/device.c
@@ -48,17 +48,27 @@ enum {
 
 static guint signals[LAST_SIGNAL] = { 0 };
 
+#ifdef HALD_MEMLEAK_DBG
+int dbg_hal_device_object_delta = 0;
+#endif
+
 static void
 hal_device_finalize (GObject *obj)
 {
 	HalDevice *device = HAL_DEVICE (obj);
 
+#ifdef HALD_MEMLEAK_DBG
+	dbg_hal_device_object_delta--;
+	printf ("************* in finalize for udi=%s\n", device->udi);
+#endif
+
 	g_slist_foreach (device->properties, (GFunc) hal_property_free, NULL);
 
 	g_free (device->udi);
 
 	if (parent_class->finalize)
 		parent_class->finalize (obj);
+
 }
 
 static void
@@ -149,6 +159,7 @@ hal_device_get_type (void)
 	return type;
 }
 
+
 HalDevice *
 hal_device_new (void)
 {
@@ -156,6 +167,9 @@ hal_device_new (void)
 
 	device = g_object_new (HAL_TYPE_DEVICE, NULL);
 
+#ifdef HALD_MEMLEAK_DBG
+	dbg_hal_device_object_delta++;
+#endif
 	return device;
 }
 
@@ -398,6 +412,8 @@ hal_device_get_udi (HalDevice *device)
 void
 hal_device_set_udi (HalDevice *device, const char *udi)
 {
+	if (device->udi != NULL)
+		g_free (device->udi);
 	device->udi = g_strdup (udi);
 }
 
diff --git a/hald/hald.c b/hald/hald.c
index 23bb380c..e5d259a3 100644
--- a/hald/hald.c
+++ b/hald/hald.c
@@ -601,10 +601,31 @@ main (int argc, char *argv[])
 	return 0;
 }
 
-gboolean
-resolve_udiprop_path (const char *path, const char *source_udi,
-		      char *udi_result, size_t udi_result_size, 
-		      char *prop_result, size_t prop_result_size);
+#ifdef HALD_MEMLEAK_DBG
+extern int dbg_hal_device_object_delta;
+
+/* useful for valgrinding; see below */
+static gboolean
+my_shutdown (gpointer data)
+{
+	HalDeviceStore *gdl;
+	
+	printf ("Num devices in TDL: %d\n", g_slist_length ((hald_get_tdl ())->devices));
+	printf ("Num devices in GDL: %d\n", g_slist_length ((hald_get_gdl ())->devices));
+	
+	gdl = hald_get_gdl ();
+next:
+	if (g_slist_length (gdl->devices) > 0) {
+		HalDevice *d = HAL_DEVICE(gdl->devices->data);
+		hal_device_store_remove (gdl, d);
+		g_object_unref (d);
+		goto next;
+	}
+	
+	printf ("hal_device_object_delta = %d (should be zero)\n", dbg_hal_device_object_delta);
+	exit (1);
+}
+#endif
 
 void 
 osspec_probe_done (void)
@@ -620,6 +641,11 @@ osspec_probe_done (void)
 
 	hald_is_initialising = FALSE;
 
+#ifdef HALD_MEMLEAK_DBG
+	g_timeout_add ((HALD_MEMLEAK_DBG) * 1000,
+		       my_shutdown,
+		       NULL);
+#endif
 }
 
 
diff --git a/hald/hald.h b/hald/hald.h
index eef5409f..0f3a4143 100644
--- a/hald/hald.h
+++ b/hald/hald.h
@@ -48,6 +48,15 @@ extern dbus_bool_t hald_is_verbose;
 extern dbus_bool_t hald_is_initialising;
 extern dbus_bool_t hald_is_shutting_down;
 
+/* If this is defined, the amount of time, in seconds, before hald
+ * does an exit where resources are freed - useful for valgrinding
+ * and finding memory leaks; e.g. plug in a device, do something
+ * with the hal daemon and then look at the report
+ *
+ * Use hald/valgrind-hald.sh for this
+ */
+/*#define HALD_MEMLEAK_DBG 60*/
+
 /**
  *  @}
  */
diff --git a/hald/hald_dbus.c b/hald/hald_dbus.c
index a1514792..17ecbc6d 100644
--- a/hald/hald_dbus.c
+++ b/hald/hald_dbus.c
@@ -1110,29 +1110,34 @@ sender_has_privileges (DBusConnection *connection, DBusMessage *message)
 	DBusError error;
 	unsigned long user_uid;
 	const char *user_base_svc;
+	dbus_bool_t ret;
+
+	ret = FALSE;
 
 	user_base_svc = dbus_message_get_sender (message);
 	if (user_base_svc == NULL) {
 		HAL_WARNING (("Cannot determine base service of caller"));
-		return FALSE;
+		goto out;
 	}
 
 	HAL_DEBUG (("base_svc = %s", user_base_svc));
 
 	dbus_error_init (&error);
-	if ((user_uid = dbus_bus_get_unix_user (connection, user_base_svc, &error)) 
-       == (unsigned long) -1) {
+	if ((user_uid = dbus_bus_get_unix_user (connection, user_base_svc, &error)) == (unsigned long) -1) {
 		HAL_WARNING (("Could not get uid for connection"));
-		return FALSE;
+		goto out;
 	}
 
 	HAL_INFO (("uid for caller is %ld", user_uid));
 
 	if (user_uid != 0 && user_uid != geteuid()) {
 		HAL_WARNING (("uid %d is doesn't have the right priviledges", user_uid));
-		return FALSE;
+		goto out;
 	}
 
+	ret = TRUE;
+
+out:
 	return TRUE;
 }
 
@@ -1829,8 +1834,7 @@ device_property_atomic_update_end (void)
 	--atomic_count;
 
 	if (atomic_count < 0) {
-		HAL_WARNING (("*** atomic_count = %d < 0 !!",
-			      atomic_count));
+		HAL_WARNING (("*** atomic_count = %d < 0 !!", atomic_count));
 		atomic_count = 0;
 	}
 
@@ -1845,22 +1849,21 @@ device_property_atomic_update_end (void)
 
 			pu_iter_next = pu_iter->next;
 
+			/* see if we've already processed this */
 			if (pu_iter->udi == NULL)
-				goto have_processed;
+				goto already_processed;
 
 			/* count number of updates for this device */
 			num_updates_this = 0;
-			for (pu_iter2 = pu_iter;
-			     pu_iter2 != NULL; pu_iter2 = pu_iter2->next) {
+			for (pu_iter2 = pu_iter; pu_iter2 != NULL; pu_iter2 = pu_iter2->next) {
 				if (strcmp (pu_iter2->udi, pu_iter->udi) == 0)
 					num_updates_this++;
 			}
 
 			/* prepare message */
-			message = dbus_message_new_signal (
-				pu_iter->udi,
-				"org.freedesktop.Hal.Device",
-				"PropertyModified");
+			message = dbus_message_new_signal (pu_iter->udi,
+							   "org.freedesktop.Hal.Device",
+							   "PropertyModified");
 			dbus_message_iter_init_append (message, &iter);
 			dbus_message_iter_append_basic (&iter, DBUS_TYPE_INT32,
 							&num_updates_this);
@@ -1898,6 +1901,7 @@ device_property_atomic_update_end (void)
 					dbus_message_iter_close_container (&iter_array, &iter_struct);
 
 					/* signal this is already processed */
+					g_free (pu_iter2->key);
 					if (pu_iter2 != pu_iter) {
 						g_free (pu_iter2->udi);
 						pu_iter2->udi = NULL;
@@ -1905,19 +1909,18 @@ device_property_atomic_update_end (void)
 				}
 			}
 
+			g_free (pu_iter->udi);
 			dbus_message_iter_close_container (&iter, &iter_array);
 
-
 			if (!dbus_connection_send
 			    (dbus_connection, message, NULL))
 				DIE (("error broadcasting message"));
-
 			dbus_message_unref (message);
 
-		      have_processed:
-			g_free (pu_iter->key);
+		already_processed:
 			g_free (pu_iter);
-		}		/* for all updates */
+
+		} /* for all updates */
 
 		num_pending_updates = 0;
 		pending_updates_head = NULL;
diff --git a/hald/linux2/blockdev.c b/hald/linux2/blockdev.c
index c40488b8..6bac6727 100644
--- a/hald/linux2/blockdev.c
+++ b/hald/linux2/blockdev.c
@@ -148,6 +148,7 @@ blockdev_callouts_remove_done (HalDevice *d, gpointer userdata1, gpointer userda
 	if (!hal_device_store_remove (hald_get_gdl (), d)) {
 		HAL_WARNING (("Error removing device"));
 	}
+	g_object_unref (d);
 
 	hotplug_event_end (end_token);
 }
@@ -272,12 +273,14 @@ add_blockdev_probing_helper_done (HalDevice *d, gboolean timed_out, gint return_
 	 */
 	if (timed_out || !(return_code == 0 || (!is_volume && return_code == 2))) {
 		hal_device_store_remove (hald_get_tdl (), d);
+		g_object_unref (d);
 		hotplug_event_end (end_token);
 		goto out;
 	}
 
 	if (!blockdev_compute_udi (d)) {
 		hal_device_store_remove (hald_get_tdl (), d);
+		g_object_unref (d);
 		hotplug_event_end (end_token);
 		goto out;
 	}
@@ -350,6 +353,7 @@ blockdev_callouts_preprobing_storage_done (HalDevice *d, gpointer userdata1, gpo
 						    NULL, add_blockdev_probing_helper_done, 
 						    HAL_HELPER_TIMEOUT) == NULL) {
 				hal_device_store_remove (hald_get_tdl (), d);
+				g_object_unref (d);
 				hotplug_event_end (end_token);
 			}
 			goto out;
@@ -377,6 +381,7 @@ blockdev_callouts_preprobing_storage_done (HalDevice *d, gpointer userdata1, gpo
 				    NULL, add_blockdev_probing_helper_done, 
 				    HAL_HELPER_TIMEOUT) == NULL) {
 		hal_device_store_remove (hald_get_tdl (), d);
+		g_object_unref (d);
 		hotplug_event_end (end_token);
 	}
 
@@ -413,6 +418,7 @@ blockdev_callouts_preprobing_volume_done (HalDevice *d, gpointer userdata1, gpoi
 				    NULL, add_blockdev_probing_helper_done, 
 				    HAL_HELPER_TIMEOUT) == NULL) {
 		hal_device_store_remove (hald_get_tdl (), d);
+		g_object_unref (d);
 		hotplug_event_end (end_token);
 	}
 
@@ -473,7 +479,7 @@ hotplug_event_begin_add_blockdev (const gchar *sysfs_path, const gchar *device_f
 		if (!is_fakevolume && hal_device_property_get_bool (parent, "storage.no_partitions_hint")) {
 			HAL_INFO (("Ignoring blockdev since not a fakevolume and parent has "
 				   "storage.no_partitions_hint==TRUE"));
-			goto out;
+			goto error;
 		}
 	}
 
diff --git a/hald/linux2/classdev.c b/hald/linux2/classdev.c
index c34f3b94..b8be83ef 100644
--- a/hald/linux2/classdev.c
+++ b/hald/linux2/classdev.c
@@ -285,6 +285,7 @@ net_add (const gchar *sysfs_path, const gchar *device_file, HalDevice *physdev,
 error:
 	if (d != NULL) {
 		hal_device_store_remove (hald_get_tdl (), d);
+		g_object_unref (d);
 		d = NULL;
 	}
 
@@ -548,6 +549,8 @@ classdev_callouts_remove_done (HalDevice *d, gpointer userdata1, gpointer userda
 		HAL_WARNING (("Error removing device"));
 	}
 
+	g_object_unref (d);
+
 	hotplug_event_end (end_token);
 }
 
@@ -561,6 +564,7 @@ add_classdev_after_probing (HalDevice *d, ClassDevHandler *handler, void *end_to
 	/* Compute UDI */
 	if (!handler->compute_udi (d)) {
 		hal_device_store_remove (hald_get_tdl (), d);
+		g_object_unref (d);
 		hotplug_event_end (end_token);
 		goto out;
 	}
@@ -586,6 +590,7 @@ add_classdev_probing_helper_done (HalDevice *d, gboolean timed_out, gint return_
 	/* Discard device if probing reports failure */
 	if (return_code != 0) {
 		hal_device_store_remove (hald_get_tdl (), d);
+		g_object_unref (d);
 		hotplug_event_end (end_token);
 		goto out;
 	}
@@ -640,6 +645,7 @@ classdev_callouts_preprobing_done (HalDevice *d, gpointer userdata1, gpointer us
 					    (gpointer) handler, add_classdev_probing_helper_done, 
 					    HAL_HELPER_TIMEOUT) == NULL) {
 			hal_device_store_remove (hald_get_tdl (), d);
+			g_object_unref (d);
 			hotplug_event_end (end_token);
 		}
 		goto out;
diff --git a/hald/linux2/coldplug.c b/hald/linux2/coldplug.c
index 79ebd9aa..1effe010 100644
--- a/hald/linux2/coldplug.c
+++ b/hald/linux2/coldplug.c
@@ -59,6 +59,17 @@ coldplug_compute_visit_device (const gchar *path,
 /* For debugging */
 /*#define HAL_COLDPLUG_VERBOSE*/
 
+static void
+free_hash_sys_to_class_in_dev (gpointer key, gpointer value, gpointer user_data)
+{
+	GSList *i;
+	GSList *list = (GSList *) value;
+
+	for (i = list; i != NULL; i = g_slist_next (i))
+		g_free (i->data);
+	g_slist_free (list);
+}
+
 /** This function serves one major purpose : build an ordered list of
  *  pairs (sysfs path, subsystem) to process when starting up:
  *  coldplugging. The ordering is arranged such that all bus-devices
@@ -180,8 +191,8 @@ coldplug_synthesize_events (void)
 	}
 	g_dir_close (dir);
 
-	/* build class map and class device map */
-	sysfs_to_class_in_devices_map = g_hash_table_new (g_str_hash, g_str_equal);
+	/* build class map and class device map (values are free in separate foreach()) */
+	sysfs_to_class_in_devices_map = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);
 	g_snprintf (path, HAL_PATH_MAX, "%s/class" , get_hal_sysfs_path ());
 	if ((dir = g_dir_open (path, 0, &err)) == NULL) {
 		HAL_ERROR (("Unable to open %/class: %s", get_hal_sysfs_path (), err->message));
@@ -211,10 +222,8 @@ coldplug_synthesize_events (void)
 				GSList *classdev_strings;
 
 				g_snprintf (path2, HAL_PATH_MAX, "%s/class/%s/%s", get_hal_sysfs_path (), f, f1);
-				if (target) {
-					normalized_target = hal_util_get_normalized_path (path2, target);
-					g_free (target);
-				}
+				normalized_target = hal_util_get_normalized_path (path2, target);
+				g_free (target);
 
 				classdev_strings = g_hash_table_lookup (sysfs_to_class_in_devices_map,
 									normalized_target);
@@ -257,6 +266,8 @@ coldplug_synthesize_events (void)
 	g_dir_close (dir);
 
 	g_hash_table_destroy (sysfs_to_bus_map);
+	/* free keys and values in this complex hash */
+	g_hash_table_foreach (sysfs_to_class_in_devices_map, free_hash_sys_to_class_in_dev, NULL);
 	g_hash_table_destroy (sysfs_to_class_in_devices_map);
 
 	/* we are guaranteed, per construction, that the len of this list is even */
diff --git a/hald/linux2/hotplug.c b/hald/linux2/hotplug.c
index 64bc38f8..47dc03e2 100644
--- a/hald/linux2/hotplug.c
+++ b/hald/linux2/hotplug.c
@@ -259,6 +259,8 @@ hotplug_event_begin_sysfs (HotplugEvent *hotplug_event)
 							     is_partition,
 							     (void *) hotplug_event);
 		}
+
+		g_free (parent_path);
 	} else {
 		/* just ignore this hotplug event */
 		hotplug_event_end ((void *) hotplug_event);
diff --git a/hald/linux2/physdev.c b/hald/linux2/physdev.c
index 534a98c2..d7fd01ef 100644
--- a/hald/linux2/physdev.c
+++ b/hald/linux2/physdev.c
@@ -852,6 +852,7 @@ physdev_callouts_remove_done (HalDevice *d, gpointer userdata1, gpointer userdat
 	if (!hal_device_store_remove (hald_get_gdl (), d)) {
 		HAL_WARNING (("Error removing device"));
 	}
+	g_object_unref (d);
 
 	hotplug_event_end (end_token);
 }
@@ -890,6 +891,7 @@ physdev_callouts_preprobing_done (HalDevice *d, gpointer userdata1, gpointer use
 	/* Compute UDI */
 	if (!handler->compute_udi (d)) {
 		hal_device_store_remove (hald_get_tdl (), d);
+		g_object_unref (d);
 		hotplug_event_end (end_token);
 		goto out;
 	}
diff --git a/hald/property.c b/hald/property.c
index a075c199..629a0e7f 100644
--- a/hald/property.c
+++ b/hald/property.c
@@ -273,6 +273,8 @@ hal_property_set_string (HalProperty *prop, const char *value)
 			  prop->type == HAL_PROPERTY_TYPE_INVALID);
 
 	prop->type = HAL_PROPERTY_TYPE_STRING;
+	if (prop->str_value != NULL)
+		g_free (prop->str_value);
 	prop->str_value = g_strdup (value);
 
 	while (!g_utf8_validate (prop->str_value, -1,
diff --git a/hald/valgrind-hald.sh b/hald/valgrind-hald.sh
new file mode 100755
index 00000000..4420b784
--- /dev/null
+++ b/hald/valgrind-hald.sh
@@ -0,0 +1,7 @@
+#!/bin/sh
+
+export PATH=linux2:linux2/probing:linux2/addons:.:../tools:../tools/linux:$PATH
+export HAL_FDI_SOURCE_PREPROBE=../fdi/preprobe
+export HAL_FDI_SOURCE_INFORMATION=../fdi/information
+export HAL_FDI_SOURCE_POLICY=../fdi/policy
+valgrind --num-callers=20 --show-reachable=yes --leak-check=yes --tool=memcheck ./hald --daemon=no --verbose=yes --retain-privileges