From e590cb64f940b2d4996a6e7773c1b855be952632 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Wed, 4 Aug 2021 08:54:09 -0400 Subject: [PATCH] [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 Run-TryBot: Austin Clements TryBot-Result: Go Bot Reviewed-by: Cherry Mui --- src/runtime/panic.go | 10 ++++++++++ src/runtime/runtime2.go | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) 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