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 <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Reviewed-by: Felix Geisendörfer <[email protected]>
Auto-Submit: Michael Knyszek <[email protected]>
Run-TryBot: Nick Ripley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: nsrip-dd

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

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

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.