[PATCH] git grep: be careful to use mutices only when they are initialized

Rather nasty things happen when a mutex is not initialized but locked
nevertheless. Now, when we're not running in a threaded manner, the mutex
is not initialized, which is correct. But then we went and used the mutex
anyway, which -- at least on Windows -- leads to a hard crash (ordinarily
it would be called a segmentation fault, but in Windows speak it is an
access violation).

This problem was identified by our faithful tests when run in the msysGit
environment.

To avoid having to wrap the line due to the 80 column limit, we use
the name "WHEN_THREADED" instead of "IF_USE_THREADS" because it is one
character shorter. Which is all we need in this case.

Signed-off-by: Johannes Schindelin <redacted>
---

	I looked around a bit but ran out of time to identify the reason why
	this was not caught earlier.

 builtin/grep.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 92eeada..e94c5fe 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -78,10 +78,11 @@ static pthread_mutex_t grep_mutex;
 /* Used to serialize calls to read_sha1_file. */
 static pthread_mutex_t read_sha1_mutex;
 
-#define grep_lock() pthread_mutex_lock(&grep_mutex)
-#define grep_unlock() pthread_mutex_unlock(&grep_mutex)
-#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex)
-#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex)
+#define WHEN_THREADED(x) do { if (use_threads) (x); } while (0)
+#define grep_lock() WHEN_THREADED(pthread_mutex_lock(&grep_mutex))
+#define grep_unlock() WHEN_THREADED(pthread_mutex_unlock(&grep_mutex))
+#define read_sha1_lock() WHEN_THREADED(pthread_mutex_lock(&read_sha1_mutex))
+#define read_sha1_unlock() WHEN_THREADED(pthread_mutex_unlock(&read_sha1_mutex))
 
 /* Signalled when a new work_item is added to todo. */
 static pthread_cond_t cond_add;

-- 
1.7.5.3.4540.g15f89

On Wed, Oct 26, 2011 at 1:25 AM, Johannes Schindelin
[off-list ref] wrote:

Rather nasty things happen when a mutex is not initialized but locked
nevertheless. Now, when we're not running in a threaded manner, the mutex
is not initialized, which is correct. But then we went and used the mutex
anyway, which -- at least on Windows -- leads to a hard crash (ordinarily
it would be called a segmentation fault, but in Windows speak it is an
access violation).

This problem was identified by our faithful tests when run in the msysGit
environment.

May I ask which test are you talking about specifically?

I ask as I'm curious how this is triggered; git-grep works fine for me
so far (1.7.6.msysgit.0.584.g2cbf)

-- 
Cheers,
Ray Chuan

On 25 October 2011 18:25, Johannes Schindelin
[off-list ref] wrote:

Rather nasty things happen when a mutex is not initialized but locked
nevertheless. Now, when we're not running in a threaded manner, the mutex
is not initialized, which is correct. But then we went and used the mutex
anyway, which -- at least on Windows -- leads to a hard crash (ordinarily
it would be called a segmentation fault, but in Windows speak it is an
access violation).

This problem was identified by our faithful tests when run in the msysGit
environment.

I did not see this failure when running the tests on my machine. But
then threaded issues are often intermittent depending on load, number
of cores, phase of the moon, etc. You never said _which_ test either
although there are only 3 to try - most likey t7810-grep.sh

I was going to point out that it should be "mutexes" but I see it is
committed already :)

To avoid having to wrap the line due to the 80 column limit, we use

So last century!

Works for me.

Pat.

Hi Tay,

On Wed, 26 Oct 2011, Tay Ray Chuan wrote:

On Wed, Oct 26, 2011 at 1:25 AM, Johannes Schindelin
[off-list ref] wrote:

quoted

Rather nasty things happen when a mutex is not initialized but locked 
nevertheless. Now, when we're not running in a threaded manner, the 
mutex is not initialized, which is correct. But then we went and used 
the mutex anyway, which -- at least on Windows -- leads to a hard 
crash (ordinarily it would be called a segmentation fault, but in 
Windows speak it is an access violation).

This problem was identified by our faithful tests when run in the 
msysGit environment.

May I ask which test are you talking about specifically?

It is t7810.

I ask as I'm curious how this is triggered; git-grep works fine for me 
so far (1.7.6.msysgit.0.584.g2cbf)

That did not expose the error. The problem is exposed in msysGit's 'devel' 
branch, though.

Ciao,
Johannes

Johannes Schindelin [off-list ref] writes:

I think, from a quick glance, this is a good first step.

The remainder of this message are hints and random thoughts on potential
follow-up patches that may want to build on top of this patch for further
clean-ups (not specifically meant for Dscho but for other people on both
mailing lists).

 - The patch makes the check for use_threads in lock_and_read_sha1_file()
   redundant. The other user of read_sha1_lock/unlock in grep_object() can
   take advantage of this change (see below).

 - It makes me wonder if it is simpler to initialize mutexes even in
   !use_threads case.

 - Wouldn't the result be more readable to make these into static inline
   functions?

 - Could we lose "#ifndef NO_PTHREADS" inside grep_sha1(), grep_file(),
   and possibly cmd_grep() functions and let the compiler optimize things
   away under NO_PTHREADS compilation?

diff --git a/builtin/grep.c b/builtin/grep.c
index 7d0779f..60daa85 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -354,13 +354,9 @@ static void *lock_and_read_sha1_file(const unsigned char *sha1, enum object_type
 {
 	void *data;
 
-	if (use_threads) {
-		read_sha1_lock();
-		data = read_sha1_file(sha1, type, size);
-		read_sha1_unlock();
-	} else {
-		data = read_sha1_file(sha1, type, size);
-	}
+	read_sha1_lock();
+	data = read_sha1_file(sha1, type, size);
+	read_sha1_unlock();
 	return data;
 }
 

On Wed, Oct 26, 2011 at 01:02:40PM -0700, Junio C Hamano wrote:

 - Could we lose "#ifndef NO_PTHREADS" inside grep_sha1(), grep_file(),
   and possibly cmd_grep() functions and let the compiler optimize things
   away under NO_PTHREADS compilation?

I don't think so. If NO_PTHREADS is set, we might not have pthread
functions at all. Sure, many compilers will optimize:

  if (0)
          pthread_mutex_lock(...);

to remove the call completely. But would a compiler be wrong to complain
that pthread_mutex_lock is not defined, or to include reference to it
for the linker? gcc, both with and without optimizations, will complain
about:

  echo 'int main() { if (0) does_not_exist(); return 0; }' >foo.c
  gcc -Wall -c foo.c

though it does actually remove the dead code and link properly. I
wouldn't be surprised if some other compilers don't work, though (and of
course the warning is ugly).

I think you would have to do something like this in thread-utils.h:

  #ifndef NO_PTHREADS
  #include <pthread.h>
  #else
  #define pthread_mutex_t int
  #define pthread_mutex_init(m, a) do {} while(0)
  #define pthread_mutex_lock(m) do {} while(0)
  #define pthread_mutex_unlock(m) do {} while (0)
  /* and so forth for every pthread function */
  #endif

-Peff