[evolution-patches] crash at startup, 41448

[Not Zed <notzed ximian com>]

Patch fixes a race at startup.

I'm pretty sure this fix will fix it, the new test program included in
the patch would crash straight away before the code was changed, in the
same places as 41448 and/or 41629, and others.

I've seen it before, but its hard to recreate in the main application. 
And although i knew that race was present, i didn't think it would cause
the crash, but it seemed to afterall.


Index: camel/ChangeLog
===================================================================
RCS file: /cvs/gnome/evolution/camel/ChangeLog,v
retrieving revision 1.1798
diff -u -3 -r1.1798 ChangeLog
--- camel/ChangeLog	19 Apr 2003 03:15:49 -0000	1.1798
+++ camel/ChangeLog	22 Apr 2003 07:51:38 -0000
@@ -1,3 +1,27 @@
+2003-04-22  Not Zed  <NotZed Ximian com>
+
+	** Should fix #41629, #41448, et al.
+	
+	* tests/folder/test10.c: a new torture test for object bag
+	creation/unreffing.
+
+	* camel-url.c (camel_url_copy): new function to copy a url.
+
+	* camel-object.c (camel_object_bag_new): add arguments for key
+	copy and key free functions.  Fixed all callers.
+	(camel_object_bag_destroy): fix a memleak, free the bag key.
+	(camel_object_bag_get, camel_object_bag_reserve)
+	(camel_object_bag_abort, save_bag, save_object): Make the key a
+	void type, rather than char *.
+	(camel_object_bag_add): As above, and also copy the key.
+	(camel_object_bag_remove_unlocked): free the key using
+	bag->free_key.
+
+	* camel-session.c (register_provider)
+	(camel_session_destroy_provider, get_service): Changed to use an
+	object bag instead of a hash table for the service 'cache'.
+	(service_cache_remove): Removed, no longer required.
+
 2003-04-16  Jeffrey Stedfast  <fejj ximian com>
 
 	* camel-url-scanner.c (camel_url_web_end): Urls are unlikely to
Index: camel/camel-data-cache.c
===================================================================
RCS file: /cvs/gnome/evolution/camel/camel-data-cache.c,v
retrieving revision 1.11
diff -u -3 -r1.11 camel-data-cache.c
--- camel/camel-data-cache.c	28 Mar 2003 00:13:42 -0000	1.11
+++ camel/camel-data-cache.c	22 Apr 2003 07:51:39 -0000
@@ -79,7 +79,7 @@
 	struct _CamelDataCachePrivate *p;
 
 	p = cdc->priv = g_malloc0(sizeof(*cdc->priv));
-	p->busy_bag = camel_object_bag_new(g_str_hash, g_str_equal);
+	p->busy_bag = camel_object_bag_new(g_str_hash, g_str_equal, g_strdup, g_free);
 }
 
 static void data_cache_finalise(CamelDataCache *cdc)
Index: camel/camel-object.c
===================================================================
RCS file: /cvs/gnome/evolution/camel/camel-object.c,v
retrieving revision 1.31
diff -u -3 -r1.31 camel-object.c
--- camel/camel-object.c	26 Mar 2003 07:01:38 -0000	1.31
+++ camel/camel-object.c	22 Apr 2003 07:51:42 -0000
@@ -83,6 +83,8 @@
 struct _CamelObjectBag {
 	GHashTable *object_table; /* object by key */
 	GHashTable *key_table;	/* key by object */
+	CamelCopyFunc copy_key;
+	GFreeFunc free_key;
 #ifdef ENABLE_THREADS
 	pthread_t owner;	/* the thread that has reserved the bag for a new entry */
 	sem_t reserve_sem;	/* used to track ownership */
@@ -1082,12 +1084,14 @@
 	object_class_dump_tree_rec(root, 0);
 }
 
-CamelObjectBag *camel_object_bag_new(GHashFunc hash, GEqualFunc equal)
+CamelObjectBag *camel_object_bag_new(GHashFunc hash, GEqualFunc equal, CamelCopyFunc keycopy, GFreeFunc keyfree)
 {
 	CamelObjectBag *bag;
 
 	bag = g_malloc(sizeof(*bag));
 	bag->object_table = g_hash_table_new(hash, equal);
+	bag->copy_key = keycopy;
+	bag->free_key = keyfree;
 	bag->key_table = g_hash_table_new(NULL, NULL);
 	bag->owner = 0;
 #ifdef ENABLE_THREADS
@@ -1098,7 +1102,7 @@
 }
 
 static void
