From 92154aed278f190a57b26b2e9e6b51c2d35c54f9 Mon Sep 17 00:00:00 2001 From: lizhaolong <0x4f4f4f4f@gmail.com> Date: Sat, 15 Aug 2026 16:29:27 +0800 Subject: [PATCH] runtime/pprof: avoid panic for sigprofNonGoPC short stacks sigprofNonGoPC produces a short stack {pc, _ExternalCode} when a SIGPROF lands while m.isExtraInC is true. This happens in a race window in cgocallbackg: when a C thread reuses an extra M (g != nil, needm skipped), isExtraInC is still true from the previous callback return, and is only cleared after exitsyscall returns. If that PC was previously sampled in a normal Go context, appendLocsForStack has cached an inlined expansion (l.pcs) longer than the short stack, and panics with "stack too short to match cached location". The panic fires in the runtime-spawned profileWriter goroutine, which user code cannot recover. In -buildmode=c-shared binaries the runtime forces tracebackCrash, so it becomes SIGABRT, crashing the host process. Fix: clear isExtraInC inside exitsyscall, right after the goroutine transitions to _Grunning and before acquiring a P. This eliminates the race window. For non-cgo syscall exits, isExtraInC is already false, so this is a no-op. The redundant clear in cgocallbackg is removed. As a defensive measure, appendLocsForStack returns locs instead of panicking if len(l.pcs) > len(stk), so any future similar race degrades gracefully. TestIssue70529 in proto_test.go reproduces the panic deterministically by feeding profileBuilder a long-stack sample followed by a short-stack sample. TestCgoCallbackPprofRace exercises the scenario with real C-to-Go callbacks under continuous CPU profiling. Updates #70529 --- src/runtime/cgocall.go | 6 +- src/runtime/crash_cgo_test.go | 28 +++++++ src/runtime/pprof/proto.go | 16 +++- src/runtime/pprof/proto_test.go | 57 ++++++++++++++ src/runtime/proc.go | 17 ++++ .../testprogcgo/callback_pprof_race.go | 77 +++++++++++++++++++ 6 files changed, 196 insertions(+), 5 deletions(-) create mode 100644 src/runtime/testdata/testprogcgo/callback_pprof_race.go 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