race/deadlock/cleanup fix in gnome-vfs-method.c

[jacob berkman <jacob ximian com>]

there was a possible race condition that i mentioned yesterday, and
after actually trying that patch out i found that it would deadlock
loading the applications: method (since that recursively tries to load
the file: method).

this patch works around that, and also cleans up / simplifies most of
the locking in that file, such that there are now only 2 locks/unlocks.

i have actually tried this patch and i haven't had any problems.

for some reason i am worried about locking in the rest of gnome-vfs...

 - jacob

Index: gnome-vfs-method.c
===================================================================
RCS file: /cvs/gnome/gnome-vfs/libgnomevfs/gnome-vfs-method.c,v
retrieving revision 1.22
diff -u -r1.22 gnome-vfs-method.c
--- gnome-vfs-method.c	4 Sep 2001 22:22:58 -0000	1.22
+++ gnome-vfs-method.c	9 Jul 2002 18:12:46 -0000
@@ -44,21 +44,17 @@
 typedef struct _ModuleElement ModuleElement;
 
 static gboolean method_already_initialized = FALSE;
-G_LOCK_DEFINE_STATIC (method_already_initialized);
 
 static GHashTable *module_hash = NULL;
-G_LOCK_DEFINE_STATIC (module_hash);
+G_LOCK_DEFINE_STATIC (gnome_vfs_method);
 
 static GList *module_path_list = NULL;
-G_LOCK_DEFINE_STATIC (module_path_list);
 
 
 static gboolean
 init_hash_table (void)
 {
-	G_LOCK (module_hash);
 	module_hash = g_hash_table_new (g_str_hash, g_str_equal);
-	G_UNLOCK (module_hash);
 
 	return TRUE;
 }
@@ -101,25 +97,16 @@
 init_path_list (void)
 {
 	const gchar *user_path_list;
-	gboolean retval;
 
-	retval = TRUE;
-
-	G_LOCK (module_path_list);
-
-	if (module_path_list != NULL) {
-		retval = TRUE;
-		goto end;
-	}
+	if (module_path_list != NULL)
+		return TRUE;
 
 	/* User-supplied path.  */
 
 	user_path_list = getenv ("GNOME_VFS_MODULE_PATH");
 	if (user_path_list != NULL) {
-		if (! install_path_list (user_path_list)) {
-			retval = FALSE;
-			goto end;
-		}
+		if (! install_path_list (user_path_list))
+			return FALSE;
 	}
 
 	/* Default path.  It comes last so that users can override it.  */
@@ -127,30 +114,28 @@
 	module_path_list = g_list_append (module_path_list,
 					  g_strdup (GNOME_VFS_MODULE_DIR));
 
- end:
-	G_UNLOCK (module_path_list);
-	return retval;
+	return TRUE;
 }
 
 gboolean
 gnome_vfs_method_init (void)
 {
-	G_LOCK (method_already_initialized);
+	G_LOCK (gnome_vfs_method);
 
-	if (method_already_initialized) {
-		G_UNLOCK (method_already_initialized);
-		return TRUE;
-	}
+	if (method_already_initialized)
+		goto gnome_vfs_method_init_out;
 
 	if (! init_hash_table ())
-		return FALSE;
+		goto gnome_vfs_method_init_out;
 	if (! init_path_list ())
-		return FALSE;
+		goto gnome_vfs_method_init_out;
 
 	method_already_initialized = TRUE;
-	G_UNLOCK (method_already_initialized);
 
-	return TRUE;
+ gnome_vfs_method_init_out:
+	G_UNLOCK (gnome_vfs_method);
+
+	return method_already_initialized;
 }
 
 static void
@@ -270,7 +255,7 @@
 	}
 }
 
