mirror of
https://github.com/golang/go
synced 2024-09-04 15:34:21 +00:00
[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>
This commit is contained in:
parent
d27a889119
commit
e590cb64f9
|
@ -338,7 +338,17 @@ func newdefer() *_defer {
|
||||||
|
|
||||||
// Free the given defer.
|
// Free the given defer.
|
||||||
// The defer cannot be used after this call.
|
// 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) {
|
func freedefer(d *_defer) {
|
||||||
|
d.link = nil
|
||||||
|
// After this point we can copy the stack.
|
||||||
|
|
||||||
if d._panic != nil {
|
if d._panic != nil {
|
||||||
freedeferpanic()
|
freedeferpanic()
|
||||||
}
|
}
|
||||||
|
|
|
@ -957,7 +957,7 @@ type _defer struct {
|
||||||
pc uintptr // pc at time of defer
|
pc uintptr // pc at time of defer
|
||||||
fn func() // can be nil for open-coded defers
|
fn func() // can be nil for open-coded defers
|
||||||
_panic *_panic // panic that is running defer
|
_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
|
// If openDefer is true, the fields below record values about the stack
|
||||||
// frame and associated function that has the open-coded defer(s). sp
|
// frame and associated function that has the open-coded defer(s). sp
|
||||||
|
|
Loading…
Reference in a new issue