]> Cypherpunks repositories - gostls13.git/commitdiff
[release-branch.go1.3] runtime: keep g->syscallsp consistent after cgo->Go callbacks
authorRuss Cox <rsc@golang.org>
Thu, 25 Sep 2014 22:06:45 +0000 (08:06 +1000)
committerAndrew Gerrand <adg@golang.org>
Thu, 25 Sep 2014 22:06:45 +0000 (08:06 +1000)
This is a manual backport of CL 131910043
to the Go 1.3 release branch.

We believe this CL can cause arbitrary corruption
in programs that call into C from Go and then
call back into Go from C.

This change will be released in Go 1.3.2.

LGTM=iant
R=iant, hector
CC=adg, dvyukov, golang-codereviews, r
https://golang.org/cl/142690043

misc/cgo/test/cgo_test.go
misc/cgo/test/issue7978.go [new file with mode: 0644]
src/pkg/runtime/cgocall.c
src/pkg/runtime/proc.c
src/pkg/runtime/runtime.h
src/run.bash
src/run.bat

index eb237725a4f4e49485700c3c30c55ebc5a0b95d9..e2e5a2bc1482ff0adbf6f0b399845be506212dca 100644 (file)
@@ -53,5 +53,6 @@ func Test5986(t *testing.T)                { test5986(t) }
 func Test7665(t *testing.T)                { test7665(t) }
 func TestNaming(t *testing.T)              { testNaming(t) }
 func Test7560(t *testing.T)                { test7560(t) }
+func Test7978(t *testing.T)                { test7978(t) }
 
 func BenchmarkCgoCall(b *testing.B) { benchCgoCall(b) }
diff --git a/misc/cgo/test/issue7978.go b/misc/cgo/test/issue7978.go
new file mode 100644 (file)
index 0000000..432e006
--- /dev/null
@@ -0,0 +1,103 @@
+// Copyright 2014 The Go Authors.  All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Issue 7978.  Stack tracing didn't work during cgo code after calling a Go
+// callback.  Make sure GC works and the stack trace is correct.
+
+package cgotest
+
+/*
+#include <stdint.h>
+
+void issue7978cb(void);
+
+// use ugly atomic variable sync since that doesn't require calling back into
+// Go code or OS dependencies
+static void issue7978c(uint32_t *sync) {
+       while(__sync_fetch_and_add(sync, 0) != 0)
+               ;
+       __sync_fetch_and_add(sync, 1);
+       while(__sync_fetch_and_add(sync, 0) != 2)
+               ;
+       issue7978cb();
+       __sync_fetch_and_add(sync, 1);
+       while(__sync_fetch_and_add(sync, 0) != 6)
+               ;
+}
+*/
+import "C"
+
+import (
+       "os"
+       "runtime"
+       "strings"
+       "sync/atomic"
+       "testing"
+)
+
+var issue7978sync uint32
+
+func issue7978check(t *testing.T, wantFunc string, badFunc string, depth int) {
+       runtime.GC()
+       buf := make([]byte, 65536)
+       trace := string(buf[:runtime.Stack(buf, true)])
+       for _, goroutine := range strings.Split(trace, "\n\n") {
+               if strings.Contains(goroutine, "test.issue7978go") {
+                       trace := strings.Split(goroutine, "\n")
+                       // look for the expected function in the stack
+                       for i := 0; i < depth; i++ {
+                               if badFunc != "" && strings.Contains(trace[1+2*i], badFunc) {
+                                       t.Errorf("bad stack: found %s in the stack:\n%s", badFunc, goroutine)
+                                       return
+                               }
+                               if strings.Contains(trace[1+2*i], wantFunc) {
+                                       return
+                               }
+                       }
+                       t.Errorf("bad stack: didn't find %s in the stack:\n%s", wantFunc, goroutine)
+                       return
+               }
+       }
+       t.Errorf("bad stack: goroutine not found. Full stack dump:\n%s", trace)
+}
+
+func issue7978wait(store uint32, wait uint32) {
+       if store != 0 {
+               atomic.StoreUint32(&issue7978sync, store)
+       }
+       for atomic.LoadUint32(&issue7978sync) != wait {
+               runtime.Gosched()
+       }
+}
+
+//export issue7978cb
+func issue7978cb() {
+       issue7978wait(3, 4)
+}
+
+func issue7978go() {
+       C.issue7978c((*C.uint32_t)(&issue7978sync))
+       issue7978wait(7, 8)
+}
+
+func test7978(t *testing.T) {
+       if os.Getenv("GOTRACEBACK") != "2" {
+               t.Fatal("GOTRACEBACK must be 2")
+       }
+       issue7978sync = 0
+       go issue7978go()
+       // test in c code, before callback
+       issue7978wait(0, 1)
+       issue7978check(t, "runtime.cgocall(", "", 1)
+       // test in go code, during callback
+       issue7978wait(2, 3)
+       issue7978check(t, "test.issue7978cb(", "test.issue7978go", 4)
+       // test in c code, after callback
+       issue7978wait(4, 5)
+       issue7978check(t, "runtime.cgocall(", "runtime.cgocallback", 1)
+       // test in go code, after return from cgo
+       issue7978wait(6, 7)
+       issue7978check(t, "test.issue7978go(", "", 4)
+       atomic.StoreUint32(&issue7978sync, 8)
+}
index 7b2ec26f3c78b2d8ffb21a8d96cae1f80f6a5130..75d3850ef7c530718c3008bdb33fdf8007556cbd 100644 (file)
@@ -234,14 +234,18 @@ void runtime·cgocallbackg1(void);
 void
 runtime·cgocallbackg(void)
 {
+       uintptr pc, sp;
+
        if(g != m->curg) {
                runtime·prints("runtime: bad g in cgocallback");
                runtime·exit(2);
        }
 
+       pc = g->syscallpc;
+       sp = g->syscallsp;
        runtime·exitsyscall(); // coming out of cgo call
        runtime·cgocallbackg1();
-       runtime·entersyscall();        // going back to cgo call
+       runtime·reentersyscall((void*)pc, sp); // going back to cgo call
 }
 
 void
