runtime: do not add open defer entry above a started open defer entry

Fix two defer bugs related to adding/removing open defer entries.
The bugs relate to the way that we add and remove open defer entries
from the defer chain. At the point of a panic, when we want to start
processing defer entries in order during the panic process, we need to
add entries to the defer chain for stack frames with open defers, since
the normal fast-defer code does not add these entries. We do this by
calling addOneOpenDeferFrame() at the beginning of each time around the
defer loop in gopanic(). Those defer entries get sorted with other open
and non-open-coded defer frames.

However, the tricky part is that we also need to remove defer entries if
they end not being needed because of a recover (which means we are back
to executing the defer code inline at function exits). But we need
to deal with multiple panics and in-process defers on the stack, so we
can't just remove all open-coded defers from the the defer chain during
a recover.

The fix (and new invariant) is that we should not add any open-coded
defers to the defer chain that are higher up the stack than an open-coded
defer that is in progress. We know that open-coded defer will still be
run until completed, and when it is completed, then a more outer frame
will be added (if there is one). This fits with existing code in gopanic
that only removes open-coded defer entries up to any defer in progress.

These bugs were because of the previous inconsistency between adding and
removing open defer entries, which meant that stale defer entries could
be left on the list, in these unusual cases with both recursive
panics plus multiple independent (non-nested) cases of panic & recover.

The test for #48898 was difficult to add to defer_test.go (while keeping
the failure mode), so I added as a go/test/fixedbug test instead.

Fixes #43920
Updates #43941
Fixes #48898

Change-Id: I593b77033e08c33094315abf8089fbc4cab07376
Reviewed-on: https://go-review.googlesource.com/c/go/+/356011
Trust: Dan Scales <[email protected]>
Trust: Cuong Manh Le <[email protected]>
Reviewed-by: Austin Clements <[email protected]>

Author: danscales

src/runtime/defer_test.go

@@ -438,3 +438,82 @@ func expect(t *testing.T, n int, err interface{}) {
 		t.Fatalf("have %v, want %v", err, n)
 	}
 }
+
+func TestIssue43920(t *testing.T) {
+	var steps int
+
+	defer func() {
+		expect(t, 1, recover())
+	}()
+	defer func() {
+		defer func() {
+			defer func() {
+				expect(t, 5, recover())
+			}()
+			defer panic(5)
+			func() {
+				panic(4)
+			}()
+		}()
+		defer func() {
+			expect(t, 3, recover())
+		}()
+		defer panic(3)
+	}()
+	func() {
+		defer step(t, &steps, 1)
+		panic(1)
+	}()
+}
+
+func step(t *testing.T, steps *int, want int) {
+	println("step", want)
+	*steps++
+	if *steps != want {
+		t.Fatalf("have %v, want %v", *steps, want)
+	}
+}
+
+func TestIssue43941(t *testing.T) {
+	var steps int = 7
+	defer func() {
+		step(t, &steps, 14)
+		expect(t, 4, recover())
+	}()
+	func() {
+		func() {
+			defer func() {
+				defer func() {
+					expect(t, 3, recover())
+				}()
+				defer panic(3)
+				panic(2)
+			}()
+			defer func() {
+				expect(t, 1, recover())
+			}()
+			defer panic(1)
+		}()
+		defer func() {}()
+		defer func() {}()
+		defer step(t, &steps, 10)
+		defer step(t, &steps, 9)
+		step(t, &steps, 8)
+	}()
+	func() {
+		defer step(t, &steps, 13)
+		defer step(t, &steps, 12)
+		func() {
+			defer step(t, &steps, 11)
+			panic(4)
+		}()
+
+		// Code below isn't executed,
+		// but removing it breaks the test case.
+		defer func() {}()
+		defer panic(-1)
+		defer step(t, &steps, -1)
+		defer step(t, &steps, -1)
+		defer func() {}()
+	}()
+}

src/runtime/panic.go

@@ -560,14 +560,28 @@ func printpanics(p *_panic) {
 	print("\n")
 }
 
