runtime: keep g->syscallsp consistent after cgo->Go callbacks

Normally, the caller to runtime.entersyscall() must not return before
calling runtime.exitsyscall(), lest g->syscallsp become a dangling
pointer. runtime.cgocallbackg() violates this constraint. To work around
this, save g->syscallsp and g->syscallpc around cgo->Go callbacks, then
restore them after calling runtime.entersyscall(), which restores the
syscall stack frame pointer saved by cgocall. This allows the GC to
correctly trace a goroutine that is currently returning from a
Go->cgo->Go chain.

This also adds a check to proc.c that panics if g->syscallsp is clearly
invalid. It is not 100% foolproof, as it will not catch a case where the
stack was popped then pushed back beyond g->syscallsp, but it does catch
the present cgo issue and makes existing tests fail without the bugfix.

Fixes issue 7978.

LGTM=dvyukov, rsc
R=golang-codereviews, dvyukov, minux, bradfitz, iant, gobot, rsc
CC=golang-codereviews, rsc
https://codereview.appspot.com/131910043

Committer: Russ Cox <rsc@golang.org>

6 files changed, 49 insertions, 13 deletions

diff --git a/src/run.bash b/src/run.bash
index b5f061d88..d6e53304d 100755
--- a/src/run.bash
+++ b/src/run.bash
@@ -119,6 +119,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
diff --git a/src/run.bat b/src/run.bat
index 62692acaf..309e06d50 100644
--- a/src/run.bat
+++ b/src/run.bat
@@ -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
diff --git a/src/runtime/cgocall.go b/src/runtime/cgocall.go
index a21474b01..7fd91469e 100644
--- a/src/runtime/cgocall.go
+++ b/src/runtime/cgocall.go
@@ -177,14 +177,22 @@ func cfree(p unsafe.Pointer) {
 // Call from C back to Go.
 //go:nosplit
 func cgocallbackg() {
-	if gp := getg(); gp != gp.m.curg {
+	gp := getg()
+	if gp != gp.m.curg {
 		println("runtime: bad g in cgocallback")
 		exit(2)
 	}
 
+	// entersyscall saves the caller's SP to allow the GC to trace the Go
+	// stack. However, since we're returning to an earlier stack frame and
+	// need to pair with the entersyscall() call made by cgocall, we must
+	// save syscall* and let reentersyscall restore them.
+	savedsp := unsafe.Pointer(gp.syscallsp)
+	savedpc := gp.syscallpc
 	exitsyscall() // coming out of cgo call
 	cgocallbackg1()
-	entersyscall() // going back to cgo call
+	// going back to cgo call
+	reentersyscall(savedpc, savedsp)
 }
 
 func cgocallbackg1() {
diff --git a/src/runtime/proc.c b/src/runtime/proc.c
index 3f4179d47..564798be7 100644
--- a/src/runtime/proc.c
+++ b/src/runtime/proc.c
@@ -1700,9 +1700,9 @@ goexit0(G *gp)
 
 #pragma textflag NOSPLIT
 static void
-save(void *pc, uintptr sp)
+save(uintptr pc, uintptr sp)
 {
-	g->sched.pc = (uintptr)pc;
+	g->sched.pc = pc;
 	g->sched.sp = sp;
 	g->sched.lr = 0;
 	g->sched.ret = 0;
@@ -1730,9 +1730,15 @@ static void entersyscall_gcwait(void);
 // In practice, this means that we make the fast path run through
 // entersyscall doing no-split things, and the slow path has to use onM
 // to run bigger things on the m stack.
+//
+// reentersyscall is the entry point used by cgo callbacks, where explicitly
+// saved SP and PC are restored. This is needed when exitsyscall will be called
+// from a function further up in the call stack than the parent, as g->syscallsp
+// must always point to a valid stack frame. entersyscall below is the normal
+// entry point for syscalls, which obtains the SP and PC from the caller.
 #pragma textflag NOSPLIT
 void
-·entersyscall(int32 dummy)
+runtime·reentersyscall(uintptr pc, uintptr sp)
 {
 	void (*fn)(void);
 
@@ -1748,9 +1754,9 @@ void
 	g->throwsplit = 1;
 
 	// Leave SP around for GC and traceback.
-	save(runtime·getcallerpc(&dummy), runtime·getcallersp(&dummy));
-	g->syscallsp = g->sched.sp;
-	g->syscallpc = g->sched.pc;
+	save(pc, sp);
+	g->syscallsp = sp;
+	g->syscallpc = pc;
 	runtime·casgstatus(g, Grunning, Gsyscall);
 	if(g->syscallsp < g->stack.lo || g->stack.hi < g->syscallsp) {
 		fn = entersyscall_bad;
@@ -1760,7 +1766,7 @@ void
 	if(runtime·atomicload(&runtime·sched.sysmonwait)) {  // TODO: fast atomic
 		fn = entersyscall_sysmon;
 		runtime·onM(&fn);
-		save(runtime·getcallerpc(&dummy), runtime·getcallersp(&dummy));
+		save(pc, sp);
 	}
 
 	g->m->mcache = nil;
@@ -1769,7 +1775,7 @@ void
 	if(runtime·sched.gcwaiting) {
 		fn = entersyscall_gcwait;
 		runtime·onM(&fn);
-		save(runtime·getcallerpc(&dummy), runtime·getcallersp(&dummy));
+		save(pc, sp);
 	}
 
 	// Goroutines must not split stacks in Gsyscall status (it would corrupt g->sched).
@@ -1779,6 +1785,14 @@ void
 	g->m->locks--;
 }
 
+// Standard syscall entry used by the go syscall library and normal cgo calls.
+#pragma textflag NOSPLIT
+void
+·entersyscall(int32 dummy)
+{
+	runtime·reentersyscall((uintptr)runtime·getcallerpc(&dummy), runtime·getcallersp(&dummy));
+}
+
 static void
 entersyscall_bad(void)
 {
@@ -1826,7 +1840,7 @@ void
 	g->stackguard0 = StackPreempt;  // see comment in entersyscall
 
 	// Leave SP around for GC and traceback.
-	save(runtime·getcallerpc(&dummy), runtime·getcallersp(&dummy));
+	save((uintptr)runtime·getcallerpc(&dummy), runtime·getcallersp(&dummy));
 	g->syscallsp = g->sched.sp;
 	g->syscallpc = g->sched.pc;
 	runtime·casgstatus(g, Grunning, Gsyscall);
@@ -1839,7 +1853,7 @@ void
 	runtime·onM(&fn);
 
 	// Resave for traceback during blocked call.
-	save(runtime·getcallerpc(&dummy), runtime·getcallersp(&dummy));
+	save((uintptr)runtime·getcallerpc(&dummy), runtime·getcallersp(&dummy));
 
 	g->m->locks--;
 }
@@ -1856,12 +1870,15 @@ entersyscallblock_handoff(void)
 // from the low-level system calls used by the runtime.
 #pragma textflag NOSPLIT
 void
-runtime·exitsyscall(void)
+·exitsyscall(int32 dummy)
 {
 	void (*fn)(G*);
 
 	g->m->locks++;  // see comment in entersyscall
 
+	if(runtime·getcallersp(&dummy) > g->syscallsp)
+		runtime·throw("exitsyscall: syscall frame is no longer valid");
+
 	g->waitsince = 0;
 	if(exitsyscallfast()) {
 		// There's a cpu for us, so we can run.
diff --git a/src/runtime/runtime.h b/src/runtime/runtime.h
index 7fefbc299..3a6d3e326 100644
--- a/src/runtime/runtime.h
+++ b/src/runtime/runtime.h
@@ -901,6 +901,7 @@ void	runtime·goexit(void);
 void	runtime·asmcgocall(void (*fn)(void*), void*);
 int32	runtime·asmcgocall_errno(void (*fn)(void*), void*);
 void	runtime·entersyscall(void);
+void	runtime·reentersyscall(uintptr, uintptr);
 void	runtime·entersyscallblock(void);
 void	runtime·exitsyscall(void);
 G*	runtime·newproc1(FuncVal*, byte*, int32, int32, void*);
diff --git a/src/runtime/stubs.go b/src/runtime/stubs.go
index 2e6aadca7..1381c7efd 100644
--- a/src/runtime/stubs.go
+++ b/src/runtime/stubs.go
@@ -164,6 +164,7 @@ func noescape(p unsafe.Pointer) unsafe.Pointer {
 }
 
 func entersyscall()
+func reentersyscall(pc uintptr, sp unsafe.Pointer)
 func entersyscallblock()
 func exitsyscall()