bug#13611: SEGV during SMOB GC

bug#13611: SEGV during SMOB GC

[Mike Gran]

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

Hello-

I have a reproducible SEGV during GC of SMOBs on Guile 2.0.7.
It was also present in 2.0.6.


To reproduce compile main.c as 

$ gcc -std=gnu99 -shared -o smobbug.so -Wall -Wextra `pkg-config guile-2.0 --cflags --libs` -fPIC main.c


Then with
$ LD_PRELOAD=./smobbug.so LD_LIBRARY_PATH=. GUILE_LOAD_PATH=. guile

;; At the repl, load the lib

 (use-modules (smobbug))
;; Make a SMOB to be GC'd

 (handlesmob-init)
;; Trigger a GC from the GC thread
 (string-length (make-string 10000000))

This gives

  Program received signal SIGSEGV, Segmentation fault.
  [Switching to Thread 0xb7d98b40 (LWP 20488)]
  0xb7f251ab in smob_mark (addr=0x8608ff0, mark_stack_ptr=0xb7d90308, 
      mark_stack_limit=0xb7d982f0, env=0) at smob.c:325
  325           SCM_I_CURRENT_THREAD->current_mark_stack_ptr = mark_stack_ptr;

Here's what's happening internally.  When Guile starts up, it creates 3
threads
* Initial thread
* GC thread from scm_storage_prehistory GC_INIT()
* signal delivery thread

That second thread is the one from which automatic garbage collection
occurs.  The way that thread gets created, it has an
scm_i_current_thread == NULL, apparently.


So dereferencing scm_i_current_thread causes null dereference.
And smob_mark() will dereference scm_i_current_thread when collecting a
smob with a mark function.

-Mike

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: smobbug.scm --]
[-- Type: text/x-scheme; name="smobbug.scm", Size: 174 bytes --]

(define-module (smobbug)
  #:export ( 
	    handlesmob-init
	    ))

(load-extension "smobbug" "smobbug_init")

(define (handlesmob-init)
  "docstring"
  (%handlesmob-init))

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: main.c --]
[-- Type: text/x-csrc; name="main.c", Size: 1389 bytes --]

#define _GNU_SOURCE
#include <stdio.h>
#include <libguile.h>

static scm_t_bits handlesmob_tag;
void smobbug_init (void);
SCM mark_handle (SCM x);


SCM handlesmob_init ()
{
  SCM s_handlesmob;
  char *handle;

  handle = malloc (1);
  
  return SCM_NEWSMOB (s_handlesmob, handlesmob_tag, handle);
}

SCM
mark_handlesmob (SCM x)
{
  // No SCMs in the handle type: nothing to do here.
  return (SCM_BOOL_F);
}

size_t
free_handlesmob (SCM handle)
{
  SCM_ASSERT (SCM_SMOB_PREDICATE (handlesmob_tag, handle), handle, SCM_ARG1, "free-handlesmob");

  char *m = SCM_SMOB_DATA (handle);

  if (m != NULL)
      free (m);

  return 0;
}

int
print_handlesmob (SCM x, SCM port, scm_print_state *pstate)
{
  char *frm = (char *) SCM_SMOB_DATA (x);
  char *str;

  scm_puts ("#<handlesmob ", port);

  if (frm == (char *) NULL)
    {
      scm_puts ("(freed)", port);
    }
  else
    {
      if (asprintf (&str, "%p", frm) < 0)
	scm_puts ("???", port);
      else
	scm_puts (str, port);
    }

  scm_puts (">", port);
  
  // non-zero means success 
  return 1;
}

void
smobbug_init ()
{
  handlesmob_tag = scm_make_smob_type ("handlesmob", sizeof (char *));
  scm_set_smob_mark (handlesmob_tag, mark_handlesmob);
  scm_set_smob_free (handlesmob_tag, free_handlesmob);
  scm_set_smob_print (handlesmob_tag, print_handlesmob);
  scm_c_define_gsubr ("%handlesmob-init", 0, 0, 0, handlesmob_init);
}

bug#13611: SEGV during SMOB GC

[Ludovic Courtès]

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

Hi Mike,

Mike Gran <spk121@yahoo.com> skribis:

