{"thread":{"id":"29897","subject":"[PATCH] builtin/index-pack.c: Fix crash on MinGW caused by uninitialised mutex","startedAt":"2012-03-09T18:44:36Z","lastAt":"2012-03-09T20:30:25Z","messageCount":2,"participants":["Ramsay Jones","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"186579","messageId":"4F5A4F94.1060309@ramsay1.demon.co.uk","threadId":"29897","inReplyTo":null,"subject":"[PATCH] builtin/index-pack.c: Fix crash on MinGW caused by uninitialised mutex","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-03-09T18:44:36Z","receivedAt":"2012-03-09T18:44:36Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nOn MinGW, the pthread emulation code uses a CRITICAL_SECTION in\nit's implementation of an pthread_mutex_t. Any attempt to use an\nuninitialised CRITICAL_SECTION results in a crash (in ntddl.dll).\nOn cygwin (and Linux), using an uninitialised pthread_mutex_t looks\nlike a no-op, or at least it does not seem to cause a problem.\n\nIn addition to not initialising and destroying the 'counter_mutex',\nin the init_thread() and cleanup_thread() routines, several code\npaths attempt to lock/unlock a mutex when not correctly initialised.\n\nFor example, in the parse_pack_objects() function, the 'first pass'\ncode contains calls to the sha1_object() function which, in turn,\nattempts to lock the 'read_mutex' via read_lock(). At this point in\nthe function, no threads are active and none of the mutex variables\nhave been initialised (since init_thread() has not been called).\n\nIn order to avoid the crash on MinGW, we guard all calls to the\nmutex (un-)locking functions to ensure that the mutex variables are\nin a valid state to be operated on. We introduce an 'threads_active'\nglobal variable which we set appropriately in the init_thread() and\ncleanup_thread() routines to reflect the current threading state.\nAlso, we add the missing initialisation and destruction of the\n'counter_mutex' to init_thread() and cleanup_thread().\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n\nHi Nguyen,\n\nThis patch, on top of my earlier one, fixes the problem on MinGW.\nI am assuming that the tests t5300-pack-object.sh, t5302-pack-index.sh,\nt5510-fetch.sh and t6050-replace.sh are sufficient to test the new\nthreaded version of git-index-pack. In any event, I have tested this\non MinGW, cygwin and Linux and all of the above tests pass.\n\nWhile writing this mail, I had second thoughts on the 'threads_active'\nname, but then (for similar reasons) init_thread() and cleanup_thread()\nare not exactly right either. So, in the end, I decided not to re-name\nthat variable ... ;-)\n\nWhen I'm away from home (and email), I sometimes use my isp's web-mail\ninterface to squint at the emails I will have waiting for me when I get home.\nThis afternoon I noticed that you had sent a patch (addressing this MinGW\nproblem) for me to try out. I won't get your mail into my MUA until after\nI've sent this out. Since it will be one of approximately 450, I probably\nwon't get to it tonight ... :-D\n\nATB,\nRamsay Jones\n\n builtin/index-pack.c |   18 ++++++++++++------\n 1 files changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex f8d93b8..ef81676 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -74,6 +74,7 @@ static int nr_processed;\n static int nr_deltas;\n static int nr_resolved_deltas;\n static int nr_threads;\n+static int threads_active;\n \n static int from_stdin;\n static int strict;\n@@ -93,16 +94,17 @@ static int input_fd, output_fd, pack_fd;\n #ifndef NO_PTHREADS\n \n static pthread_mutex_t read_mutex;\n-#define read_lock()\t\tpthread_mutex_lock(&read_mutex)\n-#define read_unlock()\t\tpthread_mutex_unlock(&read_mutex)\n+#define read_lock()\t\tif (threads_active) pthread_mutex_lock(&read_mutex)\n+#define read_unlock()\t\tif (threads_active) pthread_mutex_unlock(&read_mutex)\n \n static pthread_mutex_t counter_mutex;\n-#define counter_lock()\t\tpthread_mutex_lock(&counter_mutex)\n-#define counter_unlock()\t\tpthread_mutex_unlock(&counter_mutex)\n+#define counter_lock()\t\tif (threads_active) pthread_mutex_lock(&counter_mutex)\n+#define counter_unlock()\tif (threads_active) pthread_mutex_unlock(&counter_mutex)\n \n static pthread_mutex_t work_mutex;\n-#define work_lock()\t\tpthread_mutex_lock(&work_mutex)\n-#define work_unlock()\t\tpthread_mutex_unlock(&work_mutex)\n+#define work_lock()\t\tif (threads_active) pthread_mutex_lock(&work_mutex)\n+#define work_unlock()\t\tif (threads_active) pthread_mutex_unlock(&work_mutex)\n+\n \n /*\n  * Mutex and conditional variable can't be statically-initialized on Windows.\n@@ -110,12 +112,16 @@ static pthread_mutex_t work_mutex;\n static void init_thread(void)\n {\n \tinit_recursive_mutex(&read_mutex);\n+\tpthread_mutex_init(&counter_mutex, NULL);\n \tpthread_mutex_init(&work_mutex, NULL);\n+\tthreads_active = 1;\n }\n \n static void cleanup_thread(void)\n {\n+\tthreads_active = 0;\n \tpthread_mutex_destroy(&read_mutex);\n+\tpthread_mutex_destroy(&counter_mutex);\n \tpthread_mutex_destroy(&work_mutex);\n }\n \n-- \n1.7.9\n"},{"id":"186584","messageId":"7v1up15xge.fsf@alter.siamese.dyndns.org","threadId":"29897","inReplyTo":"4F5A4F94.1060309@ramsay1.demon.co.uk","subject":"Re: [PATCH] builtin/index-pack.c: Fix crash on MinGW caused by uninitialised mutex","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-09T20:30:25Z","receivedAt":"2012-03-09T20:30:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n> Hi Nguyen,\n>\n> This patch, on top of my earlier one, fixes the problem on MinGW.\n> I am assuming that the tests t5300-pack-object.sh, t5302-pack-index.sh,\n> t5510-fetch.sh and t6050-replace.sh are sufficient to test the new\n> threaded version of git-index-pack. In any event, I have tested this\n> on MinGW, cygwin and Linux and all of the above tests pass.\n\nI'd drop what is in 'pu' which is a few iterations old already and\nwill not hit 'master' until the next cycle begins when I'd expect a\nre-rolled series.\n\nThanks both for working on this one.  \n"}]}