qbit/go
1
0
Fork 0
mirror of https://github.com/golang/go synced 2024-11-26 05:07:59 -07:00
Code Releases Activity

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

Browse Source 
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:
Austin Clements 2021-08-04 08:54:09 -04:00
parent d27a889119
commit e590cb64f9
2 changed files with 11 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
src/runtime/panic.go
View File

@ -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()
}

2
src/runtime/runtime2.go
View File

@ -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