index 914a02e0bf5492cd9907c5c1b235aea779f2a8e1..de4f70153dcf4f3a87eb76b5156865d3e64e16e7 100644 (file)
@@ -1499,14 +1499,14 @@ save(void *pc, uintptr sp)
 // entersyscall is going to return immediately after.
 #pragma textflag NOSPLIT
 void
-·entersyscall(int32 dummy)
+runtime·reentersyscall(void *pc, uintptr sp)
 {
        // Disable preemption because during this function g is in Gsyscall status,
        // but can have inconsistent g->sched, do not let GC observe it.
        m->locks++;
 
        // Leave SP around for GC and traceback.
-       save(runtime·getcallerpc(&dummy), runtime·getcallersp(&dummy));
+       save(pc, sp);
        g->syscallsp = g->sched.sp;
        g->syscallpc = g->sched.pc;
        g->syscallstack = g->stackbase;
@@ -1525,7 +1525,7 @@ void
                        runtime·notewakeup(&runtime·sched.sysmonnote);
                }
                runtime·unlock(&runtime·sched);
-               save(runtime·getcallerpc(&dummy), runtime·getcallersp(&dummy));
+               save(pc, sp);
        }
 
        m->mcache = nil;
@@ -1538,7 +1538,7 @@ void
                                runtime·notewakeup(&runtime·sched.stopnote);
                }
                runtime·unlock(&runtime·sched);
-               save(runtime·getcallerpc(&dummy), runtime·getcallersp(&dummy));
+               save(pc, sp);
        }
 
        // Goroutines must not split stacks in Gsyscall status (it would corrupt g->sched).
@@ -1548,6 +1548,13 @@ void
        m->locks--;
 }
 
+#pragma textflag NOSPLIT
+void
+·entersyscall(int32 dummy)
+{
+       runtime·reentersyscall(runtime·getcallerpc(&dummy), runtime·getcallersp(&dummy));
+}
+
 // The same as runtime·entersyscall(), but with a hint that the syscall is blocking.
 #pragma textflag NOSPLIT
 void
@@ -1588,10 +1595,13 @@ void
 // from the low-level system calls used by the runtime.
 #pragma textflag NOSPLIT
 void
-runtime·exitsyscall(void)
+·exitsyscall(int32 dummy)
 {
        m->locks++;  // see comment in entersyscall
 
+       if(runtime·getcallersp(&dummy) > g->syscallsp)
+               runtime·throw("exitsyscall: syscall frame is no longer valid");
+
        if(g->isbackground)  // do not consider blocked scavenger for deadlock detection
                incidlelocked(-1);
 
index 5115503789be8bf7cf906e33263c8c7c1dddf0f8..42fb3a47d0a45d3c4ae989e6810954cad54a33f4 100644 (file)
@@ -921,6 +921,7 @@ M*  runtime·newm(void);
 void   runtime·goexit(void);
 void   runtime·asmcgocall(void (*fn)(void*), void*);
 void   runtime·entersyscall(void);
+void   runtime·reentersyscall(void*, uintptr);
 void   runtime·entersyscallblock(void);
 void   runtime·exitsyscall(void);
 G*     runtime·newproc1(FuncVal*, byte*, int32, int32, void*);
index 6eec7caa4040e944510dcafc7a1246d833075a5e..8590ea8d49f561ced2e6e8ac51a821df98af870b 100755 (executable)
@@ -112,6 +112,8 @@ go run $GOROOT/test/run.go - . || exit 1
 
 [ "$CGO_ENABLED" != 1 ] ||
 (xcd ../misc/cgo/test
+# cgo tests inspect the traceback for runtime functions
+export GOTRACEBACK=2
 go test -ldflags '-linkmode=auto' || exit 1
 # linkmode=internal fails on dragonfly since errno is a TLS relocation.
 [ "$GOHOSTOS" == dragonfly ] || go test -ldflags '-linkmode=internal' || exit 1
index 62692acaf2691972693a0ed89fd89b35fb4143f0..14c1b45fdad40a09fd2bdd837c9c41db603ddee8 100644 (file)
@@ -90,11 +90,18 @@ go run "%GOROOT%\test\run.go" - ..\misc\cgo\stdio
 if errorlevel 1 goto fail
 echo.
 
+:: cgo tests inspect the traceback for runtime functions
+set OLDGOTRACEBACK=%GOTRACEBACK%
+set GOTRACEBACK=2
+
 echo # ..\misc\cgo\test
 go test ..\misc\cgo\test
 if errorlevel 1 goto fail
 echo.
 
+set GOTRACEBACK=%OLDGOTRACEBACK%
+set OLDGOTRACEBACK=
+
 echo # ..\misc\cgo\testso
 cd ..\misc\cgo\testso
 set FAIL=0