This is an automated email from the ASF dual-hosted git repository. sjanc pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mynewt-newtmgr.git
commit 845be2c7310a4b24986465ab95a6e20b0e7fe984 Author: Szymon Janc <[email protected]> AuthorDate: Thu Oct 22 00:31:44 2026 +0200 nmxact/nmble: Fix race between listener timeout and Close The AfterTimeout callback read Acked and sent on TmoChan without any synchronization with Close(). If the timer fired while the listener was being closed, the callback could see Acked == false, then Close() closed TmoChan and the callback's send crashed newtmgr with "panic: send on closed channel". The race detector reports the unsynchronized Acked access as well; Close() itself carried a comment about this race. Protect the timer and a closed flag with a mutex shared by the callback and Close(), and make the callback's send non-blocking so it can never block while holding the lock. Owners still set Acked without locking; that remaining benign race on the flag is not addressed here. Assisted-by: Claude:claude-opus-5-5 --- nmxact/nmble/dispatch_test.go | 11 +++++++++++ nmxact/nmble/listen.go | 29 +++++++++++++++++++++++------ 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/nmxact/nmble/dispatch_test.go b/nmxact/nmble/dispatch_test.go index d51403d..2c07aca 100644 --- a/nmxact/nmble/dispatch_test.go +++ b/nmxact/nmble/dispatch_test.go @@ -584,3 +584,14 @@ func TestDispatcherErrorAllDeliversError(t *testing.T) { t.Errorf("ErrChan not closed after the error") } } + +// The timer callback may run concurrently with Close. It must neither +// race with Close nor panic with "send on closed channel". The race is +// timing dependent, so try many times. +func TestListenerTimeoutRacesClose(t *testing.T) { + for i := 0; i < 2000; i++ { + l := NewListener() + l.AfterTimeout(0) + l.Close() + } +} diff --git a/nmxact/nmble/listen.go b/nmxact/nmble/listen.go index 9954661..b074571 100644 --- a/nmxact/nmble/listen.go +++ b/nmxact/nmble/listen.go @@ -21,6 +21,7 @@ package nmble import ( "fmt" + "sync" "time" "mynewt.apache.org/newtmgr/nmxact/nmxutil" @@ -60,6 +61,10 @@ type Listener struct { Acked bool timer *time.Timer + + // Protects timer and closed against the timer callback. + mtx sync.Mutex + closed bool } func NewListener() *Listener { @@ -71,9 +76,20 @@ func NewListener() *Listener { } func (bl *Listener) AfterTimeout(tmo time.Duration) <-chan time.Time { + bl.mtx.Lock() + defer bl.mtx.Unlock() + fn := func() { - if !bl.Acked { - bl.TmoChan <- time.Now() + bl.mtx.Lock() + defer bl.mtx.Unlock() + + // The listener may have been closed after the timer fired. Never + // block here; a pending timeout is as good as a new one. + if !bl.closed && !bl.Acked { + select { + case bl.TmoChan <- time.Now(): + default: + } } } bl.timer = time.AfterFunc(tmo, fn) @@ -81,14 +97,15 @@ func (bl *Listener) AfterTimeout(tmo time.Duration) <-chan time.Time { } func (bl *Listener) Close() { - // This provokes a race condition. The timer may get initialized at any - // time. + bl.mtx.Lock() + defer bl.mtx.Unlock() + if bl.timer != nil { bl.timer.Stop() } - // Mark the command as acked in case the race condition mentioned above - // occurred. If the timer goes off, nothing will happen. + // If the timer already fired, its callback sees this and does nothing. + bl.closed = true bl.Acked = true close(bl.MsgChan)
