bug#22152: fat_mutex owner corruption (fmoc) inside fat_mutex_unlock (guile-v2.0.11)

[Mark H Weaver <mhw@netris.org>]

[-- Attachment #1: Type: text/plain, Size: 458 bytes --]

Iwan Aucamp <aucampia@gmail.com> writes:
> We sporadically get "mutex not locked" and "mutex not locked by current thread"
> exceptions on Solaris 10u10 with guile-2.0.11.

Thanks very much for your detailed analysis and proposed fix.

I've attached a patch that hopefully fixes this bug and also refactors
the code to hopefully be somewhat more clear.  Can you please test it on
Solaris and verify that it works for your use cases?

    Thanks,
      Mark


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Preliminary patch to fix and refactor fat_mutex_unlock --]
[-- Type: text/x-patch, Size: 2754 bytes --]

From 1d945f638033e7649ea61fa1c4050b30da891d6b Mon Sep 17 00:00:00 2001
From: Mark H Weaver <mhw@netris.org>
Date: Fri, 18 Dec 2015 02:29:48 -0500
Subject: [PATCH] PRELIMINARY: Fix bug in fat_mutex_unlock.

---
 libguile/threads.c | 87 ++++++++++++++++++++++--------------------------------
 1 file changed, 36 insertions(+), 51 deletions(-)

diff --git a/libguile/threads.c b/libguile/threads.c
index 6ae6818..dcbd24d 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -1607,70 +1607,55 @@ fat_mutex_unlock (SCM mutex, SCM cond,
 	}
     }
 
+  if (m->level > 0)
+    m->level--;
+  if (m->level == 0)
+    {
+      /* Change the owner of MUTEX.  */
+      t->mutexes = scm_delq_x (mutex, t->mutexes);
+      m->owner = unblock_from_queue (m->waiting);
+    }
+
   if (! (SCM_UNBNDP (cond)))
     {
       c = SCM_CONDVAR_DATA (cond);
-      while (1)
-	{
-	  int brk = 0;
-
-	  if (m->level > 0)
-	    m->level--;
-	  if (m->level == 0)
-	    {
-	      /* Change the owner of MUTEX.  */
-	      t->mutexes = scm_delq_x (mutex, t->mutexes);
-	      m->owner = unblock_from_queue (m->waiting);
-	    }
-
-	  t->block_asyncs++;
+      t->block_asyncs++;
 
+      do
+	{
 	  err = block_self (c->waiting, cond, &m->lock, waittime);
 	  scm_i_pthread_mutex_unlock (&m->lock);
 
-	  if (err == 0)
-	    {
-	      ret = 1;
-	      brk = 1;
-	    }
-	  else if (err == ETIMEDOUT)
-	    {
-	      ret = 0;
-	      brk = 1;
-	    }
-	  else if (err != EINTR)
-	    {
-	      errno = err;
-	      scm_syserror (NULL);
-	    }
-
-	  if (brk)
-	    {
-	      if (relock)
-		scm_lock_mutex_timed (mutex, SCM_UNDEFINED, owner);
-	      t->block_asyncs--;
-	      break;
-	    }
-
-	  t->block_asyncs--;
-	  scm_async_click ();
+          if (err == EINTR)
+            {
+              t->block_asyncs--;
+              scm_async_click ();
 
-	  scm_remember_upto_here_2 (cond, mutex);
+              scm_remember_upto_here_2 (cond, mutex);
 
-	  scm_i_scm_pthread_mutex_lock (&m->lock);
+              scm_i_scm_pthread_mutex_lock (&m->lock);
+              t->block_asyncs++;
+            }
 	}
+      while (err == EINTR);
+
+      if (err == 0)
+        ret = 1;
+      else if (err == ETIMEDOUT)
+        ret = 0;
+      else
+        {
+          t->block_asyncs--;
+          errno = err;
+          scm_syserror (NULL);
+        }
+
+      if (relock)
+        scm_lock_mutex_timed (mutex, SCM_UNDEFINED, owner);
+      t->block_asyncs--;
     }
   else
     {
-      if (m->level > 0)
-	m->level--;
-      if (m->level == 0)
-	{
-	  /* Change the owner of MUTEX.  */
-	  t->mutexes = scm_delq_x (mutex, t->mutexes);
-	  m->owner = unblock_from_queue (m->waiting);
-	}
-
       scm_i_pthread_mutex_unlock (&m->lock);
       ret = 1;
     }
-- 
2.6.3