-save_object(char *key, CamelObject *o, GPtrArray *objects)
+save_object(void *key, CamelObject *o, GPtrArray *objects)
 {
 	g_ptr_array_add(objects, o);
 }
@@ -1112,8 +1116,10 @@
 	g_assert(i == 1);
 
 	g_hash_table_foreach(bag->object_table, (GHFunc)save_object, objects);
-	for (i=0;i<objects->len;i++)
+	for (i=0;i<objects->len;i++) {
 		camel_object_bag_remove(bag, objects->pdata[i]);
+		bag->free_key(objects->pdata[i]);
+	}
 	g_ptr_array_free(objects, TRUE);
 	g_hash_table_destroy(bag->object_table);
 	g_hash_table_destroy(bag->key_table);
@@ -1123,12 +1129,12 @@
 	g_free(bag);
 }
 
-void camel_object_bag_add(CamelObjectBag *bag, const char *key, void *vo)
+void camel_object_bag_add(CamelObjectBag *bag, const void *key, void *vo)
 {
 	CamelObject *o = vo;
 	CamelHookList *hooks;
 	CamelHookPair *pair;
-	char *k;
+	void *k;
 
 	hooks = camel_object_get_hooks(o);
 	E_LOCK(type_lock);
@@ -1152,7 +1158,7 @@
 	hooks->list = pair;
 	hooks->list_length++;
 
-	k = g_strdup(key);
+	k = bag->copy_key(key);
 	g_hash_table_insert(bag->object_table, k, vo);
 	g_hash_table_insert(bag->key_table, vo, k);
 
@@ -1167,7 +1173,7 @@
 	camel_object_unget_hooks(o);
 }
 
