internal/impl: fix data race in atomicNilMessage

The problem is that atomicNilMessage.m.mi is accessed both by atomic and
non-atomic operations. (Init uses an atomic read to verify that m.mi is
non-nil, but then returns a non-atomic m.)

Race condition is demonstrated by this test with
"go test -race -count=1000":

	func TestPointer(t *testing.T) {
		var m atomicNilMessage
		var mi MessageInfo
		ch := make(chan *MessageInfo)
		for i := 0; i < 20; i++ {
			go func() {
				r := m.Init(&mi)
				if &mi != r.mi {
					// This conditional exists just
					// ensure r.mi is touched.
					t.Error("mismatch")
				}
				ch <- r.mi
			}()
		}
		for i := 0; i < 20; i++ {
			<-ch
		}
	}

I chose not to add the test since it seems a bit overfit to the specific
situation.

Change-Id: Id4664ef3cd5b29515ed310851b9aeb7561be30d0
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/188337
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
This commit is contained in:
Damien Neil 2019-07-31 09:29:19 -07:00
parent 45f14b4bdc
commit a6af044c3f

View File

@ -154,11 +154,13 @@ func (ms *messageState) StoreMessageInfo(mi *MessageInfo) {
atomic.StorePointer((*unsafe.Pointer)(unsafe.Pointer(&ms.mi)), unsafe.Pointer(mi))
}
type atomicNilMessage struct{ m messageReflectWrapper }
type atomicNilMessage struct{ p unsafe.Pointer } // p is a *messageReflectWrapper
func (m *atomicNilMessage) Init(mi *MessageInfo) *messageReflectWrapper {
if (*messageReflectWrapper)(atomic.LoadPointer((*unsafe.Pointer)(unsafe.Pointer(&m.m.mi)))) == nil {
atomic.StorePointer((*unsafe.Pointer)(unsafe.Pointer(&m.m.mi)), unsafe.Pointer(mi))
if p := atomic.LoadPointer(&m.p); p != nil {
return (*messageReflectWrapper)(p)
}
return &m.m
w := &messageReflectWrapper{mi: mi}
atomic.CompareAndSwapPointer(&m.p, nil, (unsafe.Pointer)(w))
return (*messageReflectWrapper)(atomic.LoadPointer(&m.p))
}