While testing a libgo patch on Solaris, which does not support
split-stack, I ran across a bug in the handling of caller-saved
registers for the garbage collector.  For non-split-stack systems,
runtime_mcall is responsible for saving all caller-saved registers on
the stack so that the GC stack scan will see them.  It does this by
calling __builtin_unwind_init and setting the g's gcnextsp field to
point to the current stack.  The garbage collector then scans the
stack from gcnextsp to the top of stack.

Unfortunately, the code was setting gcnextsp to point to
runtime_mcall's argument, which meant that even though runtime_mcall
was careful to store all caller-saved registers on the stack, the GC
never saw them.  This is, of course, only a problem if a value lives
only in a caller-saved register, and not anywhere else on the stack or
heap.  And it is only a problem if that caller-saved register manages
to make it all the way down to runtime_mcall without being saved by
any function on the way.  This is moderately unlikely but it turns out
that the recent changes to keep values on the stack when compiling the
runtime package caused it to happen for the local variable `s` in
`notifyListWait` in runtime/sema.go.  That function calls goparkunlock
which is simple enough to not require all registers, and itself calls
runtime_mcall.  So it was possible for `s` to be released by the GC
before the goroutine returned from goparkunlock, which eventually
caused a dangling pointerto be passed to releaseSudog.

This is not a problem on split-stack systems, which use
__splitstack_get_context, which saves a stack pointer low enough on
the stack to scan the registers saved by runtime_mcall.

This patch fixes the problem by introducing a local variable which
should be on the stack below the saved registers.

Bootstrapped and ran Go testsuite on x86_64-pc-linux-gnu and
i386-sun-solaris.  Committed to mainline.

Index: gcc/go/gofrontend/MERGE
--- gcc/go/gofrontend/MERGE     (revision 241261)
+++ gcc/go/gofrontend/MERGE     (working copy)
@@ -1,4 +1,4 @@
 The first line of this file holds the git revision number of the last
 merge done from the gofrontend repository.
Index: libgo/runtime/proc.c
--- libgo/runtime/proc.c        (revision 241261)
+++ libgo/runtime/proc.c        (working copy)
@@ -283,6 +283,9 @@ runtime_mcall(void (*pfn)(G*))
        M *mp;
        G *gp;
+       void *afterregs;
        // Ensure that all registers are on the stack for the garbage
        // collector.
@@ -298,7 +301,9 @@ runtime_mcall(void (*pfn)(G*))
-               gp->gcnextsp = &pfn;
+               // We have to point to an address on the stack that is
+               // below the saved registers.
+               gp->gcnextsp = &afterregs;
                gp->fromgogo = false;

Reply via email to