-// addOneOpenDeferFrame scans the stack for the first frame (if any) with
-// open-coded defers and if it finds one, adds a single record to the defer chain
-// for that frame. If sp is non-nil, it starts the stack scan from the frame
-// specified by sp. If sp is nil, it uses the sp from the current defer record
-// (which has just been finished). Hence, it continues the stack scan from the
-// frame of the defer that just finished. It skips any frame that already has an
-// open-coded _defer record, which would have been created from a previous
-// (unrecovered) panic.
+// addOneOpenDeferFrame scans the stack (in gentraceback order, from inner frames to
+// outer frames) for the first frame (if any) with open-coded defers. If it finds
+// one, it adds a single entry to the defer chain for that frame. The entry added
+// represents all the defers in the associated open defer frame, and is sorted in
+// order with respect to any non-open-coded defers.
+//
+// addOneOpenDeferFrame stops (possibly without adding a new entry) if it encounters
+// an in-progress open defer entry. An in-progress open defer entry means there has
+// been a new panic because of a defer in the associated frame. addOneOpenDeferFrame
+// does not add an open defer entry past a started entry, because that started entry
+// still needs to finished, and addOneOpenDeferFrame will be called when that started
+// entry is completed. The defer removal loop in gopanic() similarly stops at an
+// in-progress defer entry. Together, addOneOpenDeferFrame and the defer removal loop
+// ensure the invariant that there is no open defer entry further up the stack than
+// an in-progress defer, and also that the defer removal loop is guaranteed to remove
+// all not-in-progress open defer entries from the defer chain.
+//
+// If sp is non-nil, addOneOpenDeferFrame starts the stack scan from the frame
+// specified by sp. If sp is nil, it uses the sp from the current defer record (which
+// has just been finished). Hence, it continues the stack scan from the frame of the
+// defer that just finished. It skips any frame that already has a (not-in-progress)
+// open-coded _defer record in the defer chain.
 //
 // Note: All entries of the defer chain (including this new open-coded entry) have
 // their pointers (including sp) adjusted properly if the stack moves while
@@ -608,6 +622,16 @@ func addOneOpenDeferFrame(gp *g, pc uintptr, sp unsafe.Pointer) {
 						if !d.openDefer {
 							throw("duplicated defer entry")
 						}
+						// Don't add any record past an
+						// in-progress defer entry. We don't
+						// need it, and more importantly, we
+						// want to keep the invariant that
+						// there is no open defer entry
+						// passed an in-progress entry (see
+						// header comment).
+						if d.started {
+							return false
+						}
 						return true
 					}
 					prev = d
@@ -849,12 +873,15 @@ func gopanic(e interface{}) {
 			}
 			atomic.Xadd(&runningPanicDefers, -1)
 
-			// 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.
+			// After a recover, remove any remaining non-started,
+			// open-coded defer entries, 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. We only remove up to
+			// the first started (in-progress) open defer entry, not
+			// including the current frame, since any higher entries will
+			// be from a higher panic in progress, and will still be
+			// needed.
 			d := gp._defer
 			var prev *_defer
 			if !done {

test/fixedbugs/issue48898.go

@@ -0,0 +1,40 @@
+// run
+
+// Copyright 2021 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+func main() {
+	defer func() {
+		println(recover().(int))
+	}()
+	func() {
+		func() (_ [2]int) { type _ int; return }()
+		func() {
+			defer func() {
+				defer func() {
+					recover()
+				}()
+				defer panic(3)
+				panic(2)
+			}()
+			defer func() {
+				recover()
+			}()
+			panic(1)
+		}()
+		defer func() {}()
+	}()
+
+	var x = 123
+	func() {
+		// in the original issue, this defer was not executed (which is incorrect)
+		defer print(x)
+		func() {
+			defer func() {}()
+			panic(4)
+		}()
+	}()
+}

test/fixedbugs/issue48898.out

@@ -0,0 +1 @@
+1234