[dev.typeparams] runtime: handle d.link carefully when freeing a defer

CL 339396 allowed stack copying on entry to and during freedefer, but
this introduced a subtle bug: if d is heap-allocated, and d.link
points to a stack-allocated defer, stack copying during freedefer can
briefly introduce a stale pointer, which the garbage collector can
discover and panic about. This happens because d has already been
unlinked from the defer chain when freedefer is called, so stack
copying won't update stack pointers in it.

Fix this by making freedefer nosplit again and immediately clearing
d.link.

This should fix the longtest builders, which currently fail on
GOMAXPROCS=2 runtime -cpu=1,2,4 -quick in the TestDeferHeapAndStack
test.

This seems like the simplest fix, but it just deals with the subtlety
rather than eliminating it. Really, every call site of freedefer (of
which there are surprisingly many) has hidden subtlety between
unlinking the defer and calling freedefer. We could consolidate the
subtlety into each call site by requiring that they unlink the defer
and set d.link to nil before calling freedefer. freedefer could check
this condition like it checks that various other fields have already
been zeroed. A more radical option is to replace freedefer with
"popDefer", which would both pop the defer off the link and take care
of freeing it. There would still be a brief moment of subtlety, but it
would be in one place, in popDefer. Annoyingly, *almost* every call to
freedefer just pops the defer from the head of the G's list, but
there's one place when handling open-coded defers where we have to
remove a defer from the middle of the list. I'm inclined to first fix
that subtlety by only expanding open-coded defer records when they're
at the head of the defer list, and then revisit the popDefer idea.

Change-Id: I3130d2542c01a421a5d60e8c31f5379263219627
Reviewed-on: https://go-review.googlesource.com/c/go/+/339730
Trust: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>

2 files changed, 11 insertions, 1 deletions

diff --git a/src/runtime/panic.go b/src/runtime/panic.go
index 48b1b5dd9d..e4bdceb32f 100644
--- a/src/runtime/panic.go
+++ b/src/runtime/panic.go
@@ -338,7 +338,17 @@ func newdefer() *_defer {
 
 // Free the given defer.
 // The defer cannot be used after this call.
+//
+// This is nosplit because the incoming defer is in a perilous state.
+// It's not on any defer list, so stack copying won't adjust stack
+// pointers in it (namely, d.link). Hence, if we were to copy the
+// stack, d could then contain a stale pointer.
+//
+//go:nosplit
 func freedefer(d *_defer) {
+	d.link = nil
+	// After this point we can copy the stack.
+
 	if d._panic != nil {
 		freedeferpanic()
 	}
diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go
index b5e4b3dec8..c5e2501991 100644
--- a/src/runtime/runtime2.go
+++ b/src/runtime/runtime2.go
@@ -957,7 +957,7 @@ type _defer struct {
 	pc        uintptr // pc at time of defer
 	fn        func()  // can be nil for open-coded defers
 	_panic    *_panic // panic that is running defer
-	link      *_defer
+	link      *_defer // next defer on G; can point to either heap or stack!
 
 	// If openDefer is true, the fields below record values about the stack
 	// frame and associated function that has the open-coded defer(s). sp