-static gboolean
+static ModuleElement *
 gnome_vfs_add_module_to_hash_table (const gchar *name)
 {
 	GnomeVFSMethod *method = NULL;
@@ -281,16 +266,40 @@
 	gid_t saved_gid;
 	const char *args;
 
-	G_LOCK (module_hash);
+	/*
+	 * it's ok for this function to recurse in one thread -
+	 * eg. the applications: method's init function tries to load
+	 * the file: method.
+	 *
+	 * however, we don't want multiple threads in here mucking
+	 * with the hash table at the same time, so we do use the
+	 * mutex.
+	 *
+	 * we do not guard against cyclic method dependencies, ie
+	 * method foo: trying to load bar: as it's easy for methods to
+	 * Just Not Do That
+	 *
+	 * we use a GPrivate as a per-thread lock on the shared mutex
+	 */
+	static GStaticPrivate priv_key = G_STATIC_PRIVATE_INIT;
+	gpointer was_locked;
+
+	was_locked = g_static_private_get (&priv_key);
+	if (!was_locked) {
+		g_static_private_set (&priv_key, GINT_TO_POINTER (1), NULL);
+		G_LOCK (gnome_vfs_method);
+	}
+
 	module_element = g_hash_table_lookup (module_hash, name);
-	G_UNLOCK (module_hash);
 
 	if (module_element != NULL)
-		return TRUE;
+		goto add_module_out;
 
 	module_name = gnome_vfs_configuration_get_module_path (name, &args);
-	if (module_name == NULL)
-		return FALSE;
+	if (module_name == NULL) {
+		module_element = NULL;
+		goto add_module_out;
+	}
 
 	/* Set the effective UID/GID to the user UID/GID to prevent attacks to
            setuid/setgid executables.  */
@@ -308,19 +317,25 @@
 	seteuid (saved_uid);
 	setegid (saved_gid);
 
-	if (method == NULL && transform == NULL)
-		return FALSE;
+	if (method == NULL && transform == NULL) {
+		module_element = NULL;
+		goto add_module_out;
+	}
 
 	module_element = g_new (ModuleElement, 1);
 	module_element->name = g_strdup (name);
 	module_element->method = method;
 	module_element->transform = transform;
 
-	G_LOCK (module_hash);
 	g_hash_table_insert (module_hash, module_element->name, module_element);
-	G_UNLOCK (module_hash);
 
-	return TRUE;
+ add_module_out:
+	if (!was_locked) {
+		g_static_private_set (&priv_key, NULL, NULL);
+		G_UNLOCK (gnome_vfs_method);
+	}
+
+	return module_element;
 }
 
 GnomeVFSMethod *
@@ -330,23 +345,8 @@
 
 	g_return_val_if_fail (name != NULL, NULL);
 
-	G_LOCK (module_hash);
-	module_element = g_hash_table_lookup (module_hash, name);
-	G_UNLOCK (module_hash);
-
-	if (module_element != NULL)
-		return module_element->method;
-
-	if (gnome_vfs_add_module_to_hash_table (name)) {
-		G_LOCK (module_hash);
-		module_element = g_hash_table_lookup (module_hash, name);
-		G_UNLOCK (module_hash);
-		
-		if (module_element != NULL)
-			return module_element->method;
-	}
-
-	return NULL;
+	module_element = gnome_vfs_add_module_to_hash_table (name);
+	return module_element ? module_element->method : NULL;
 }
 
 GnomeVFSTransform *
@@ -356,21 +356,6 @@
 
 	g_return_val_if_fail (name != NULL, NULL);
 
-	G_LOCK (module_hash);
-	module_element = g_hash_table_lookup (module_hash, name);
-	G_UNLOCK (module_hash);
-
-	if (module_element != NULL)
-		return module_element->transform;
-
-	if (gnome_vfs_add_module_to_hash_table (name)) {
-		G_LOCK (module_hash);
-		module_element = g_hash_table_lookup (module_hash, name);
-		G_UNLOCK (module_hash);
-		
-		if (module_element != NULL)
-			return module_element->transform;
-	}
-
-	return NULL;
+	module_element = gnome_vfs_add_module_to_hash_table (name);
+	return module_element ? module_element->transform : NULL;
 }