From f57fdf07d6374028f35bcb1ee748a94022deda6d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Thu, 2 Sep 2010 14:42:14 +0200
Subject: [PATCH] Fix memory leak in `lock-mutex' (aka. `scm_lock_mutex'.)

The memory leak is trivially reproducible with:

  (define m (make-mutex))
  (let loop () (lock-mutex m) (unlock-mutex m) (loop))

or similarly with:

  (define p (delay (+ 1 2)))
  (let loop () (force p) (loop))

since `force' acquires P's mutex.

It could also lead to premature release of a thread waiting in
`fat_mutex_lock' when a former owner's `do_thread_exit' is run.

* libguile/threads.c (fat_mutex_unlock): When `m->level' becomes 0,
  remove MUTEX from `t->mutexes'.
  (fat_mutex_lock): Update comment above the `t->mutexes' assignment.
  (do_thread_exit): Add an assertion making sure that each mutex in
  `t->mutexes' is owned by T.
---
 libguile/threads.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/libguile/threads.c b/libguile/threads.c
index 457b1903f..8249ebf90 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -38,6 +38,8 @@
 #include <sys/time.h>
 #endif
 
+#include <assert.h>
+
 #include "libguile/validate.h"
 #include "libguile/root.h"
 #include "libguile/eval.h"
@@ -466,7 +468,12 @@ do_thread_exit (void *v)
 	  fat_mutex *m  = SCM_MUTEX_DATA (mutex);
 
 	  scm_i_pthread_mutex_lock (&m->lock);
+
+	  /* Since MUTEX is in `t->mutexes', T must be its owner.  */
+	  assert (scm_is_eq (m->owner, t->handle));
+
 	  unblock_from_queue (m->waiting);
+
 	  scm_i_pthread_mutex_unlock (&m->lock);
 	}
 
@@ -1234,10 +1241,10 @@ fat_mutex_lock (SCM mutex, scm_t_timespec *timeout, SCM owner, int *ret)
 	      scm_i_pthread_mutex_lock (&t->admin_mutex);
 
 	      /* Only keep a weak reference to MUTEX so that it's not
-		 retained when not referenced elsewhere (bug #27450).  Note
-		 that the weak pair itself it still retained, but it's better
-		 than retaining MUTEX and the threads referred to by its
-		 associated queue.  */
+		 retained when not referenced elsewhere (bug #27450).
+		 The weak pair itself is eventually removed when MUTEX
+		 is unlocked.  Note that `t->mutexes' lists mutexes
+		 currently held by T, so it should be small.  */
 	      t->mutexes = scm_weak_car_pair (mutex, t->mutexes);
 
 	      scm_i_pthread_mutex_unlock (&t->admin_mutex);
@@ -1409,7 +1416,11 @@ fat_mutex_unlock (SCM mutex, SCM cond,
 	  if (m->level > 0)
 	    m->level--;
 	  if (m->level == 0)
-	    m->owner = unblock_from_queue (m->waiting);
+	    {
+	      /* Change the owner of MUTEX.  */
+	      t->mutexes = scm_delq_x (mutex, t->mutexes);
+	      m->owner = unblock_from_queue (m->waiting);
+	    }
 
 	  t->block_asyncs++;
 
@@ -1453,7 +1464,11 @@ fat_mutex_unlock (SCM mutex, SCM cond,
       if (m->level > 0)
 	m->level--;
       if (m->level == 0)
-	m->owner = unblock_from_queue (m->waiting);
+	{
+	  /* 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;