From cf826bfcb494a7dba5451bd7e4432f150590b19e Mon Sep 17 00:00:00 2001 From: Mark Freeman Date: Thu, 23 Oct 2025 12:52:19 -0400 Subject: [PATCH] go/types, types2: set t.underlying exactly once in resolveUnderlying While the locking in Named.resolveUnderlying is mostly fine, we do not perform an atomic read before the write to underlying. If resolveUnderlying returns and another thread was waiting on the lock, it can perform a second (in this case identical) write to t.underlying. A reader thread on n.underlying can thus observe either of those writes, tripping the race detector. Michael was kind enough to provide a diagram: T1 T2 1. t.stateHas(underlying) // false 2. t.stateHas(underlying) // false 3. t.mu.Lock() // acquired 4. t.mu.Lock() // blocked 5. t.underlying = u 6. t.setState(underlying) 7. t.mu.Unlock() 8. t.underlying = u // overwritten 9. t.setState(underlying) 10. t.mu.Unlock() Adding a second check before setting t.underlying prevents the write on line 8. Change-Id: Ia43a6d3ba751caef436b9926c6ece2a71dfb9d38 Reviewed-on: https://go-review.googlesource.com/c/go/+/714300 Auto-Submit: Mark Freeman Reviewed-by: Michael Pratt Reviewed-by: Robert Griesemer LUCI-TryBot-Result: Go LUCI --- src/cmd/compile/internal/types2/named.go | 7 +++++++ src/go/types/named.go | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/src/cmd/compile/internal/types2/named.go b/src/cmd/compile/internal/types2/named.go index 26be3104b8c..a75ca75d0cd 100644 --- a/src/cmd/compile/internal/types2/named.go +++ b/src/cmd/compile/internal/types2/named.go @@ -630,6 +630,7 @@ func (n *Named) resolveUnderlying() { t.resolve() t.mu.Lock() defer t.mu.Unlock() + assert(t.fromRHS != nil || t.allowNilRHS) rhs = t.fromRHS @@ -640,6 +641,12 @@ func (n *Named) resolveUnderlying() { // set underlying for all Named types in the chain for t := range seen { + // Careful, t.underlying has lock-free readers. Since we might be racing + // another call to resolveUnderlying, we have to avoid overwriting + // t.underlying. Otherwise, the race detector will be tripped. + if t.stateHas(underlying) { + continue + } t.underlying = u t.setState(underlying) } diff --git a/src/go/types/named.go b/src/go/types/named.go index 79f6fc6a494..7f0e6f4904e 100644 --- a/src/go/types/named.go +++ b/src/go/types/named.go @@ -633,6 +633,7 @@ func (n *Named) resolveUnderlying() { t.resolve() t.mu.Lock() defer t.mu.Unlock() + assert(t.fromRHS != nil || t.allowNilRHS) rhs = t.fromRHS @@ -643,6 +644,12 @@ func (n *Named) resolveUnderlying() { // set underlying for all Named types in the chain for t := range seen { + // Careful, t.underlying has lock-free readers. Since we might be racing + // another call to resolveUnderlying, we have to avoid overwriting + // t.underlying. Otherwise, the race detector will be tripped. + if t.stateHas(underlying) { + continue + } t.underlying = u t.setState(underlying) }