From fd85ff5ee0b0c999da59e5f56237070e1aad2df3 Mon Sep 17 00:00:00 2001 From: Dan Scales Date: Thu, 5 Mar 2020 12:46:04 -0800 Subject: [PATCH] [release-branch.go1.14] 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). Updates #37664. Fixes #37782. Change-Id: I8e3da5d610b5e607411451b66881dea887f7484d Reviewed-on: https://go-review.googlesource.com/c/go/+/222420 Run-TryBot: Dan Scales TryBot-Result: Gobot Gobot Reviewed-by: Keith Randall (cherry picked from commit fae87a2223e1fa959a20017742455200fe3c35f1) Reviewed-on: https://go-review.googlesource.com/c/go/+/222818 Run-TryBot: Dmitri Shuralyov --- src/runtime/defer_test.go | 54 +++++++++++++++++++++++++++++++++++++++ src/runtime/panic.go | 14 +++++----- 2 files changed, 62 insertions(+), 6 deletions(-) diff --git a/src/runtime/defer_test.go b/src/runtime/defer_test.go index 3d8f81277f312..f35535e773de6 100644 --- 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 4cb6c8a3604ae..c6ab1bac3fe79 100644 --- 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