[PATCH] fix for Re: crash in gc with upside-down stack

[PATCH] fix for Re: crash in gc with upside-down stack

[Linas Vepstas]

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

Patch below; I'm also attaching the same patch, in case
gmail is scrambling this thing :-/  Also, I've long had a
generic assignment on file with the FSF.

--linas

The patch below fixes a crash during garbage collection, where, during
the mark-stack phase, the top and bottom of the stack are found to be
in backwards order, typically because scm_with_guile() was called when
the stack is much shorter than when a thread was first guilified. That
is, the stack base pointer is stale, and can be inverted from the stack
top. If GC runs due to activity in some other thread, the stale base
pointer leads to the crash (as base-top is approximately 2^32 or 2^64).

A typical symptom of this bug, on a 32-bit system, is:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xf5333b90 (LWP 20587)]
0xf7711ce3 in scm_mark_locations (x=0xf5333110, n=4294966782) at gc-mark.c:435
435           SCM obj = * (SCM *) &x[m];
Current language:  auto; currently c
(gdb) bt
#0  0xf7711ce3 in scm_mark_locations (x=0xf5333110, n=4294966782) at
gc-mark.c:435
#1  0xf7766a12 in scm_threads_mark_stacks () at threads.c:1375

Notice that 4294966782 == fffffdfe == -202

Please apply in time for guile-1.8.6!

Signed-off-by: Linas Vepstas <linasvepstas@gmail.com>

---
 libguile/threads.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

Index: guile-1.8.5/libguile/threads.c
===================================================================
--- guile-1.8.5.orig/libguile/threads.c	2008-11-13 15:17:12.000000000 -0600
+++ guile-1.8.5/libguile/threads.c	2008-11-13 15:32:07.000000000 -0600
@@ -577,9 +577,24 @@ scm_i_init_thread_for_guile (SCM_STACKIT
       /* This thread is already guilified but not in guile mode, just
 	 resume it.
 	
-	 XXX - base might be lower than when this thread was first
-	 guilified.
+         A user call to scm_with_guile() will lead us to here. This
+         could happen anywhere on the stack, and in particular, the
+         stack can be *much* shorter than what it was when this thread
+         was first guilified. This will typically happen in
+         on_thread_exit(), where the stack is *always* shorter than
+         when the thread was first guilified. If the GC happens to
+         get triggered due to some other thread, we'd end up with
+         t->top "upside-down" w.r.t. t->base, which will result in
+         chaos in scm_threads_mark_stacks() when top-base=2^32 or 2^64.
+         Thus, reset the base, if needed.
        */
+#if SCM_STACK_GROWS_UP
+      if (base < t->base)
+         t->base = base;
+#else
+      if (base > t->base)
+         t->base = base;
+#endif
       scm_enter_guile ((scm_t_guile_ticket) t);
       return 1;
     }

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

The patch below fixes a crash during garbage collection, where, during
the mark-stack phase, the top and bottom of the stack are found to be 
in backwards order, typically because scm_with_guile() was called when
the stack is much shorter than when a thread was first guilified. That
is, the stack base pointer is stale, and can be inverted from the stack
top. If GC runs due to activity in some other thread, the stale base
pointer leads to the crash (as base-top is approximately 2^32 or 2^64).

A typical symptom of this bug, on a 32-bit system, is:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xf5333b90 (LWP 20587)]
0xf7711ce3 in scm_mark_locations (x=0xf5333110, n=4294966782) at gc-mark.c:435
435           SCM obj = * (SCM *) &x[m];
Current language:  auto; currently c
(gdb) bt
#0  0xf7711ce3 in scm_mark_locations (x=0xf5333110, n=4294966782) at gc-mark.c:435
#1  0xf7766a12 in scm_threads_mark_stacks () at threads.c:1375

Notice that 4294966782 == fffffdfe == -202

Please apply in time for guile-1.8.6!

Signed-off-by: Linas Vepstas <linasvepstas@gmail.com>

---
 libguile/threads.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

Index: guile-1.8.5/libguile/threads.c
===================================================================
--- guile-1.8.5.orig/libguile/threads.c	2008-11-13 15:17:12.000000000 -0600
+++ guile-1.8.5/libguile/threads.c	2008-11-13 15:32:07.000000000 -0600
@@ -577,9 +577,24 @@ scm_i_init_thread_for_guile (SCM_STACKIT
       /* This thread is already guilified but not in guile mode, just
 	 resume it.
 	 
-	 XXX - base might be lower than when this thread was first
-	 guilified.
+         A user call to scm_with_guile() will lead us to here. This
+         could happen anywhere on the stack, and in particular, the
+         stack can be *much* shorter than what it was when this thread
+         was first guilified. This will typically happen in
+         on_thread_exit(), where the stack is *always* shorter than
+         when the thread was first guilified. If the GC happens to
+         get triggered due to some other thread, we'd end up with
+         t->top "upside-down" w.r.t. t->base, which will result in
+         chaos in scm_threads_mark_stacks() when top-base=2^32 or 2^64.
+         Thus, reset the base, if needed.
        */
+#if SCM_STACK_GROWS_UP
+      if (base < t->base)
+         t->base = base;
+#else
+      if (base > t->base)
+         t->base = base;
+#endif
       scm_enter_guile ((scm_t_guile_ticket) t);
       return 1;
     }

Re: [PATCH] fix for Re: crash in gc with upside-down stack

[Ludovic Courtès]

Hi,

"Linas Vepstas" <linasvepstas@gmail.com> writes:

> The patch below fixes a crash during garbage collection, where, during
> the mark-stack phase, the top and bottom of the stack are found to be
> in backwards order, typically because scm_with_guile() was called when
> the stack is much shorter than when a thread was first guilified. That
> is, the stack base pointer is stale, and can be inverted from the stack
> top. If GC runs due to activity in some other thread, the stale base
> pointer leads to the crash (as base-top is approximately 2^32 or 2^64).

Good catch!  I applied it, along with a test case that reproduced the
problem:

  http://git.savannah.gnu.org/gitweb/?p=guile.git;a=commitdiff;h=cd1a1e47b5e781277560d9933a44e6aabd0c9c49

Thanks!

Ludo'.

Re: [PATCH] fix for Re: crash in gc with upside-down stack

[Neil Jerram]

2008/11/13 Ludovic Courtès <ludo@gnu.org>:
> Hi,
>
> "Linas Vepstas" <linasvepstas@gmail.com> writes:
>
>> The patch below fixes a crash during garbage collection, where, during
>> the mark-stack phase, the top and bottom of the stack are found to be
>> in backwards order, typically because scm_with_guile() was called when
>> the stack is much shorter than when a thread was first guilified. That
>> is, the stack base pointer is stale, and can be inverted from the stack
>> top. If GC runs due to activity in some other thread, the stale base
>> pointer leads to the crash (as base-top is approximately 2^32 or 2^64).
>
> Good catch!  I applied it, along with a test case that reproduced the
> problem:
>
>  http://git.savannah.gnu.org/gitweb/?p=guile.git;a=commitdiff;h=cd1a1e47b5e781277560d9933a44e6aabd0c9c49

Yes indeed.  Nice work Linas, and nice test Ludovic.

    Neil