runtime: fix systemstack frame pointer adjustment

Change adjustframe to adjust the frame pointer of systemstack (aka
FuncID_systemstack_switch) before returning early.

Without this fix it is possible for traceEvent() to crash when using
frame pointer unwinding. The issue occurs when a goroutine calls
systemstack in order to call shrinkstack. While returning, systemstack
will restore the unadjusted frame pointer from its frame as part of its
epilogue. If the callee of systemstack then triggers a traceEvent, it
will try to unwind into the old stack. This can lead to a crash if the
memory of the old stack has been reused or freed in the meantime.

The most common situation in which this will manifest is when when
gcAssistAlloc() invokes gcAssistAlloc1() on systemstack() and performs a
shrinkstack() followed by a traceGCMarkAssistDone() or Gosched()
triggering traceEvent().

See CL 489115 for a deterministic test case that triggers the issue.
Meanwhile the problem can frequently be observed using the command
below:

$ GODEBUG=tracefpunwindoff=0 ../bin/go test -trace /dev/null -run TestDeferHeapAndStack ./runtime
SIGSEGV: segmentation violation
PC=0x45f977 m=14 sigcode=128

goroutine 0 [idle]:
runtime.fpTracebackPCs(...)
	.../go/src/runtime/trace.go:945
runtime.traceStackID(0xcdab904677a?, {0x7f1584346018, 0x0?, 0x80}, 0x0?)
	.../go/src/runtime/trace.go:917 +0x217 fp=0x7f1565ffab00 sp=0x7f1565ffaab8 pc=0x45f977
runtime.traceEventLocked(0x0?, 0x0?, 0x0?, 0xc00003dbd0, 0x12, 0x0, 0x1, {0x0, 0x0, 0x0})
	.../go/src/runtime/trace.go:760 +0x285 fp=0x7f1565ffab78 sp=0x7f1565ffab00 pc=0x45ef45
runtime.traceEvent(0xf5?, 0x1, {0x0, 0x0, 0x0})
	.../go/src/runtime/trace.go:692 +0xa9 fp=0x7f1565ffabe0 sp=0x7f1565ffab78 pc=0x45ec49
runtime.traceGoPreempt(...)
	.../go/src/runtime/trace.go:1535
runtime.gopreempt_m(0xc000328340?)
	.../go/src/runtime/proc.go:3551 +0x45 fp=0x7f1565ffac20 sp=0x7f1565ffabe0 pc=0x4449a5
runtime.newstack()
	.../go/src/runtime/stack.go:1077 +0x3cb fp=0x7f1565ffadd0 sp=0x7f1565ffac20 pc=0x455feb
runtime.morestack()
	.../go/src/runtime/asm_amd64.s:593 +0x8f fp=0x7f1565ffadd8 sp=0x7f1565ffadd0 pc=0x47644f

goroutine 19 [running]:
runtime.traceEvent(0x2c?, 0xffffffffffffffff, {0x0, 0x0, 0x0})
	.../go/src/runtime/trace.go:669 +0xe8 fp=0xc0006e6c28 sp=0xc0006e6c20 pc=0x45ec88
runtime.traceGCMarkAssistDone(...)
	.../go/src/runtime/trace.go:1497
runtime.gcAssistAlloc(0xc0003281a0)
	.../go/src/runtime/mgcmark.go:517 +0x27d fp=0xc0006e6c88 sp=0xc0006e6c28 pc=0x421a1d
runtime.deductAssistCredit(0x0?)
	.../go/src/runtime/malloc.go:1287 +0x54 fp=0xc0006e6cb0 sp=0xc0006e6c88 pc=0x40fed4
runtime.mallocgc(0x400, 0x7a9420, 0x1)
	.../go/src/runtime/malloc.go:1002 +0xc9 fp=0xc0006e6d18 sp=0xc0006e6cb0 pc=0x40f709
runtime.newobject(0xb3?)
	.../go/src/runtime/malloc.go:1324 +0x25 fp=0xc0006e6d40 sp=0xc0006e6d18 pc=0x40ffc5
runtime_test.deferHeapAndStack(0xb4)
	.../go/src/runtime/stack_test.go:924 +0x165 fp=0xc0006e6e20 sp=0xc0006e6d40 pc=0x75c2a5

Fixes #59692

Co-Authored-By: Cherry Mui <cherryyz@google.com>
Co-Authored-By: Michael Knyszek <mknyszek@google.com>
Co-Authored-By: Nick Ripley <nick.ripley@datadoghq.com>
Change-Id: I1c0c28327fc2fac0b8cfdbaa72e25584331be31e
Reviewed-on: https://go-review.googlesource.com/c/go/+/489015
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Felix Geisendörfer <felix.geisendoerfer@datadoghq.com>

diff --git a/src/runtime/stack.go b/src/runtime/stack.go
index 3f1e5ff9198c02f36f3835e2d74ed3832f4aa8f5..54513eba6578bd53530ad7beeb27bceadcf68fb9 100644 (file)
--- a/src/runtime/stack.go
+++ b/src/runtime/stack.go
@@ -650,20 +650,6 @@ func adjustframe(frame *stkframe, adjinfo *adjustinfo) {
        if stackDebug >= 2 {
                print("    adjusting ", funcname(f), " frame=[", hex(frame.sp), ",", hex(frame.fp), "] pc=", hex(frame.pc), " continpc=", hex(frame.continpc), "\n")
        }
-       if f.funcID == abi.FuncID_systemstack_switch {
-               // A special routine at the bottom of stack of a goroutine that does a systemstack call.
-               // We will allow it to be copied even though we don't
-               // have full GC info for it (because it is written in asm).
-               return
-       }
-
-       locals, args, objs := frame.getStackMap(&adjinfo.cache, true)
-
-       // Adjust local variables if stack frame has been allocated.
-       if locals.n > 0 {
-               size := uintptr(locals.n) * goarch.PtrSize
-               adjustpointers(unsafe.Pointer(frame.varp-size), &locals, adjinfo, f)
-       }
 
        // Adjust saved frame pointer if there is one.
        if (goarch.ArchFamily == goarch.AMD64 || goarch.ArchFamily == goarch.ARM64) && frame.argp-frame.varp == 2*goarch.PtrSize {
@@ -687,6 +673,21 @@ func adjustframe(frame *stkframe, adjinfo *adjustinfo) {
                adjustpointer(adjinfo, unsafe.Pointer(frame.varp))
        }
 
+       if f.funcID == abi.FuncID_systemstack_switch {
+               // A special routine at the bottom of stack of a goroutine that does a systemstack call.
+               // We will allow it to be copied even though we don't
+               // have full GC info for it (because it is written in asm).
+               return
+       }
+
+       locals, args, objs := frame.getStackMap(&adjinfo.cache, true)
+
+       // Adjust local variables if stack frame has been allocated.
+       if locals.n > 0 {
+               size := uintptr(locals.n) * goarch.PtrSize
+               adjustpointers(unsafe.Pointer(frame.varp-size), &locals, adjinfo, f)
+       }
+
        // Adjust arguments.
        if args.n > 0 {
                if stackDebug >= 3 {