> This gives
>
>   Program received signal SIGSEGV, Segmentation fault.
>   [Switching to Thread 0xb7d98b40 (LWP 20488)]
>   0xb7f251ab in smob_mark (addr=0x8608ff0, mark_stack_ptr=0xb7d90308, 
>       mark_stack_limit=0xb7d982f0, env=0) at smob.c:325
>   325           SCM_I_CURRENT_THREAD->current_mark_stack_ptr = mark_stack_ptr;
>
> Here's what's happening internally.  When Guile starts up, it creates 3
> threads
> * Initial thread
> * GC thread from scm_storage_prehistory GC_INIT()
> * signal delivery thread
>
> That second thread is the one from which automatic garbage collection
> occurs.  The way that thread gets created, it has an
> scm_i_current_thread == NULL, apparently.

Is there any chance that you’re using a GC 7.3 pre-release?

There was a similar report on IRC, and the fix appears to be:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 716 bytes --]

--- a/libguile/smob.c
+++ b/libguile/smob.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 1995,1996,1998,1999,2000,2001, 2003, 2004, 2006, 2009, 2010, 2011, 2012 Free Software Foundation, Inc.
+/* Copyright (C) 1995,1996,1998,1999,2000,2001, 2003, 2004, 2006, 2009, 2010, 2011, 2012, 2013 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
@@ -318,7 +318,7 @@ smob_mark (GC_word *addr, struct GC_ms_entry *mark_stack_ptr,
 				     mark_stack_ptr,
 				     mark_stack_limit, NULL);
 
-  if (scm_smobs[smobnum].mark)
+  if (scm_smobs[smobnum].mark && SCM_I_CURRENT_THREAD != NULL)
     {
       SCM obj;
 

[-- Attachment #3: Type: text/plain, Size: 72 bytes --]


(Note that on 2.0 SMOB mark procedures are unnecessary.)

Ludo’.

bug#13611: SEGV during SMOB GC

[Mike Gran]

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

> From: Ludovic Courtès <ludo@gnu.org>

> Is there any chance that you’re using a GC 7.3 pre-release?

Using gc-7.2b-2.fc17.i686
on Linux 3.6.10-2.fc17.i686 #1 SMP 

> There was a similar report on IRC, and the fix appears to be:

It does fix my SEGV

> (Note that on 2.0 SMOB mark procedures are unnecessary.)

Cool.  Let's yank it from the manual.  Case closed.

Yet...

For what it is worth, I decided to get som statistics on how
often smob_mark is called from a thread with scm_i_current_thread
== NULL vs how often it is called from a thread where it is 
not null.

I wrote the attached patch, and then, using the same little
library as in my initial report, I ran

 (use-modules (smobbug))
 ;; Create a SMOB type
 (handlesmob-init)
 (for-each (lambda (x) (gc))
       (iota 1000))
 (gc-smob-mark-report)

This returned

 Count of GC SMOB marks from null thread: 176
 Count of GC SMOB marks from current thread: 825

Is that expected that GC is sometimes called from a 
thread where scm_i_current_thread is null and sometimes
called from a thread where scm_i_current_thread is
not null?

-Mike 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: smob_gc_mark_report.patch --]
[-- Type: text/x-patch; name="smob_gc_mark_report.patch", Size: 3183 bytes --]

From 807b00d91fa3b7016987ecfd6992e7b7e943d1e3 Mon Sep 17 00:00:00 2001
From: Mike Gran <spk121@yahoo.com>
Date: Tue, 5 Feb 2013 08:18:07 -0800
Subject: [PATCH] Add function to debug smob gc

* libguile/gc.c (scm_gc_smob_mark_report): new report function
* libguile/gc.h: new declaration of scm_gc_smob_mark_report
* libguile/smob.c (smob_mark): gather statistics
---
 libguile/gc.c   | 22 ++++++++++++++++++++++
 libguile/gc.h   |  1 +
 libguile/smob.c |  8 +++++++-
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/libguile/gc.c b/libguile/gc.c
index 06b5044..d06fa6c 100644
--- a/libguile/gc.c
+++ b/libguile/gc.c
@@ -77,6 +77,10 @@ extern unsigned long * __libc_ia64_register_backing_store_base;
 int scm_debug_cell_accesses_p = 0;
 int scm_expensive_debug_cell_accesses_p = 0;
 
+
+extern long scm_mark_from_null;
+extern long scm_mark_from_current;
+
 /* Set this to 0 if no additional gc's shall be performed, otherwise set it to
  * the number of cell accesses after which a gc shall be called.
  */
