Skip to content

Commit

Permalink
[release-branch.go1.23] runtime: fix GoroutineProfile stacks not gett…
Browse files Browse the repository at this point in the history
…ing null terminated

Fix a regression introduced in CL 572396 causing goroutine stacks not
getting null terminated.

This bug impacts callers that reuse the []StackRecord slice for multiple
calls to GoroutineProfile. See felixge/fgprof#33
for an example of the problem.

Add a test case to prevent similar regressions in the future. Use null
padding instead of null termination to be consistent with other profile
types and because it's less code to implement. Also fix the
ThreadCreateProfile code path.

Fixes golang#69258

Change-Id: I0b9414f6c694c304bc03a5682586f619e9bf0588
Reviewed-on: https://go-review.googlesource.com/c/go/+/609815
Reviewed-by: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
(cherry picked from commit 49e542a)
Reviewed-on: https://go-review.googlesource.com/c/go/+/621277
Reviewed-by: Cherry Mui <cherryyz@google.com>
  • Loading branch information
felixge authored and prattmic committed Oct 21, 2024
1 parent 6495ce0 commit 35c010a
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 12 deletions.
6 changes: 4 additions & 2 deletions src/runtime/mprof.go
Original file line number Diff line number Diff line change
Expand Up @@ -1270,7 +1270,8 @@ func pprof_mutexProfileInternal(p []profilerecord.BlockProfileRecord) (n int, ok
// of calling ThreadCreateProfile directly.
func ThreadCreateProfile(p []StackRecord) (n int, ok bool) {
return threadCreateProfileInternal(len(p), func(r profilerecord.StackRecord) {
copy(p[0].Stack0[:], r.Stack)
i := copy(p[0].Stack0[:], r.Stack)
clear(p[0].Stack0[i:])
p = p[1:]
})
}
Expand Down Expand Up @@ -1649,7 +1650,8 @@ func GoroutineProfile(p []StackRecord) (n int, ok bool) {
return
}
for i, mr := range records[0:n] {
copy(p[i].Stack0[:], mr.Stack)
l := copy(p[i].Stack0[:], mr.Stack)
clear(p[i].Stack0[l:])
}
return
}
Expand Down
92 changes: 82 additions & 10 deletions src/runtime/pprof/pprof_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2441,16 +2441,7 @@ func TestTimeVDSO(t *testing.T) {
}

func TestProfilerStackDepth(t *testing.T) {
// Disable sampling, otherwise it's difficult to assert anything.
oldMemRate := runtime.MemProfileRate
runtime.MemProfileRate = 1
runtime.SetBlockProfileRate(1)
oldMutexRate := runtime.SetMutexProfileFraction(1)
t.Cleanup(func() {
runtime.MemProfileRate = oldMemRate
runtime.SetBlockProfileRate(0)
runtime.SetMutexProfileFraction(oldMutexRate)
})
t.Cleanup(disableSampling())

const depth = 128
go produceProfileEvents(t, depth)
Expand Down Expand Up @@ -2742,3 +2733,84 @@ runtime/pprof.inlineA`,
})
}
}

func TestProfileRecordNullPadding(t *testing.T) {
// Produce events for the different profile types.
t.Cleanup(disableSampling())
memSink = make([]byte, 1) // MemProfile
<-time.After(time.Millisecond) // BlockProfile
blockMutex(t) // MutexProfile
runtime.GC()

// Test that all profile records are null padded.
testProfileRecordNullPadding(t, "MutexProfile", runtime.MutexProfile)
testProfileRecordNullPadding(t, "GoroutineProfile", runtime.GoroutineProfile)
testProfileRecordNullPadding(t, "BlockProfile", runtime.BlockProfile)
testProfileRecordNullPadding(t, "MemProfile/inUseZero=true", func(p []runtime.MemProfileRecord) (int, bool) {
return runtime.MemProfile(p, true)
})
testProfileRecordNullPadding(t, "MemProfile/inUseZero=false", func(p []runtime.MemProfileRecord) (int, bool) {
return runtime.MemProfile(p, false)
})
// Not testing ThreadCreateProfile because it is broken, see issue 6104.
}

func testProfileRecordNullPadding[T runtime.StackRecord | runtime.MemProfileRecord | runtime.BlockProfileRecord](t *testing.T, name string, fn func([]T) (int, bool)) {
stack0 := func(sr *T) *[32]uintptr {
switch t := any(sr).(type) {
case *runtime.StackRecord:
return &t.Stack0
case *runtime.MemProfileRecord:
return &t.Stack0
case *runtime.BlockProfileRecord:
return &t.Stack0
default:
panic(fmt.Sprintf("unexpected type %T", sr))
}
}

t.Run(name, func(t *testing.T) {
var p []T
for {
n, ok := fn(p)
if ok {
p = p[:n]
break
}
p = make([]T, n*2)
for i := range p {
s0 := stack0(&p[i])
for j := range s0 {
// Poison the Stack0 array to identify lack of zero padding
s0[j] = ^uintptr(0)
}
}
}

if len(p) == 0 {
t.Fatal("no records found")
}

for _, sr := range p {
for i, v := range stack0(&sr) {
if v == ^uintptr(0) {
t.Fatalf("record p[%d].Stack0 is not null padded: %+v", i, sr)
}
}
}
})
}

// disableSampling configures the profilers to capture all events, otherwise
// it's difficult to assert anything.
func disableSampling() func() {
oldMemRate := runtime.MemProfileRate
runtime.MemProfileRate = 1
runtime.SetBlockProfileRate(1)
oldMutexRate := runtime.SetMutexProfileFraction(1)
return func() {
runtime.MemProfileRate = oldMemRate
runtime.SetBlockProfileRate(0)
runtime.SetMutexProfileFraction(oldMutexRate)
}
}

0 comments on commit 35c010a

Please sign in to comment.