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

From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date: Mar 10, 2012, 21:20 UTC
Message-ID: <4F5BC5BA.9000404@ramsay1.demon.co.uk>
In-Reply-To: <20120308012953.GA8444@duynguyen-vnpc.dek-tpc.internal>

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< --