runtime: fix problem with repeated panic/recover/re-panics and open-coded defers

In the open-code defer implementation, we add defer struct entries to the defer
chain on-the-fly at panic time to represent stack frames that contain open-coded
defers. This allows us to process non-open-coded and open-coded defers in the
correct order. Also, we need somewhere to be able to store the 'started' state of
open-coded defers. However, if a recover succeeds, defers will now be processed
inline again (unless another panic happens). Any defer entry representing a frame
with open-coded defers will become stale once we run the corresponding defers
inline and exit the associated stack frame. So, we need to remove all entries for
open-coded defers at recover time.

The current code was only removing the top-most open-coded defer from the defer
chain during recovery. However, with recursive functions that do repeated
panic-recover-repanic, multiple stale entries can accumulate on the chain. So, we
just adjust the loop to process the entire chain. Since this is at panic/recover
case, it is fine to scan through the entire chain (which should usually have few
elements in it, since most defers are open-coded).

The added test fails with a SEGV without the fix, because it tries to run a stale
open-code defer entry (and the stack has changed).

Fixes #37664.

Change-Id: I8e3da5d610b5e607411451b66881dea887f7484d
Reviewed-on: https://go-review.googlesource.com/c/go/+/222420
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>

diff --git a/src/runtime/defer_test.go b/src/runtime/defer_test.go
index 3d8f81277f312dd4c62bf053433f2c191d7cd8b1..f35535e773de614c80bf9a46f2cba03b342cfc67 100644 (file)
--- a/src/runtime/defer_test.go
+++ b/src/runtime/defer_test.go
@@ -6,6 +6,7 @@ package runtime_test
 
 import (
        "fmt"
+       "os"
        "reflect"
        "runtime"
        "testing"
@@ -281,3 +282,56 @@ func TestDeferForFuncWithNoExit(t *testing.T) {
        for {
        }
 }
+
+// Test case approximating issue #37664, where a recursive function (interpreter)
+// may do repeated recovers/re-panics until it reaches the frame where the panic
+// can actually be handled. The recurseFnPanicRec() function is testing that there
+// are no stale defer structs on the defer chain after the interpreter() sequence,
+// by writing a bunch of 0xffffffffs into several recursive stack frames, and then
+// doing a single panic-recover which would invoke any such stale defer structs.
+func TestDeferWithRepeatedRepanics(t *testing.T) {
+       interpreter(0, 6, 2)
+       recurseFnPanicRec(0, 10)
+       interpreter(0, 5, 1)
+       recurseFnPanicRec(0, 10)
+       interpreter(0, 6, 3)
+       recurseFnPanicRec(0, 10)
+}
+
+func interpreter(level int, maxlevel int, rec int) {
+       defer func() {
+               e := recover()
+               if e == nil {
+                       return
+               }
+               if level != e.(int) {
+                       //fmt.Fprintln(os.Stderr, "re-panicing, level", level)
+                       panic(e)
+               }
+               //fmt.Fprintln(os.Stderr, "Recovered, level", level)
+       }()
+       if level+1 < maxlevel {
+               interpreter(level+1, maxlevel, rec)
+       } else {
+               //fmt.Fprintln(os.Stderr, "Initiating panic")
+               panic(rec)
+       }
+}
+
+func recurseFnPanicRec(level int, maxlevel int) {
+       defer func() {
+               recover()
+       }()
+       recurseFn(level, maxlevel)
+}
+
+func recurseFn(level int, maxlevel int) {
+       a := [40]uint32{0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff}
+       if level+1 < maxlevel {
+               // Need this print statement to keep a around.  '_ = a[4]' doesn't do it.
+               fmt.Fprintln(os.Stderr, "recurseFn", level, a[4])
+               recurseFn(level+1, maxlevel)
+       } else {
+               panic("recurseFn panic")
+       }
+}

diff --git a/src/runtime/panic.go b/src/runtime/panic.go
index 4cb6c8a3604ae2bdd769c0de74b1d3da4df9e0cf..c6ab1bac3fe79ee81944361a4db6f24ec3f2d4ce 100644 (file)
--- a/src/runtime/panic.go
+++ b/src/runtime/panic.go
@@ -1003,11 +1003,12 @@ func gopanic(e interface{}) {
                        atomic.Xadd(&runningPanicDefers, -1)
 
                        if done {
-                               // Remove any remaining non-started, open-coded defer
-                               // entry after a recover (there's at most one, if we just
-                               // ran a non-open-coded defer), since the entry will
-                               // become out-dated and the defer will be executed
-                               // normally.
+                               // Remove any remaining non-started, open-coded
+                               // defer entries after a recover, since the
+                               // corresponding defers will be executed normally
+                               // (inline). Any such entry will become stale once
+                               // we run the corresponding defers inline and exit
+                               // the associated stack frame.
                                d := gp._defer
                                var prev *_defer
                                for d != nil {
@@ -1025,8 +1026,9 @@ func gopanic(e interface{}) {
                                                } else {
                                                        prev.link = d.link
                                                }
+                                               newd := d.link
                                                freedefer(d)
-                                               break
+                                               d = newd
                                        } else {
                                                prev = d
                                                d = d.link