runtime: fix undead arguments in cgocall

From the garbage collector's perspective, time can move backwards in
cgocall. However, in the midst of this time warp, the pointer
arguments to cgocall can go from dead back to live. If a stack growth
happens while they're dead and then a GC happens when they become live
again, GC can crash with a bad heap pointer.

Specifically, the sequence that leads to a panic is:

1. cgocall calls entersyscall, which saves the PC and SP of its call
site in cgocall. Call this PC/SP "X". At "X" both pointer arguments
are live.

2. cgocall calls asmcgocall. Call the PC/SP of this call "Y". At "Y"
neither pointer argument is live.

3. asmcgocall calls the C code, which eventually calls back into the
Go code.

4. cgocallbackg remembers the saved PC/SP "X" in some local variables,
calls exitsyscall, and then calls cgocallbackg1.

5. The Go code causes a stack growth. This stack unwind sees PC/SP "Y"
in the cgocall frame. Since the arguments are dead at "Y", they are
not adjusted.

6. The Go code returns to cgocallbackg1, which calls reentersyscall
with the recorded saved PC/SP "X", so "X" gets stashed back into
gp.syscallpc/sp.

7. GC scans the stack. It sees there's a saved syscall PC/SP, so it
starts the traceback at PC/SP "X". At "X" the arguments are considered
live, so it scans them, but since they weren't adjusted, the pointers
are bad, so it panics.

This issue started as of commit ca4089ad, when the compiler stopped
marking arguments as live for the whole function.

Since this is a variable liveness issue, fix it by adding KeepAlive
calls that keep the arguments live across this whole time warp.

The existing issue7978 test has all of the infrastructure for testing
this except that it's currently up to chance whether a stack growth
happens in the callback (it currently only happens on the
linux-amd64-noopt builder, for example). Update this test to force a
stack growth, which causes it to fail reliably without this fix.

Fixes #17785.

Change-Id: If706963819ee7814e6705693247bcb97a6f7adb8
Reviewed-on: https://go-review.googlesource.com/33710
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>

2 files changed, 30 insertions, 1 deletions

diff --git a/misc/cgo/test/issue7978.go b/misc/cgo/test/issue7978.go
index e4cbf1d926..7fb62e807b 100644
--- a/misc/cgo/test/issue7978.go
+++ b/misc/cgo/test/issue7978.go
@@ -88,9 +88,20 @@ func issue7978wait(store uint32, wait uint32) {
 
 //export issue7978cb
 func issue7978cb() {
+	// Force a stack growth from the callback to put extra
+	// pressure on the runtime. See issue #17785.
+	growStack(64)
 	issue7978wait(3, 4)
 }
 
+func growStack(n int) int {
+	var buf [128]int
+	if n == 0 {
+		return 0
+	}
+	return buf[growStack(n-1)]
+}
+
 func issue7978go() {
 	C.issue7978c((*C.uint32_t)(&issue7978sync))
 	issue7978wait(7, 8)
diff --git a/src/runtime/cgocall.go b/src/runtime/cgocall.go
index dc4a9a9820..007406b426 100644
--- a/src/runtime/cgocall.go
+++ b/src/runtime/cgocall.go
@@ -120,13 +120,31 @@ func cgocall(fn, arg unsafe.Pointer) int32 {
 	// foreign code.
 	//
 	// The call to asmcgocall is guaranteed not to
-	// split the stack and does not allocate memory,
+	// grow the stack and does not allocate memory,
 	// so it is safe to call while "in a system call", outside
 	// the $GOMAXPROCS accounting.
+	//
+	// fn may call back into Go code, in which case we'll exit the
+	// "system call", run the Go code (which may grow the stack),
+	// and then re-enter the "system call" reusing the PC and SP
+	// saved by entersyscall here.
 	entersyscall(0)
 	errno := asmcgocall(fn, arg)
 	exitsyscall(0)
 
+	// From the garbage collector's perspective, time can move
+	// backwards in the sequence above. If there's a callback into
+	// Go code, GC will see this function at the call to
+	// asmcgocall. When the Go call later returns to C, the
+	// syscall PC/SP is rolled back and the GC sees this function
+	// back at the call to entersyscall. Normally, fn and arg
+	// would be live at entersyscall and dead at asmcgocall, so if
+	// time moved backwards, GC would see these arguments as dead
+	// and then live. Prevent these undead arguments from crashing
+	// GC by forcing them to stay live across this time warp.
+	KeepAlive(fn)
+	KeepAlive(arg)
+
 	endcgo(mp)
 	return errno
 }