diff --git a/src/runtime/cgocall.go b/src/runtime/cgocall.go index 626f7edf011611..98304360765a3d 100644 --- a/src/runtime/cgocall.go +++ b/src/runtime/cgocall.go @@ -349,9 +349,9 @@ func cgocallbackg(fn, frame unsafe.Pointer, ctxt uintptr) { savedbp := unsafe.Pointer(gp.syscallbp) exitsyscall() // coming out of cgo call gp.m.incgo = false - if gp.m.isextra { - gp.m.isExtraInC = false - } + // isExtraInC is now cleared inside exitsyscall (see proc.go), + // eliminating the race window where a SIGPROF could see + // isExtraInC == true while we're already running Go code (#70529). osPreemptExtExit(gp.m) diff --git a/src/runtime/crash_cgo_test.go b/src/runtime/crash_cgo_test.go index d4b7421ac09a65..5e12cc0aaef571 100644 --- a/src/runtime/crash_cgo_test.go +++ b/src/runtime/crash_cgo_test.go @@ -505,6 +505,34 @@ func TestCgoPprofThreadNoTraceback(t *testing.T) { testCgoPprof(t, "", "CgoPprofThreadNoTraceback", "cpuHogThread", "runtime._ExternalCode") } +// TestCgoCallbackPprofRace tests that high-frequency C→Go callbacks +// under continuous CPU profiling do not crash due to the #70529 race +// (isExtraInC stale during exitsyscall). Running under -race changes +// scheduling timing and increases the likelihood of hitting the window. +func TestCgoCallbackPprofRace(t *testing.T) { + if runtime.GOOS != "linux" || (runtime.GOARCH != "amd64" && runtime.GOARCH != "arm64") { + t.Skipf("not yet supported on %s/%s", runtime.GOOS, runtime.GOARCH) + } + if runtime.GOOS == "freebsd" && race.Enabled { + t.Skipf("race + cgo freebsd not supported. See https://go.dev/issue/73788.") + } + testenv.MustHaveGoRun(t) + + exe, err := buildTestProg(t, "testprogcgo") + if err != nil { + t.Fatal(err) + } + + cmd := testenv.CleanCmdEnv(exec.Command(exe, "CgoCallbackPprofRace")) + got, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("CgoCallbackPprofRace failed (this is the #70529 panic if unpatched): %v\n%s", err, got) + } + if want := "OK\n"; string(got) != want { + t.Fatalf("expected %q got %q", want, string(got)) + } +} + func TestRaceProf(t *testing.T) { if !race.Enabled { t.Skip("skipping: race detector not enabled") diff --git a/src/runtime/pprof/proto.go b/src/runtime/pprof/proto.go index 5ad917f14a7292..073feee003b015 100644 --- a/src/runtime/pprof/proto.go +++ b/src/runtime/pprof/proto.go @@ -407,7 +407,6 @@ func (b *profileBuilder) appendLocsForStack(locs []uint64, stk []uintptr) (newLo b.deck.reset() // The last frame might be truncated. Recover lost inline frames. - origStk := stk stk = runtime_expandFinalInlineFrame(stk) for len(stk) > 0 { @@ -444,8 +443,21 @@ func (b *profileBuilder) appendLocsForStack(locs []uint64, stk []uintptr) (newLo // Even if stk was truncated due to the stack depth // limit, expandFinalInlineFrame above has already // fixed the truncation, ensuring it is long enough. + // + // However, sigprofNonGoPC (runtime/signal_unix.go) can + // produce a short stack {pc, _ExternalCode} for a PC + // that is actually running Go code, when a SIGPROF lands + // in the window between exitsyscall() returning and + // m.isExtraInC being cleared in cgocallbackg + // (golang/go#70529). If that PC was previously sampled + // in a normal Go context, l.pcs here holds its full + // inlined expansion, which can be longer than the short + // stack. Rather than panic, drop the remaining + // (unmatchable) PCs of this sample; the Location we just + // recorded is still correct for the leaf, and the cost + // is a few missing caller frames on a rare sample. if len(l.pcs) > len(stk) { - panic(fmt.Sprintf("stack too short to match cached location; stk = %#x, l.pcs = %#x, original stk = %#x", stk, l.pcs, origStk)) + return locs } stk = stk[len(l.pcs):] continue diff --git a/src/runtime/pprof/proto_test.go b/src/runtime/pprof/proto_test.go index b22d6e2b0366c3..e4aaadae08470b 100644 --- a/src/runtime/pprof/proto_test.go +++ b/src/runtime/pprof/proto_test.go @@ -489,3 +489,60 @@ func TestWriteToErr(t *testing.T) { t.Fatalf("want error from writer, got: %v", err) } } + +// TestIssue70529 reproduces golang/go#70529. sigprofNonGoPC can +// produce a short stack {pc, _ExternalCode} for a PC that was +// previously sampled with a full inlined expansion. This triggers a +// panic in appendLocsForStack when the cached l.pcs is longer than +// the short stack. +// +// The test feeds profileBuilder a long-stack sample (building an +// l.pcs cache of length >=3 via a 3-level inlined call chain) followed +// by a short-stack sample simulating sigprofNonGoPC's output. +func TestIssue70529(t *testing.T) { + if _, found := findInlinedCall(recursionChainBottom, 4<<10); !found { + t.Skip("Can't determine whether anything was inlined into recursionChainBottom.") + } + + pcs := make([]uintptr, 6) + recursionChainTop(1, pcs) + + // Find a PC that expands to >=3 frames via runtime_expandFinalInlineFrame. + var inlinedPC, callerPC uint64 + for i, pc := range pcs { + if pc == 0 { + break + } + if expanded := runtime_expandFinalInlineFrame([]uintptr{pc}); len(expanded) >= 3 && inlinedPC == 0 { + inlinedPC = uint64(pc) + if i+1 < len(pcs) && pcs[i+1] != 0 { + callerPC = uint64(pcs[i+1]) + } + break + } + } + if inlinedPC == 0 { + t.Skip("No PC that expands to >=3 frames found; can't trigger #70529.") + } + if callerPC == 0 { + callerPC = uint64(abi.FuncPCABIInternal(recursionChainTop)) + } + + externalPC := uint64(abi.FuncPCABIInternal(externalCodeForTest) + 1) + + data := []uint64{ + 3, 0, 500, // hz = 500 + 5, 0, 10, inlinedPC, callerPC, // full stack: builds l.pcs cache (len >=3) + 5, 0, 5, inlinedPC, externalPC, // short stack: simulates sigprofNonGoPC (len 2) + } + p, err := translateCPUProfile(data, 3) + if err != nil { + t.Fatalf("translateCPUProfile failed: %v", err) + } + if p == nil || len(p.Sample) != 2 { + t.Fatalf("expected 2 samples, got %d", len(p.Sample)) + } +} + +//go:noinline +func externalCodeForTest() { externalCodeForTest() } diff --git a/src/runtime/proc.go b/src/runtime/proc.go index 79f82c2ed46dd8..514b9d0707ee53 100644 --- a/src/runtime/proc.go +++ b/src/runtime/proc.go @@ -4954,6 +4954,23 @@ func exitsyscall() { casgstatus(gp, _Gsyscall, _Grunning) } + // We are now running Go code. Clear isExtraInC early (before + // acquiring a P) so that a SIGPROF landing in the window between + // here and P acquisition routes through sigprof (full Go stack + // traceback) rather than sigprofNonGoPC (short {pc, _ExternalCode} + // stack). Previously this was cleared in cgocallbackg after + // exitsyscall returned, leaving a race window (#70529) where a + // reused extra M still had isExtraInC == true while executing Go + // code. For non-cgo syscall exits, isExtraInC is already false, so + // this is a no-op. + // + // This is safe because isExtraInC is only read by addGSyscallNoP/ + // decGSyscallNoP, which require it to be stable outside _Gsyscall; + // we are now in _Grunning. + if gp.m.isextra { + gp.m.isExtraInC = false + } + // Caution: we're in a window where we may be in _Grunning without a P. // Either we will grab a P or call exitsyscall0, where we'll switch to // _Grunnable. diff --git a/src/runtime/testdata/testprogcgo/callback_pprof_race.go b/src/runtime/testdata/testprogcgo/callback_pprof_race.go new file mode 100644 index 00000000000000..90fd40b706e6aa --- /dev/null +++ b/src/runtime/testdata/testprogcgo/callback_pprof_race.go @@ -0,0 +1,77 @@ +// Copyright 2026 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build !plan9 && !windows + +package main + +// Test program for TestCgoCallbackPprofRace: high-frequency C→Go +// callbacks under continuous CPU profiling, exercising the #70529 +// race window in cgocallbackg. + +/* +#include +#include + +extern void GoCallback70529(); + +static volatile int stop70529 = 0; + +static void *callback_race_worker(void *arg) { + while (!stop70529) { + GoCallback70529(); + sched_yield(); + } + return 0; +} + +static void start_callback_race_workers(int n) { + pthread_t tids[16]; + for (int i = 0; i < n && i < 16; i++) { + pthread_create(&tids[i], 0, callback_race_worker, 0); + } +} + +static void stop_callback_race_workers(void) { + stop70529 = 1; +} +*/ +import "C" + +import ( + "bytes" + "fmt" + "runtime/pprof" + "sync/atomic" + "time" +) + +func init() { + register("CgoCallbackPprofRace", CgoCallbackPprofRace) +} + +func CgoCallbackPprofRace() { + C.start_callback_race_workers(8) + + for round := 0; round < 50; round++ { + var buf bytes.Buffer + if err := pprof.StartCPUProfile(&buf); err != nil { + continue + } + time.Sleep(200 * time.Millisecond) + pprof.StopCPUProfile() + } + + C.stop_callback_race_workers() + time.Sleep(100 * time.Millisecond) + + fmt.Printf("OK\n") +} + +//export GoCallback70529 +func GoCallback70529() { + atomic.AddUint64(&callbackSink70529, 1) +} + +var callbackSink70529 uint64