runtime: fix dangling pointer in readyExecute

readyExecute passes a closure to mcall that captures an argument to
readyExecute. Since mcall is marked noescape, this closure lives on
the stack of the calling goroutine. However, the closure puts the
calling goroutine on the run queue (and switches to a new
goroutine). If the calling goroutine gets scheduled before the mcall
returns, this stack-allocated closure will become invalid while it's
still executing. One consequence of this we've observed is that the
captured gp variable can get overwritten before the call to
execute(gp), causing execute(gp) to segfault.

Fix this by passing the currently captured gp variable through a field
in the calling goroutine's g struct so that the func is no longer a
closure.

To prevent problems like this in the future, this change also removes
the go:noescape annotation from mcall. Due to a compiler bug, this
will currently cause a func closure passed to mcall to be implicitly
allocated rather than refusing the implicit allocation. However, this
is okay because there are no other closures passed to mcall right now
and the compiler bug will be fixed shortly.

Fixes #10428.

Change-Id: I49b48b85de5643323b89e9eaa4df63854e968c32
Reviewed-on: https://go-review.googlesource.com/8866
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>

diff --git a/src/runtime/proc1.go b/src/runtime/proc1.go
index e3565a6d3356ec9069510e0837cc790d214e786a..2786e7e441f7cb77b52553ff929af6281e2ef007 100644 (file)
--- a/src/runtime/proc1.go
+++ b/src/runtime/proc1.go
@@ -158,7 +158,15 @@ func ready(gp *g, traceskip int) {
 // readyExecute marks gp ready to run, preempt the current g, and execute gp.
 // This is used to start concurrent GC promptly when we reach its trigger.
 func readyExecute(gp *g, traceskip int) {
+       // Squirrel away gp so we don't allocate a closure for the
+       // mcall'd func below. If we allocate a closure, it could go
+       // away as soon as we put _g_ on the runqueue.
+       getg().readyg = gp
+
        mcall(func(_g_ *g) {
+               gp := _g_.readyg
+               _g_.readyg = nil
+
                if trace.enabled {
                        traceGoUnpark(gp, traceskip)
                        traceGoSched()

diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go
index 609c7cf6f67012e8fa44b3e0a7ac2bec46c95412..3230d4e1a8b9ece64dfabf0b46a5db6b0462d41b 100644 (file)
--- a/src/runtime/runtime2.go
+++ b/src/runtime/runtime2.go
@@ -243,6 +243,7 @@ type g struct {
        startpc      uintptr // pc of goroutine function
        racectx      uintptr
        waiting      *sudog // sudog structures this g is waiting on (that have a valid elem ptr)
+       readyg       *g     // scratch for readyExecute
 }
 
 type mts struct {

diff --git a/src/runtime/stubs.go b/src/runtime/stubs.go
index 7b6fbb0349da2b6aa0406a7fa669247940b1a048..50e2a207dad2ee1b91333df69b9d495cba76f553 100644 (file)
--- a/src/runtime/stubs.go
+++ b/src/runtime/stubs.go
@@ -33,7 +33,10 @@ func getg() *g
 // run other goroutines.
 //
 // mcall can only be called from g stacks (not g0, not gsignal).
-//go:noescape
+//
+// This must NOT be go:noescape: if fn is a stack-allocated closure,
+// fn puts g on a run queue, and g executes before fn returns, the
+// closure will be invalidated while it is still executing.
 func mcall(fn func(*g))
 
 // systemstack runs fn on a system stack.