From 3073f3f9411737de2232e6f6d634c118b53aed22 Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Tue, 14 Nov 2023 18:56:21 +0000 Subject: [PATCH] runtime: prevent send on closed channel in wakeableSleep Currently wakeableSleep has a race where, although stopTimer is called, the timer could be queued already and fire *after* the wakeup channel is closed. Fix this by protecting wakeup with a lock used on the close and wake paths and assigning the wakeup to nil on close. The wake path then ignores a nil wakeup channel. This fixes the problem by ensuring that a failure to stop the timer only results in the timer doing nothing, rather than trying to send on a closed channel. The addition of this lock requires some changes to the static lock ranking system. Thiere's also a second problem here: the timer could be delayed far enough into the future that when it fires, it observes a non-nil wakeup if the wakeableSleep has been re-initialized and reset. Fix this problem too by allocating the wakeableSleep on the heap and creating a new one instead of reinitializing the old one. The GC will make sure that the reference to the old one stays alive for the timer to fire, but that timer firing won't cause a spurious wakeup in the new one. Change-Id: I2b979304e755c015d4466991f135396f6a271069 Reviewed-on: https://go-review.googlesource.com/c/go/+/542335 Reviewed-by: Michael Pratt Commit-Queue: Michael Knyszek Auto-Submit: Michael Knyszek TryBot-Result: Gopher Robot Run-TryBot: Michael Knyszek --- src/runtime/lockrank.go | 3 ++ src/runtime/mklockrank.go | 1 + src/runtime/trace2.go | 62 +++++++++++++++++++++++++++------------ src/runtime/trace2cpu.go | 2 +- 4 files changed, 49 insertions(+), 19 deletions(-) diff --git a/src/runtime/lockrank.go b/src/runtime/lockrank.go index 4d661e93dc..103131df5e 100644 --- a/src/runtime/lockrank.go +++ b/src/runtime/lockrank.go @@ -24,6 +24,7 @@ const ( lockRankAllg lockRankAllp lockRankTimers + lockRankWakeableSleep lockRankNetpollInit lockRankHchan lockRankNotifyList @@ -84,6 +85,7 @@ var lockNames = []string{ lockRankAllg: "allg", lockRankAllp: "allp", lockRankTimers: "timers", + lockRankWakeableSleep: "wakeableSleep", lockRankNetpollInit: "netpollInit", lockRankHchan: "hchan", lockRankNotifyList: "notifyList", @@ -151,6 +153,7 @@ var lockPartialOrder [][]lockRank = [][]lockRank{ lockRankAllg: {lockRankSysmon, lockRankScavenge, lockRankForcegc, lockRankSweepWaiters, lockRankAssistQueue, lockRankSweep, lockRankPollDesc, lockRankCpuprof, lockRankSched}, lockRankAllp: {lockRankSysmon, lockRankScavenge, lockRankForcegc, lockRankSweepWaiters, lockRankAssistQueue, lockRankSweep, lockRankPollDesc, lockRankCpuprof, lockRankSched}, lockRankTimers: {lockRankSysmon, lockRankScavenge, lockRankForcegc, lockRankSweepWaiters, lockRankAssistQueue, lockRankSweep, lockRankPollDesc, lockRankCpuprof, lockRankSched, lockRankAllp, lockRankTimers}, + lockRankWakeableSleep: {lockRankSysmon, lockRankScavenge, lockRankForcegc, lockRankSweepWaiters, lockRankAssistQueue, lockRankSweep, lockRankPollDesc, lockRankCpuprof, lockRankSched, lockRankAllp, lockRankTimers}, lockRankNetpollInit: {lockRankSysmon, lockRankScavenge, lockRankForcegc, lockRankSweepWaiters, lockRankAssistQueue, lockRankSweep, lockRankPollDesc, lockRankCpuprof, lockRankSched, lockRankAllp, lockRankTimers}, lockRankHchan: {lockRankSysmon, lockRankScavenge, lockRankSweep, lockRankHchan}, lockRankNotifyList: {}, diff --git a/src/runtime/mklockrank.go b/src/runtime/mklockrank.go index c0d5a02146..4cb232b1ba 100644 --- a/src/runtime/mklockrank.go +++ b/src/runtime/mklockrank.go @@ -64,6 +64,7 @@ assistQueue, < sched; sched < allg, allp; allp < timers; +timers < wakeableSleep; timers < netpollInit; # Channels diff --git a/src/runtime/trace2.go b/src/runtime/trace2.go index 7d95eeaaca..b0842d99ec 100644 --- a/src/runtime/trace2.go +++ b/src/runtime/trace2.go @@ -85,7 +85,7 @@ var trace struct { cpuLogRead *profBuf signalLock atomic.Uint32 // protects use of the following member, only usable in signal handlers cpuLogWrite atomic.Pointer[profBuf] // copy of cpuLogRead for use in signal handlers, set without signalLock - cpuSleep wakeableSleep + cpuSleep *wakeableSleep cpuLogDone <-chan struct{} cpuBuf [2]*traceBuf @@ -856,7 +856,7 @@ func traceReaderAvailable() *g { var traceAdvancer traceAdvancerState type traceAdvancerState struct { - timer wakeableSleep + timer *wakeableSleep done chan struct{} } @@ -864,7 +864,7 @@ type traceAdvancerState struct { func (s *traceAdvancerState) start() { // Start a goroutine to periodically advance the trace generation. s.done = make(chan struct{}) - s.timer.init() + s.timer = newWakeableSleep() go func() { for traceEnabled() { // Set a timer to wake us up @@ -895,50 +895,76 @@ const defaultTraceAdvancePeriod = 1e9 // 1 second. // close to free up resources. Once close is called, init // must be called before another use. type wakeableSleep struct { - timer *timer + timer *timer + + // lock protects access to wakeup, but not send/recv on it. + lock mutex wakeup chan struct{} } -// init initializes the timer. -func (s *wakeableSleep) init() { +// newWakeableSleep initializes a new wakeableSleep and returns it. +func newWakeableSleep() *wakeableSleep { + s := new(wakeableSleep) + lockInit(&s.lock, lockRankWakeableSleep) s.wakeup = make(chan struct{}, 1) s.timer = new(timer) s.timer.arg = s s.timer.f = func(s any, _ uintptr) { s.(*wakeableSleep).wake() } + return s } // sleep sleeps for the provided duration in nanoseconds or until // another goroutine calls wake. // -// Must not be called by more than one goroutine at a time. +// Must not be called by more than one goroutine at a time and +// must not be called concurrently with close. func (s *wakeableSleep) sleep(ns int64) { resetTimer(s.timer, nanotime()+ns) - <-s.wakeup + lock(&s.lock) + wakeup := s.wakeup + unlock(&s.lock) + <-wakeup stopTimer(s.timer) } // wake awakens any goroutine sleeping on the timer. // -// Safe for concurrent use. +// Safe for concurrent use with all other methods. func (s *wakeableSleep) wake() { - // Non-blocking send. - // - // Others may also write to this channel and we don't - // want to block on the receiver waking up. This also - // effectively batches together wakeup notifications. - select { - case s.wakeup <- struct{}{}: - default: + // Grab the wakeup channel, which may be nil if we're + // racing with close. + lock(&s.lock) + if s.wakeup != nil { + // Non-blocking send. + // + // Others may also write to this channel and we don't + // want to block on the receiver waking up. This also + // effectively batches together wakeup notifications. + select { + case s.wakeup <- struct{}{}: + default: + } } + unlock(&s.lock) } // close wakes any goroutine sleeping on the timer and prevents // further sleeping on it. // +// Once close is called, the wakeableSleep must no longer be used. +// // It must only be called once no goroutine is sleeping on the // timer *and* nothing else will call wake concurrently. func (s *wakeableSleep) close() { - close(s.wakeup) + // Set wakeup to nil so that a late timer ends up being a no-op. + lock(&s.lock) + wakeup := s.wakeup + s.wakeup = nil + + // Close the channel. + close(wakeup) + unlock(&s.lock) + return } diff --git a/src/runtime/trace2cpu.go b/src/runtime/trace2cpu.go index 4c9bad434c..a33c0b6b6d 100644 --- a/src/runtime/trace2cpu.go +++ b/src/runtime/trace2cpu.go @@ -37,7 +37,7 @@ func traceStartReadCPU() { throw("traceStartReadCPU called with trace disabled") } // Spin up the logger goroutine. - trace.cpuSleep.init() + trace.cpuSleep = newWakeableSleep() done := make(chan struct{}, 1) go func() { for traceEnabled() { -- 2.44.0