Subject: Re: [PATCH] builtin/index-pack.c: Fix some pthread_t misuse

From: Ramsay Jones <[email-masked]>
Date: Sat, 10 Mar 2012 21:20:58 +0000

Nguyen Thai Ngoc Duy wrote:
> Does this help your crash Ramsay? It also moves down cleanup_thread()
> so counter_mutex is always valid when it's used. Seems to work with
> linux-2.6.git.

[ patch snipped ]

No this does not help, because it doesn't address the fundamental problem.

The problem is simply that mutex variables are being used when they are
not in a usable state; either they are not initialised at all (counter_mutex),
or they are being used *before* they are initialised (in init_thread()) or
*after* they are destroyed (in cleanup_thread()).

If I apply the patch below on top of your patch (it should look familiar!),
then everything works fine. (This time I only tested on MinGW and cygwin).

Also, I would move the call to cleanup_thread() back up to it's original
location, since the move down serves no useful purpose.

I don't know that using TLS here is an improvement or not (it's probably a
wash). ;-)

[BTW, using an uninitialised mutex on cygwin and Linux *seems* to work without
problem, but I can't be sure that's true (I haven't looked at the code). :-P ]

ATB,
Ramsay Jones

-- 8< --
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 098f350..31f923c 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -74,6 +74,7 @@ static int nr_processed;
 static int nr_deltas;
 static int nr_resolved_deltas;
 static int nr_threads;
+static int threads_active;
 
 static int from_stdin;
 static int strict;
@@ -93,16 +94,16 @@ static int input_fd, output_fd, pack_fd;
 #ifndef NO_PTHREADS
 
 static pthread_mutex_t read_mutex;
-#define read_lock()		pthread_mutex_lock(&read_mutex)
-#define read_unlock()		pthread_mutex_unlock(&read_mutex)
+#define read_lock()		if (threads_active) pthread_mutex_lock(&read_mutex)
+#define read_unlock()		if (threads_active) pthread_mutex_unlock(&read_mutex)
 
 static pthread_mutex_t counter_mutex;
-#define counter_lock()		pthread_mutex_lock(&counter_mutex)
-#define counter_unlock()		pthread_mutex_unlock(&counter_mutex)
+#define counter_lock()		if (threads_active) pthread_mutex_lock(&counter_mutex)
+#define counter_unlock()	if (threads_active) pthread_mutex_unlock(&counter_mutex)
 
 static pthread_mutex_t work_mutex;
-#define work_lock()		pthread_mutex_lock(&work_mutex)
-#define work_unlock()		pthread_mutex_unlock(&work_mutex)
+#define work_lock()		if (threads_active) pthread_mutex_lock(&work_mutex)
+#define work_unlock()		if (threads_active) pthread_mutex_unlock(&work_mutex)
 
 static pthread_key_t key;
 
@@ -112,13 +113,17 @@ static pthread_key_t key;
 static void init_thread(void)
 {
 	init_recursive_mutex(&read_mutex);
+	pthread_mutex_init(&counter_mutex, NULL);
 	pthread_mutex_init(&work_mutex, NULL);
 	pthread_key_create(&key, NULL);
+	threads_active = 1;
 }
 
 static void cleanup_thread(void)
 {
+	threads_active = 0;
 	pthread_mutex_destroy(&read_mutex);
+	pthread_mutex_destroy(&counter_mutex);
 	pthread_mutex_destroy(&work_mutex);
 }
 
-- 8< --