-void *camel_object_bag_get(CamelObjectBag *bag, const char *key)
+void *camel_object_bag_get(CamelObjectBag *bag, const void *key)
 {
 	CamelObject *o;
 
@@ -1201,7 +1207,7 @@
 /* After calling reserve, you MUST call bag_abort or bag_add */
 /* Also note that currently you can only reserve a single key
    at any one time in a given thread */
-void *camel_object_bag_reserve(CamelObjectBag *bag, const char *key)
+void *camel_object_bag_reserve(CamelObjectBag *bag, const void *key)
 {
 	CamelObject *o;
 
@@ -1235,7 +1241,7 @@
 }
 
 /* abort a reserved key */
-void camel_object_bag_abort(CamelObjectBag *bag, const char *key)
+void camel_object_bag_abort(CamelObjectBag *bag, const void *key)
 {
 #ifdef ENABLE_THREADS
 	g_assert(bag->owner == pthread_self());
@@ -1246,7 +1252,7 @@
 }
 
 static void
-save_bag(char *key, CamelObject *o, GPtrArray *list)
+save_bag(void *key, CamelObject *o, GPtrArray *list)
 {
 	/* we have the refcount lock already */
 	o->ref_count++;
@@ -1272,7 +1278,7 @@
 static void camel_object_bag_remove_unlocked(CamelObjectBag *inbag, CamelObject *o, CamelHookList *hooks)
 {
 	CamelHookPair *pair, *parent;
-	char *oldkey;
+	void *oldkey;
 	CamelObjectBag *bag;
 
 	parent = (CamelHookPair *)&hooks->list;
@@ -1286,7 +1292,7 @@
 			if (oldkey) {
 				g_hash_table_remove(bag->key_table, o);
 				g_hash_table_remove(bag->object_table, oldkey);
-				g_free(oldkey);
+				bag->free_key(oldkey);
 			}
 			parent->next = pair->next;
 			pair_free(pair);
Index: camel/camel-object.h
===================================================================
RCS file: /cvs/gnome/evolution/camel/camel-object.h,v
retrieving revision 1.21
diff -u -3 -r1.21 camel-object.h
--- camel/camel-object.h	4 Mar 2003 19:21:05 -0000	1.21
+++ camel/camel-object.h	22 Apr 2003 07:51:42 -0000
@@ -216,12 +216,13 @@
 
 /* for managing bags of weakly-ref'd 'child' objects */
 typedef struct _CamelObjectBag CamelObjectBag;
+typedef void *(*CamelCopyFunc)(const void *vo);
 
-CamelObjectBag *camel_object_bag_new(GHashFunc hash, GEqualFunc equal);
-void *camel_object_bag_get(CamelObjectBag *bag, const char *key);
-void *camel_object_bag_reserve(CamelObjectBag *bag, const char *key);
-void camel_object_bag_add(CamelObjectBag *bag, const char *key, void *o);
-void camel_object_bag_abort(CamelObjectBag *bag, const char *key);
+CamelObjectBag *camel_object_bag_new(GHashFunc hash, GEqualFunc equal, CamelCopyFunc keycopy, GFreeFunc keyfree);
+void *camel_object_bag_get(CamelObjectBag *bag, const void *key);
+void *camel_object_bag_reserve(CamelObjectBag *bag, const void *key);
+void camel_object_bag_add(CamelObjectBag *bag, const void *key, void *o);
+void camel_object_bag_abort(CamelObjectBag *bag, const void *key);
 GPtrArray *camel_object_bag_list(CamelObjectBag *bag);
 void camel_object_bag_remove(CamelObjectBag *bag, void *o);
 void camel_object_bag_destroy(CamelObjectBag *bag);
Index: camel/camel-provider.h
===================================================================
RCS file: /cvs/gnome/evolution/camel/camel-provider.h,v
retrieving revision 1.36
diff -u -3 -r1.36 camel-provider.h
--- camel/camel-provider.h	7 Jan 2003 21:03:56 -0000	1.36
+++ camel/camel-provider.h	22 Apr 2003 07:51:42 -0000
@@ -172,7 +172,7 @@
 	/* GList of CamelServiceAuthTypes the provider supports */
 	GList *authtypes;
 	
-	GHashTable *service_cache[CAMEL_NUM_PROVIDER_TYPES];
+	CamelObjectBag *service_cache[CAMEL_NUM_PROVIDER_TYPES];
 	
 	GHashFunc url_hash;
 	GCompareFunc url_equal;
Index: camel/camel-session.c
===================================================================
RCS file: /cvs/gnome/evolution/camel/camel-session.c,v
retrieving revision 1.88
diff -u -3 -r1.88 camel-session.c
--- camel/camel-session.c	6 Mar 2003 14:47:05 -0000	1.88
+++ camel/camel-session.c	22 Apr 2003 07:51:44 -0000
@@ -109,7 +109,7 @@
 
 	for (i = 0; i < CAMEL_NUM_PROVIDER_TYPES; i++) {
 		if (prov->service_cache[i])
-			g_hash_table_destroy (prov->service_cache[i]);
+			camel_object_bag_destroy (prov->service_cache[i]);
 	}
 	return TRUE;
 }
@@ -203,7 +203,8 @@
 
 	for (i = 0; i < CAMEL_NUM_PROVIDER_TYPES; i++) {
 		if (provider->object_types[i])
-			provider->service_cache[i] = g_hash_table_new (provider->url_hash, provider->url_equal);
+			provider->service_cache[i] = camel_object_bag_new (provider->url_hash, provider->url_equal,
+									   (CamelCopyFunc)camel_url_copy, (GFreeFunc)camel_url_free);
 	}
 
 	/* Translate all strings here */
@@ -383,24 +384,6 @@
 	return provider;
 }
 
-
-static void
-service_cache_remove (CamelService *service, gpointer event_data, gpointer user_data)
-{
-	CamelSession *session = service->session;
-	CamelProviderType type = GPOINTER_TO_INT (user_data);
-
-	g_return_if_fail (CAMEL_IS_SESSION (session));
-	g_return_if_fail (service != NULL);
-	g_return_if_fail (service->url != NULL);
-
-	CAMEL_SESSION_LOCK(session, lock);
-
-	g_hash_table_remove (service->provider->service_cache[type], service->url);
-
-	CAMEL_SESSION_UNLOCK(session, lock);
-}
-
 static CamelService *
 get_service (CamelSession *session, const char *url_string,
 	     CamelProviderType type, CamelException *ex)
@@ -436,10 +419,9 @@
 		camel_url_set_path (url, NULL);
 	
 	/* Now look up the service in the provider's cache */
-	service = g_hash_table_lookup (provider->service_cache[type], url);
+	service = camel_object_bag_reserve(provider->service_cache[type], url);
 	if (service != NULL) {
 		camel_url_free (url);
-		camel_object_ref (CAMEL_OBJECT (service));
 		return service;
 	}
 
@@ -448,15 +430,15 @@
 	camel_service_construct (service, session, provider, url, &internal_ex);
 	if (camel_exception_is_set (&internal_ex)) {
 		camel_exception_xfer (ex, &internal_ex);
-		camel_object_unref (CAMEL_OBJECT (service));
+		camel_object_unref (service);
 		service = NULL;
+		camel_object_bag_abort(provider->service_cache[type], url);
 	} else {
-		g_hash_table_insert (provider->service_cache[type], url, service);
-		camel_object_hook_event (CAMEL_OBJECT (service), "finalize",
-					 (CamelObjectEventHookFunc) service_cache_remove,
-					 GINT_TO_POINTER (type));
+		camel_object_bag_add(provider->service_cache[type], url, service);
 	}
-	
+
+	camel_url_free(url);
+
 	return service;
 }
 
