From c1c667542cb831303d332f3699a9cf32dfa490e1 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Mon, 13 Apr 2015 16:50:20 -0400 Subject: [PATCH] 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 Reviewed-by: David Chase Reviewed-by: Russ Cox --- src/runtime/proc1.go | 8 ++++++++ src/runtime/runtime2.go | 1 + src/runtime/stubs.go | 5 ++++- 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/runtime/proc1.go b/src/runtime/proc1.go index e3565a6d33..2786e7e441 100644 --- 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 609c7cf6f6..3230d4e1a8 100644 --- 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 7b6fbb0349..50e2a207da 100644 --- 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. -- 2.50.0