[Guile-commits] 01/01: Fix race between SMOB marking and finalization

[Andy Wingo]

wingo pushed a commit to branch stable-2.0
in repository guile.

commit 8dff3af087c6eaa83ae0d72aa8b22aef5c65d65d
Author: Andy Wingo <address@hidden>
Date:   Thu Jun 23 11:47:42 2016 +0200

    Fix race between SMOB marking and finalization
    
    * libguile/smob.c (clear_smobnum): New helper.
      (finalize_smob): Re-set the smobnum to the "finalized smob" type
      before finalizing.  Fixes #19883.
      (scm_smob_prehistory): Pre-register a "finalized smob" type, which has
      no mark procedure.
    * test-suite/standalone/test-smob-mark-race.c: New file.
    * test-suite/standalone/Makefile.am: Arrange to build and run the new
      test.
---
 libguile/smob.c                             |   33 ++++++++++++--
 test-suite/standalone/Makefile.am           |    6 +++
 test-suite/standalone/test-smob-mark-race.c |   65 +++++++++++++++++++++++++++
 3 files changed, 101 insertions(+), 3 deletions(-)

diff --git a/libguile/smob.c b/libguile/smob.c
index 90849a8..ed9d91a 100644
--- a/libguile/smob.c
+++ b/libguile/smob.c
@@ -374,20 +374,43 @@ scm_gc_mark (SCM o)
 }
 
 
+static void*
+clear_smobnum (void *ptr)
+{
+  SCM smob;
+  scm_t_bits smobnum;
+
+  smob = PTR2SCM (ptr);
+
+  smobnum = SCM_SMOBNUM (smob);
+  /* Frob the object's type in place, re-setting it to be the "finalized
+     smob" type.  This will prevent other routines from accessing its
+     internals in a way that assumes that the smob data is valid.  This
+     is notably the case for SMOB's own "mark" procedure, if any; as the
+     finalizer runs without the alloc lock, it's possible for a GC to
+     occur while it's running, in which case the object is alive and yet
+     its data is invalid.  */
+  SCM_SET_SMOB_DATA_0 (smob, SCM_SMOB_DATA_0 (smob) & ~(scm_t_bits) 0xff00);
+
+  return (void *) smobnum;
+}
+
 /* Finalize SMOB by calling its SMOB type's free function, if any.  */
 static void
 finalize_smob (void *ptr, void *data)
 {
   SCM smob;
+  scm_t_bits smobnum;
   size_t (* free_smob) (SCM);
 
   smob = PTR2SCM (ptr);
+  smobnum = (scm_t_bits) GC_call_with_alloc_lock (clear_smobnum, ptr);
+
 #if 0
-  printf ("finalizing SMOB %p (smobnum: %u)\n",
-         ptr, SCM_SMOBNUM (smob));
+  printf ("finalizing SMOB %p (smobnum: %u)\n", ptr, smobnum);
 #endif
 
-  free_smob = scm_smobs[SCM_SMOBNUM (smob)].free;
+  free_smob = scm_smobs[smobnum].free;
   if (free_smob)
     free_smob (smob);
 }
@@ -470,6 +493,7 @@ void
 scm_smob_prehistory ()
 {
   long i;
+  scm_t_bits finalized_smob_tc16;
 
   scm_i_pthread_key_create (&current_mark_stack_pointer, NULL);
   scm_i_pthread_key_create (&current_mark_stack_limit, NULL);
@@ -493,6 +517,9 @@ scm_smob_prehistory ()
       scm_smobs[i].apply      = 0;
       scm_smobs[i].apply_trampoline_objcode = SCM_BOOL_F;
     }
+
+  finalized_smob_tc16 = scm_make_smob_type ("finalized smob", 0);
+  if (SCM_TC2SMOBNUM (finalized_smob_tc16) != 0) abort ();
 }
 
 /*
diff --git a/test-suite/standalone/Makefile.am 
b/test-suite/standalone/Makefile.am
index 712418a..aec7418 100644
--- a/test-suite/standalone/Makefile.am
+++ b/test-suite/standalone/Makefile.am
@@ -275,4 +275,10 @@ test_smob_mark_LDADD = $(LIBGUILE_LDADD)
 check_PROGRAMS += test-smob-mark
 TESTS += test-smob-mark
 
+test_smob_mark_race_SOURCES = test-smob-mark-race.c
+test_smob_mark_race_CFLAGS = ${test_cflags}
+test_smob_mark_race_LDADD = $(LIBGUILE_LDADD)
+check_PROGRAMS += test-smob-mark-race
+TESTS += test-smob-mark-race
+
 EXTRA_DIST += ${check_SCRIPTS}
diff --git a/test-suite/standalone/test-smob-mark-race.c 
b/test-suite/standalone/test-smob-mark-race.c
new file mode 100644
index 0000000..eca0325
--- /dev/null
+++ b/test-suite/standalone/test-smob-mark-race.c
@@ -0,0 +1,65 @@
+/* Copyright (C) 2016 Free Software Foundation, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 3 of
+ * the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#if HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#undef NDEBUG
+
+#include <libguile.h>
+#include <assert.h>
+
+static SCM
+mark_smob (SCM smob)
+{
+  assert (SCM_SMOB_DATA (smob) == 1);
+  return SCM_BOOL_F;
+}
+
+static size_t
+finalize_smob (SCM smob)
+{
+  assert (SCM_SMOB_DATA (smob) == 1);
+  SCM_SET_SMOB_DATA (smob, 0);
+  /* Allocate a bit in the hopes of triggering a new GC, making the
+     marker race with the finalizer.  */
+  scm_cons (SCM_BOOL_F, SCM_BOOL_F);
+  return 0;
+}
+
+static void
+tests (void *data, int argc, char **argv)
+{
+  scm_t_bits tc16;
+  int i;
+
+  tc16 = scm_make_smob_type ("smob with finalizer", 0);
+  scm_set_smob_mark (tc16, mark_smob);
+  scm_set_smob_free (tc16, finalize_smob);
+
+  for (i = 0; i < 1000 * 1000; i++)
+    scm_new_smob (tc16, 1);
+}
+
+int
+main (int argc, char *argv[])
+{
+  scm_boot_guile (argc, argv, tests, NULL);
+  return 0;
+}