runtime: avoid possible preemption when returning from Go to C
When returning from Go to C, it was possible for the goroutine to be
preempted after calling unlockOSThread. This could happen when there
a context function installed by SetCgoTraceback set a non-zero context,
leading to a defer call in cgocallbackg1. The defer function wrapper,
introduced in 1.17 as part of the regabi support, was not nosplit,
and hence was a potential preemption point. If it did get preempted,
the G would move to a new M. It would then attempt to return to C
code on a different stack, typically leading to a SIGSEGV.
Fix this in a simple way by postponing the unlockOSThread until after
the other defer. Also check for the failure condition and fail early,
rather than waiting for a SIGSEGV.
Without the fix to cgocall.go, the test case fails about 50% of the
time on my laptop.
Fixes #47441
Change-Id: Ib8ca13215bd36cddc2a49e86698824a29c6a68ba
Reviewed-on: https://go-review.googlesource.com/c/go/+/338197
Trust: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
diff --git a/src/runtime/cgocall.go b/src/runtime/cgocall.go
index 8ffb48a..2626216 100644
--- a/src/runtime/cgocall.go
+++ b/src/runtime/cgocall.go
@@ -212,6 +212,8 @@
// a different M. The call to unlockOSThread is in unwindm.
lockOSThread()
+ checkm := gp.m
+
// Save current syscall parameters, so m.syscall can be
// used again if callback decide to make syscall.
syscall := gp.m.syscall
@@ -227,15 +229,20 @@
osPreemptExtExit(gp.m)
- cgocallbackg1(fn, frame, ctxt)
+ cgocallbackg1(fn, frame, ctxt) // will call unlockOSThread
// At this point unlockOSThread has been called.
// The following code must not change to a different m.
// This is enforced by checking incgo in the schedule function.
+ gp.m.incgo = true
+
+ if gp.m != checkm {
+ throw("m changed unexpectedly in cgocallbackg")
+ }
+
osPreemptExtEnter(gp.m)
- gp.m.incgo = true
// going back to cgo call
reentersyscall(savedpc, uintptr(savedsp))
@@ -244,6 +251,11 @@
func cgocallbackg1(fn, frame unsafe.Pointer, ctxt uintptr) {
gp := getg()
+
+ // When we return, undo the call to lockOSThread in cgocallbackg.
+ // We must still stay on the same m.
+ defer unlockOSThread()
+
if gp.m.needextram || atomic.Load(&extraMWaiters) > 0 {
gp.m.needextram = false
systemstack(newextram)
@@ -323,10 +335,6 @@
releasem(mp)
}
-
- // Undo the call to lockOSThread in cgocallbackg.
- // We must still stay on the same m.
- unlockOSThread()
}
// called from assembly
diff --git a/src/runtime/crash_cgo_test.go b/src/runtime/crash_cgo_test.go
index 7d25c51..5729942 100644
--- a/src/runtime/crash_cgo_test.go
+++ b/src/runtime/crash_cgo_test.go
@@ -282,6 +282,15 @@
}
}
+func TestCgoTracebackContextPreemption(t *testing.T) {
+ t.Parallel()
+ got := runTestProg(t, "testprogcgo", "TracebackContextPreemption")
+ want := "OK\n"
+ if got != want {
+ t.Errorf("expected %q got %v", want, got)
+ }
+}
+
func testCgoPprof(t *testing.T, buildArg, runArg, top, bottom string) {
t.Parallel()
if runtime.GOOS != "linux" || (runtime.GOARCH != "amd64" && runtime.GOARCH != "ppc64le") {
diff --git a/src/runtime/testdata/testprogcgo/tracebackctxt.go b/src/runtime/testdata/testprogcgo/tracebackctxt.go
index 51fa4ad..62ff8ec 100644
--- a/src/runtime/testdata/testprogcgo/tracebackctxt.go
+++ b/src/runtime/testdata/testprogcgo/tracebackctxt.go
@@ -2,8 +2,6 @@
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
-// The __attribute__((weak)) used below doesn't seem to work on Windows.
-
package main
// Test the context argument to SetCgoTraceback.
@@ -14,20 +12,24 @@
extern void C1(void);
extern void C2(void);
extern void tcContext(void*);
+extern void tcContextSimple(void*);
extern void tcTraceback(void*);
extern void tcSymbolizer(void*);
extern int getContextCount(void);
+extern void TracebackContextPreemptionCallGo(int);
*/
import "C"
import (
"fmt"
"runtime"
+ "sync"
"unsafe"
)
func init() {
register("TracebackContext", TracebackContext)
+ register("TracebackContextPreemption", TracebackContextPreemption)
}
var tracebackOK bool
@@ -105,3 +107,30 @@
tracebackOK = false
}
}
+
+// Issue 47441.
+func TracebackContextPreemption() {
+ runtime.SetCgoTraceback(0, unsafe.Pointer(C.tcTraceback), unsafe.Pointer(C.tcContextSimple), unsafe.Pointer(C.tcSymbolizer))
+
+ const funcs = 10
+ const calls = 1e5
+ var wg sync.WaitGroup
+ for i := 0; i < funcs; i++ {
+ wg.Add(1)
+ go func(i int) {
+ defer wg.Done()
+ for j := 0; j < calls; j++ {
+ C.TracebackContextPreemptionCallGo(C.int(i*calls + j))
+ }
+ }(i)
+ }
+ wg.Wait()
+
+ fmt.Println("OK")
+}
+
+//export TracebackContextPreemptionGoFunction
+func TracebackContextPreemptionGoFunction(i C.int) {
+ // Do some busy work.
+ fmt.Sprintf("%d\n", i)
+}
diff --git a/src/runtime/testdata/testprogcgo/tracebackctxt_c.c b/src/runtime/testdata/testprogcgo/tracebackctxt_c.c
index 900cada..910cb7b 100644
--- a/src/runtime/testdata/testprogcgo/tracebackctxt_c.c
+++ b/src/runtime/testdata/testprogcgo/tracebackctxt_c.c
@@ -11,6 +11,7 @@
// Functions exported from Go.
extern void G1(void);
extern void G2(void);
+extern void TracebackContextPreemptionGoFunction(int);
void C1() {
G1();
@@ -62,10 +63,17 @@
}
}
+void tcContextSimple(void* parg) {
+ struct cgoContextArg* arg = (struct cgoContextArg*)(parg);
+ if (arg->context == 0) {
+ arg->context = 1;
+ }
+}
+
void tcTraceback(void* parg) {
int base, i;
struct cgoTracebackArg* arg = (struct cgoTracebackArg*)(parg);
- if (arg->context == 0) {
+ if (arg->context == 0 && arg->sigContext == 0) {
// This shouldn't happen in this program.
abort();
}
@@ -89,3 +97,7 @@
arg->func = "cFunction";
arg->lineno = arg->pc + (arg->more << 16);
}
+
+void TracebackContextPreemptionCallGo(int i) {
+ TracebackContextPreemptionGoFunction(i);
+}