Index: camel/camel-store.c
===================================================================
RCS file: /cvs/gnome/evolution/camel/camel-store.c,v
retrieving revision 1.115
diff -u -3 -r1.115 camel-store.c
--- camel/camel-store.c	28 Mar 2003 00:13:43 -0000	1.115
+++ camel/camel-store.c	22 Apr 2003 07:51:46 -0000
@@ -127,7 +127,8 @@
 
 	if (store_class->hash_folder_name) {
 		store->folders = camel_object_bag_new(store_class->hash_folder_name,
-						      store_class->compare_folder_name);
+						      store_class->compare_folder_name,
+						      (CamelCopyFunc)g_strdup, g_free);
 	} else
 		store->folders = NULL;
 	
Index: camel/camel-url.c
===================================================================
RCS file: /cvs/gnome/evolution/camel/camel-url.c,v
retrieving revision 1.34
diff -u -3 -r1.34 camel-url.c
--- camel/camel-url.c	27 Mar 2003 15:37:46 -0000	1.34
+++ camel/camel-url.c	22 Apr 2003 07:51:47 -0000
@@ -560,3 +560,23 @@
 		&& check_equal(u1->query, u2->query)
 		&& u1->port == u2->port;
 }
+
+CamelURL *camel_url_copy(const CamelURL *in)
+{
+	CamelURL *out;
+
+	out = g_malloc0(sizeof(*out));
+	out->protocol = g_strdup(in->protocol);
+	out->user = g_strdup(in->user);
+	out->authmech = g_strdup(in->authmech);
+	out->passwd = g_strdup(in->passwd);
+	out->host = g_strdup(in->host);
+	out->port = in->port;
+	out->path = g_strdup(in->path);
+	if (in->params)
+		g_datalist_foreach(&((CamelURL *)in)->params, copy_param, &out->params);
+	out->query = g_strdup(in->query);
+	out->fragment = g_strdup(in->fragment);
+
+	return out;
+}
Index: camel/camel-url.h
===================================================================
RCS file: /cvs/gnome/evolution/camel/camel-url.h,v
retrieving revision 1.18
diff -u -3 -r1.18 camel-url.h
--- camel/camel-url.h	27 Mar 2003 15:37:46 -0000	1.18
+++ camel/camel-url.h	22 Apr 2003 07:51:48 -0000
@@ -79,6 +79,7 @@
 /* for putting url's into hash tables */
 guint camel_url_hash (const void *v);
 int camel_url_equal(const void *v, const void *v2);
+CamelURL *camel_url_copy(const CamelURL *in);
 
 #ifdef __cplusplus
 }
Index: camel/tests/folder/Makefile.am
===================================================================
RCS file: /cvs/gnome/evolution/camel/tests/folder/Makefile.am,v
retrieving revision 1.11
diff -u -3 -r1.11 Makefile.am
--- camel/tests/folder/Makefile.am	19 Nov 2002 17:21:40 -0000	1.11
+++ camel/tests/folder/Makefile.am	22 Apr 2003 07:51:48 -0000
@@ -19,8 +19,10 @@
 check_PROGRAMS =  	\
 	test1	test2	test3	\
 	test4	test5	test6	\
