[PATCH] builtin/index-pack.c: Fix crash on MinGW caused by uninitialised mutex

[PATCH] builtin/index-pack.c: Fix crash on MinGW caused by uninitialised mutex

[Ramsay Jones]

On MinGW, the pthread emulation code uses a CRITICAL_SECTION in
it's implementation of an pthread_mutex_t. Any attempt to use an
uninitialised CRITICAL_SECTION results in a crash (in ntddl.dll).
On cygwin (and Linux), using an uninitialised pthread_mutex_t looks
like a no-op, or at least it does not seem to cause a problem.

In addition to not initialising and destroying the 'counter_mutex',
in the init_thread() and cleanup_thread() routines, several code
paths attempt to lock/unlock a mutex when not correctly initialised.

For example, in the parse_pack_objects() function, the 'first pass'
code contains calls to the sha1_object() function which, in turn,
attempts to lock the 'read_mutex' via read_lock(). At this point in
the function, no threads are active and none of the mutex variables
have been initialised (since init_thread() has not been called).

In order to avoid the crash on MinGW, we guard all calls to the
mutex (un-)locking functions to ensure that the mutex variables are
in a valid state to be operated on. We introduce an 'threads_active'
global variable which we set appropriately in the init_thread() and
cleanup_thread() routines to reflect the current threading state.
Also, we add the missing initialisation and destruction of the
'counter_mutex' to init_thread() and cleanup_thread().

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---

Hi Nguyen,

This patch, on top of my earlier one, fixes the problem on MinGW.
I am assuming that the tests t5300-pack-object.sh, t5302-pack-index.sh,
t5510-fetch.sh and t6050-replace.sh are sufficient to test the new
threaded version of git-index-pack. In any event, I have tested this
on MinGW, cygwin and Linux and all of the above tests pass.

While writing this mail, I had second thoughts on the 'threads_active'
name, but then (for similar reasons) init_thread() and cleanup_thread()
are not exactly right either. So, in the end, I decided not to re-name
that variable ... ;-)

When I'm away from home (and email), I sometimes use my isp's web-mail
interface to squint at the emails I will have waiting for me when I get home.
This afternoon I noticed that you had sent a patch (addressing this MinGW
problem) for me to try out. I won't get your mail into my MUA until after
I've sent this out. Since it will be one of approximately 450, I probably
won't get to it tonight ... :-D

ATB,
Ramsay Jones

 builtin/index-pack.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index f8d93b8..ef81676 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,17 @@ 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)
+
 
 /*
  * Mutex and conditional variable can't be statically-initialized on Windows.
@@ -110,12 +112,16 @@ static pthread_mutex_t work_mutex;
 static void init_thread(void)
 {
 	init_recursive_mutex(&read_mutex);
+	pthread_mutex_init(&counter_mutex, NULL);
 	pthread_mutex_init(&work_mutex, 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);
 }
 
-- 
1.7.9

Re: [PATCH] builtin/index-pack.c: Fix crash on MinGW caused by uninitialised mutex

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

> Hi Nguyen,
>
> This patch, on top of my earlier one, fixes the problem on MinGW.
> I am assuming that the tests t5300-pack-object.sh, t5302-pack-index.sh,
> t5510-fetch.sh and t6050-replace.sh are sufficient to test the new
> threaded version of git-index-pack. In any event, I have tested this
> on MinGW, cygwin and Linux and all of the above tests pass.

I'd drop what is in 'pu' which is a few iterations old already and
will not hit 'master' until the next cycle begins when I'd expect a
re-rolled series.

Thanks both for working on this one.