@@ -383,6 +387,24 @@ SCM_DEFINE (scm_gc_enable, "gc-enable", 0, 0, 0,
 }
 #undef FUNC_NAME
 
+SCM_DEFINE (scm_gc_smob_mark_report, "gc-smob-mark-report", 0, 0, 0,
+	    (),
+	    "Print statistics on gc marking of smobs.")
+#define FUNC_NAME s_scm_gc_smob_mark_report
+{
+  scm_puts ("Count of GC SMOB marks from null thread: ",
+	    scm_current_output_port ());
+  scm_display (scm_from_long (scm_mark_from_null), scm_current_output_port ());
+  scm_newline (scm_current_output_port ());
+  scm_puts ("Count of GC SMOB marks from current thread: ",
+	    scm_current_output_port ());
+  scm_display (scm_from_long (scm_mark_from_current),
+	       scm_current_output_port ());
+  scm_newline (scm_current_output_port ());
+  return SCM_UNSPECIFIED;
+}
+#undef FUNC_NAME
+
 
 SCM_DEFINE (scm_gc, "gc", 0, 0, 0,
            (),
diff --git a/libguile/gc.h b/libguile/gc.h
index 9f00e01..1120aa8 100644
--- a/libguile/gc.h
+++ b/libguile/gc.h
@@ -173,6 +173,7 @@ SCM_API SCM scm_set_debug_cell_accesses_x (SCM flag);
 
 SCM_API SCM scm_object_address (SCM obj);
 SCM_API SCM scm_gc_enable (void);
+SCM_API SCM scm_gc_smob_mark_report (void);
 SCM_API SCM scm_gc_disable (void);
 SCM_API SCM scm_gc_dump (void);
 SCM_API SCM scm_gc_stats (void);
diff --git a/libguile/smob.c b/libguile/smob.c
index c2e8f24..cc8b59a 100644
--- a/libguile/smob.c
+++ b/libguile/smob.c
@@ -52,6 +52,7 @@
 
 long scm_numsmob;
 scm_smob_descriptor scm_smobs[MAX_SMOB_COUNT];
+long scm_mark_from_null = 0, scm_mark_from_current = 0;
 
 void
 scm_assert_smob_type (scm_t_bits tag, SCM val)
@@ -294,6 +295,11 @@ smob_mark (GC_word *addr, struct GC_ms_entry *mark_stack_ptr,
   register SCM cell;
   register scm_t_bits tc, smobnum;
 
+  if (SCM_I_CURRENT_THREAD == NULL)
+    scm_mark_from_null ++;
+  else
+    scm_mark_from_current ++;
+
   cell = PTR2SCM (addr);
 
   if (SCM_TYP7 (cell) != scm_tc7_smob)
@@ -318,7 +324,7 @@ smob_mark (GC_word *addr, struct GC_ms_entry *mark_stack_ptr,
 				     mark_stack_ptr,
 				     mark_stack_limit, NULL);
 
-  if (scm_smobs[smobnum].mark)
+  if (scm_smobs[smobnum].mark && SCM_I_CURRENT_THREAD != NULL)
     {
       SCM obj;
 
-- 
1.7.11.7

bug#13611: SEGV during SMOB GC

[Ludovic Courtès]

Hi,

Mike Gran <spk121@yahoo.com> skribis:

>> From: Ludovic Courtès <ludo@gnu.org>
>
>> Is there any chance that you’re using a GC 7.3 pre-release?
>
> Using gc-7.2b-2.fc17.i686

OK.

>> There was a similar report on IRC, and the fix appears to be:
>
> It does fix my SEGV

Good.

[...]

>  Count of GC SMOB marks from null thread: 176
>  Count of GC SMOB marks from current thread: 825
>
> Is that expected that GC is sometimes called from a 
> thread where scm_i_current_thread is null and sometimes
> called from a thread where scm_i_current_thread is
> not null?

Can you check whether your GC was built with --enable-parallel-mark?

I’m confident that the SMOB mark procedure is never called with null
scm_i_current_thread with 7.2 compiled with the default options (the
GnuTLS bindings rely on this, and I had not seen any such report until
someone tried with GC 7.3pre, which uses the parallel marker by
default.)

Thanks!

Ludo’.

bug#13611: SEGV during SMOB GC

[Mike Gran]

>>  Is that expected that GC is sometimes called from a 

>>  thread where scm_i_current_thread is null and sometimes
>>  called from a thread where scm_i_current_thread is
>>  not null?
> 
> Can you check whether your GC was built with --enable-parallel-mark?
> 
> I’m confident that the SMOB mark procedure is never called with null
> scm_i_current_thread with 7.2 compiled with the default options (the
> GnuTLS bindings rely on this, and I had not seen any such report until
> someone tried with GC 7.3pre, which uses the parallel marker by
> default.)

It looks like fedora gc rpms do use --enable-parallel-mark
for x86 architectures.

You can see that here:
  http://pkgs.fedoraproject.org/cgit/gc.git/tree/gc.spec?h=f17

But it looks like it has been that way for a long time.
Since 2005 at least.

-Mike

bug#13611: SEGV during SMOB GC

[Ludovic Courtès]

Mike Gran <spk121@yahoo.com> skribis:

>>>  Is that expected that GC is sometimes called from a 
>
>>>  thread where scm_i_current_thread is null and sometimes
>>>  called from a thread where scm_i_current_thread is
>>>  not null?
>> 
>> Can you check whether your GC was built with --enable-parallel-mark?
>> 
>> I’m confident that the SMOB mark procedure is never called with null
>> scm_i_current_thread with 7.2 compiled with the default options (the
>> GnuTLS bindings rely on this, and I had not seen any such report until
>> someone tried with GC 7.3pre, which uses the parallel marker by
>> default.)
>
> It looks like fedora gc rpms do use --enable-parallel-mark
> for x86 architectures.

Then that’s the problem.

> But it looks like it has been that way for a long time.
> Since 2005 at least.

And you did not have the problem before?  That part of Guile hasn’t
changed in a long time, I think.

Ludo’.

bug#13611: SEGV during SMOB GC

[Mike Gran]

> From: Ludovic Courtès <ludo@gnu.org>

>>>  I’m confident that the SMOB mark procedure is never called with null
>>>  scm_i_current_thread with 7.2 compiled with the default options (the
>>>  GnuTLS bindings rely on this, and I had not seen any such report until
>>>  someone tried with GC 7.3pre, which uses the parallel marker by
>>>  default.)
>> 
>>  It looks like fedora gc rpms do use --enable-parallel-mark
>>  for x86 architectures.
> 
> Then that’s the problem.
> 
>>  But it looks like it has been that way for a long time.
>>  Since 2005 at least.
> 
> And you did not have the problem before?  That part of Guile hasn’t
> changed in a long time, I think.

I have a different box than before: more cores.

Well, I guess that, for my libraries, I can make a preprocessor conditional
on SCM_MAJOR_VERSION == 2 to eliminate all smob marking for guile-2.x.

Could parallel marking have other, non-SMOB-related, side effects?
I can disable it by setting the envirnomnent variable GC_MARKERS to 1.

Thanks,

Mike

bug#13611: SEGV during SMOB GC

[Ludovic Courtès]

Hi Mike,

AFAICS, commit 01b69e7 fixes the problem for me.  I tested with your
test-smob-mark.c program, both with a 7.2ish and 7.3ish libgc.

Can you confirm that it works for you, and commit your test case with
the changes as discussed on the list?

Thanks!

Ludo’.

bug#13611: SEGV during SMOB GC

[Andy Wingo]

On Fri 01 Mar 2013 18:02, ludo@gnu.org (Ludovic Courtès) writes:

> AFAICS, commit 01b69e7 fixes the problem for me.  I tested with your
> test-smob-mark.c program, both with a 7.2ish and 7.3ish libgc.
>
> Can you confirm that it works for you, and commit your test case with
> the changes as discussed on the list?

Changes committed, closing bug.  Thanks, all!

Andy
-- 
http://wingolog.org/