-	test7	test8	test9
+	test7	test8	test9	\
+	test10
 
 TESTS = test1 	test2 	test3 	\
 	test4 	test5 	test6 	\
-	test7	test8	test9
+	test7	test8	test9	\
+	test10
Index: camel/tests/folder/README
===================================================================
RCS file: /cvs/gnome/evolution/camel/tests/folder/README,v
retrieving revision 1.4
diff -u -3 -r1.4 README
--- camel/tests/folder/README	17 Jan 2001 01:07:01 -0000	1.4
+++ camel/tests/folder/README	22 Apr 2003 07:51:48 -0000
@@ -9,3 +9,4 @@
 
 test8	multithreaded folder torture test, local
 test9	filtering
+test10  multithreaded folder/store object bag torture test
Index: camel/tests/folder/test10.c
===================================================================
RCS file: camel/tests/folder/test10.c
diff -N camel/tests/folder/test10.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ camel/tests/folder/test10.c	22 Apr 2003 07:51:48 -0000
@@ -0,0 +1,115 @@
+/* threaded folder testing */
+
+#include <string.h>
+
+#include "camel-test.h"
+#include "folders.h"
+#include "messages.h"
+#include "session.h"
+
+#include <camel/camel-exception.h>
+#include <camel/camel-service.h>
+#include <camel/camel-store.h>
+
+#define MAX_LOOP (10000)
+#define MAX_THREADS (5)
+
+#define d(x) 
+
+#ifndef ENABLE_THREADS
+int main(int argc, char **argv)
+{
+	printf("Test %s is only compiled with threads enabled\n", argv[0]);
+	return 77;
+}
+#else
+
+#include <pthread.h>
+
+
+#define ARRAY_LEN(x) (sizeof(x)/sizeof(x[0]))
+
+static char *local_providers[] = {
+	"mbox",
+	"mh",
+	"maildir"
+};
+
+static char *path;
+static CamelSession *session;
+static int testid;
+
+static void *
+worker(void *d)
+{
+	int i;
+	CamelException *ex = camel_exception_new();
+	CamelStore *store;
+	CamelFolder *folder;
+
+	for (i=0;i<MAX_LOOP;i++) {
+		store = camel_session_get_store(session, path, ex);
+		camel_exception_clear(ex);
+		folder = camel_store_get_folder(store, "testbox", CAMEL_STORE_FOLDER_CREATE, ex);
+		camel_exception_clear(ex);
+		if (testid == 0) {
+			camel_object_unref(folder);
+			camel_object_unref(store);
+		} else {
+			camel_object_unref(store);
+			camel_object_unref(folder);
+		}
+	}
+
+	camel_exception_free(ex);
+
+	return NULL;
+}
+
+int main(int argc, char **argv)
+{
+	CamelException *ex;
+	int i, j;
+	pthread_t threads[MAX_THREADS];
+
+	camel_test_init(argc, argv);
+
+	ex = camel_exception_new();
+
+	/* clear out any camel-test data */
+	system("/bin/rm -rf /tmp/camel-test");
+
+	session = camel_test_session_new ("/tmp/camel-test");
+
+	for (testid=0;testid<2;testid++) {
+		if (testid == 0)
+			camel_test_start("store and folder bag torture test, stacked references");
+		else
+			camel_test_start("store and folder bag torture test, unstacked references");
+
+		for (j=0;j<ARRAY_LEN(local_providers);j++) {
+
+			camel_test_push("provider %s", local_providers[j]);
+			path = g_strdup_printf("%s:///tmp/camel-test/%s", local_providers[j], local_providers[j]);
+		
+			for (i=0;i<MAX_THREADS;i++)
+				pthread_create(&threads[i], 0, worker, NULL);
+			
+			for (i=0;i<MAX_THREADS;i++) 
+				pthread_join(threads[i], NULL);
+			
+			test_free(path);
+
+			camel_test_pull();
+		}
+
+		camel_test_end();
+	}
+
+	camel_object_unref((CamelObject *)session);
+	camel_exception_free(ex);
+
+	return 0;
+}
+
+#endif /* ENABLE_THREADS */