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 <markfreeman@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

diff --git a/src/cmd/compile/internal/types2/named.go b/src/cmd/compile/internal/types2/named.go
index 26be3104b8c0bb5b378f4363ed2214ad509eecb4..a75ca75d0cdd953aba9d590f579454839067f964 100644 (file)
--- 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 79f6fc6a494160ddedd0a640b715ac9549697f72..7f0e6f4904e0fd126a7221ab413b970749f43d1c 100644 (file)
--- 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)
        }