Fix potential deadlock in `make-struct'.

* libguile/struct.c (scm_make_struct): Remove critical section, as
  suggested by Linas Vepstas <linasvepstas@gmail.com>.  See
  http://lists.gnu.org/archive/html/bug-guile/2008-11/msg00036.html for
  a discussion.

diff --git a/NEWS b/NEWS
index 0e6e081..c0020af 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -84,6 +84,7 @@ available: Guile is now always configured in "maintainer mode".
    same thread
 ** The handler of SRFI-34 `with-exception-handler' is now invoked in the
    dynamic environment of the call to `raise'
+** Fix potential deadlock in `make-struct'
 
 \f
 Changes in 1.8.5 (since 1.8.4)

diff --git a/libguile/struct.c b/libguile/struct.c
index 511fca4..cae0f31 100644 (file)
--- a/libguile/struct.c
+++ b/libguile/struct.c
@@ -449,8 +449,14 @@ SCM_DEFINE (scm_make_struct, "make-struct", 2, 0, 1,
       if (! SCM_LAYOUT_TAILP (SCM_CHAR (last_char)))
         goto bad_tail;
     }
-    
-  SCM_CRITICAL_SECTION_START;
+
+  /* In guile 1.8.5 and earlier, everything below was covered by a
+     CRITICAL_SECTION lock.  This can lead to deadlocks in garbage
+     collection, since other threads might be holding the heap_mutex, while
+     sleeping on the CRITICAL_SECTION lock.  There does not seem to be any
+     need for a lock on the section below, as it does not access or update
+     any globals, so the critical section has been removed. */
+
   if (SCM_STRUCT_DATA (vtable)[scm_struct_i_flags] & SCM_STRUCTF_ENTITY)
     {
       data = scm_alloc_struct (basic_size + tail_elts,
@@ -466,15 +472,7 @@ SCM_DEFINE (scm_make_struct, "make-struct", 2, 0, 1,
   handle = scm_double_cell ((((scm_t_bits) SCM_STRUCT_DATA (vtable))
                             + scm_tc3_struct),
                            (scm_t_bits) data, 0, 0);
-  SCM_CRITICAL_SECTION_END;
 
-  /* In guile 1.8.1 and earlier, the SCM_CRITICAL_SECTION_END above covered
-     also the following scm_struct_init.  But that meant if scm_struct_init
-     finds an invalid type for a "u" field then there's an error throw in a
-     critical section, which results in an abort().  Not sure if we need any
-     protection across scm_struct_init.  The data array contains garbage at
-     this point, but until we return it's not visible to anyone except
-     `gc'.  */
   scm_struct_init (handle, layout, data, tail_elts, init);
 
   return handle;