[release-branch.go1.14] runtime: fix code so defer record is not added to g0 defer...

newdefer() actually adds the new defer to the current g's defer chain. That
happens even if we are on the system stack, in which case the g will be the g0
stack. For open-coded defers, we call newdefer() (only during panic processing)
while on the system stack, so the new defer is unintentionally added to the
g0._defer defer list. The code later correctly adds the defer to the user g's
defer list.

The g0._defer list is never used. However, that pointer on the g0._defer list can
keep a defer struct alive that is intended to be garbage-collected (smaller defers
use a defer pool, but larger-sized defer records are just GC'ed). freedefer() does
not zero out pointers when it intends that a defer become garbage-collected. So,
we can have the pointers in a defer that is held alive by g0._defer become invalid
(in particular d.link). This is the cause of the bad pointer bug in this issue

The fix is to change newdefer (only used in two places) to not add the new defer
to the gp._defer list. We just do it after the call with the correct gp pointer.
(As mentioned above, this code was already there after the newdefer in
addOneOpenDeferFrame.) That ensures that defers will be correctly
garbage-collected and eliminate the bad pointer.

This fix definitely fixes the original repro. I added a test and tried hard to
reproduce the bug (based on the original repro code), but awasn't actually able to
cause the bug. However, the test is still an interesting mix of heap-allocated,
stack-allocated, and open-coded defers.

For #37688
Fixes #37968

Fixes #37688

Change-Id: I1a481b9d9e9b9ba4e8726ef718a1f4512a2d6faf
Reviewed-on: https://go-review.googlesource.com/c/go/+/224581
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
(cherry picked from commit 825ae71e567593d3a28b7dddede8745701273c52)
Reviewed-on: https://go-review.googlesource.com/c/go/+/225279
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Dan Scales <danscales@google.com>

diff --git a/src/runtime/defer_test.go b/src/runtime/defer_test.go
index f35535e773de614c80bf9a46f2cba03b342cfc67..11436a1f08be1668d71adaad034c26f20dad85ae 100644 (file)
--- a/src/runtime/defer_test.go
+++ b/src/runtime/defer_test.go
@@ -335,3 +335,69 @@ func recurseFn(level int, maxlevel int) {
                panic("recurseFn panic")
        }
 }
+
+// Try to reproduce issue #37688, where a pointer to an open-coded defer struct is
+// mistakenly held, and that struct keeps a pointer to a stack-allocated defer
+// struct, and that stack-allocated struct gets overwritten or the stack gets
+// moved, so a memory error happens on GC.
+func TestIssue37688(t *testing.T) {
+       for j := 0; j < 10; j++ {
+               g2()
+               g3()
+       }
+}
+
+type foo struct {
+}
+
+func (f *foo) method1() {
+       fmt.Fprintln(os.Stderr, "method1")
+}
+
+func (f *foo) method2() {
+       fmt.Fprintln(os.Stderr, "method2")
+}
+
+func g2() {
+       var a foo
+       ap := &a
+       // The loop forces this defer to be heap-allocated and the remaining two
+       // to be stack-allocated.
+       for i := 0; i < 1; i++ {
+               defer ap.method1()
+       }
+       defer ap.method2()
+       defer ap.method1()
+       ff1(ap, 1, 2, 3, 4, 5, 6, 7, 8, 9)
+       // Try to get the stack to be be moved by growing it too large, so
+       // existing stack-allocated defer becomes invalid.
+       rec1(2000)
+}
+
+func g3() {
+       // Mix up the stack layout by adding in an extra function frame
+       g2()
+}
+
+func ff1(ap *foo, a, b, c, d, e, f, g, h, i int) {
+       defer ap.method1()
+
+       // Make a defer that has a very large set of args, hence big size for the
+       // defer record for the open-coded frame (which means it won't use the
+       // defer pool)
+       defer func(ap *foo, a, b, c, d, e, f, g, h, i int) {
+               if v := recover(); v != nil {
+                       fmt.Fprintln(os.Stderr, "did recover")
+               }
+               fmt.Fprintln(os.Stderr, "debug", ap, a, b, c, d, e, f, g, h)
+       }(ap, a, b, c, d, e, f, g, h, i)
+       panic("ff1 panic")
+}
+
+func rec1(max int) {
+       if max > 0 {
+               rec1(max - 1)
+       } else {
+               fmt.Fprintln(os.Stderr, "finished recursion", max)
+       }
+}

diff --git a/src/runtime/panic.go b/src/runtime/panic.go
index 28b5cbefcc8887a2fd828536edc4fcf2115f165b..615249f33cdac0b42b917d180880dece395cab63 100644 (file)
--- a/src/runtime/panic.go
+++ b/src/runtime/panic.go
@@ -216,7 +216,8 @@ func panicmem() {
 // The compiler turns a defer statement into a call to this.
 //go:nosplit
 func deferproc(siz int32, fn *funcval) { // arguments of fn follow fn
-       if getg().m.curg != getg() {
+       gp := getg()
+       if gp.m.curg != gp {
                // go code on the system stack can't defer
                throw("defer on system stack")
        }
@@ -234,6 +235,8 @@ func deferproc(siz int32, fn *funcval) { // arguments of fn follow fn
        if d._panic != nil {
                throw("deferproc: d.panic != nil after newdefer")
        }
+       d.link = gp._defer
+       gp._defer = d
        d.fn = fn
        d.pc = callerpc
        d.sp = sp
@@ -374,7 +377,8 @@ func init() {
 }
 
 // Allocate a Defer, usually using per-P pool.
-// Each defer must be released with freedefer.
+// Each defer must be released with freedefer.  The defer is not
+// added to any defer chain yet.
 //
 // This must not grow the stack because there may be a frame without
 // stack map information when this is called.
@@ -424,8 +428,6 @@ func newdefer(siz int32) *_defer {
        }
        d.siz = siz
        d.heap = true
-       d.link = gp._defer
-       gp._defer = d
        return d
 }