runtime/trace: avoid frame pointer unwinding for events during cgocallbackg

The current mp.incgocallback() logic allows for trace events to be
recorded using frame pointer unwinding during cgocallbackg when they
shouldn't be. Specifically, mp.incgo will be false during the
reentersyscall call at the end. It's possible to crash with tracing
enabled because of this, if C code which uses the frame pointer register
for other purposes calls into Go. This can be seen, for example, by
forcing testprogcgo/trace_unix.c to write a garbage value to RBP prior
to calling into Go.

We can drop the mp.incgo check, and instead conservatively avoid doing
frame pointer unwinding if there is any C on the stack. This is the case
if mp.ncgo > 0, or if mp.isextra is true (meaning we're coming from a
thread created by C). Rename incgocallback to reflect that we're
checking if there's any C on the stack. We can also move the ncgo
increment in cgocall closer to where the transition to C happens, which
lets us use frame pointer unwinding for the entersyscall event during
the first Go-to-C call on a stack, when there isn't yet any C on the
stack.

Fixes #59830.

Change-Id: If178a705a9d38d0d2fb19589a9e669cd982d32cd
Reviewed-on: https://go-review.googlesource.com/c/go/+/488755
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Felix Geisendörfer <felix.geisendoerfer@datadoghq.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Nick Ripley <nick.ripley@datadoghq.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

diff --git a/src/runtime/cgocall.go b/src/runtime/cgocall.go
index 7f1a02fb4b67e6b7e24584f3ccd4fac4f9e89cb9..c2552cbdf5bb997dae702da20b5b235b68e84e99 100644 (file)
--- a/src/runtime/cgocall.go
+++ b/src/runtime/cgocall.go
@@ -136,7 +136,6 @@ func cgocall(fn, arg unsafe.Pointer) int32 {
 
        mp := getg().m
        mp.ncgocall++
-       mp.ncgo++
 
        // Reset traceback.
        mp.cgoCallers[0] = 0
@@ -165,6 +164,14 @@ func cgocall(fn, arg unsafe.Pointer) int32 {
        osPreemptExtEnter(mp)
 
        mp.incgo = true
+       // We use ncgo as a check during execution tracing for whether there is
+       // any C on the call stack, which there will be after this point. If
+       // there isn't, we can use frame pointer unwinding to collect call
+       // stacks efficiently. This will be the case for the first Go-to-C call
+       // on a stack, so it's prefereable to update it here, after we emit a
+       // trace event in entersyscall above.
+       mp.ncgo++
+
        errno := asmcgocall(fn, arg)
 
        // Update accounting before exitsyscall because exitsyscall may

diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index d2901e3aa0257bc0a57982bc4c3a0647a53418b7..0b9df169b2056a1d18060fc3d547eb3ff4d9d349 100644 (file)
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -868,8 +868,8 @@ func (mp *m) becomeSpinning() {
        sched.needspinning.Store(0)
 }
 
-func (mp *m) incgocallback() bool {
-       return (!mp.incgo && mp.ncgo > 0) || mp.isextra
+func (mp *m) hasCgoOnStack() bool {
+       return mp.ncgo > 0 || mp.isextra
 }
 
 var fastrandseed uintptr

diff --git a/src/runtime/trace.go b/src/runtime/trace.go
index c382068e2fd606844ec1e7c218a6983314305a82..79ccebb4b31736c8e2546efe2dd408a251436f4c 100644 (file)
--- a/src/runtime/trace.go
+++ b/src/runtime/trace.go
@@ -889,10 +889,10 @@ func traceStackID(mp *m, pcBuf []uintptr, skip int) uint64 {
        gp := getg()
        curgp := mp.curg
        nstk := 1
-       if tracefpunwindoff() || mp.incgocallback() {
+       if tracefpunwindoff() || mp.hasCgoOnStack() {
                // Slow path: Unwind using default unwinder. Used when frame pointer
                // unwinding is unavailable or disabled (tracefpunwindoff), or might
-               // produce incomplete results or crashes (incgocallback). Note that no
+               // produce incomplete results or crashes (hasCgoOnStack). Note that no
                // cgo callback related crashes have been observed yet. The main
                // motivation is to take advantage of a potentially registered cgo
                // symbolizer.