mirrors
/
tailscale
mirror of https://github.com/tailscale/tailscale.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

wgengine/magicsock: fix data race in ReceiveIPv4. 

The UDP reader goroutine was clobbering `n` and `err` from the
main goroutine, whose accesses are not synchronized the way `b` is.

Signed-off-by: David Anderson <danderson@tailscale.com>
This commit is contained in:
David Anderson 2020-03-06 20:39:40 -08:00
parent 77354d4617
commit f265603110
1 changed files with 17 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
wgengine/magicsock/magicsock.go
View File

@ -830,8 +830,7 @@ func (c *Conn) ReceiveIPv4(b []byte) (n int, ep conn.Endpoint, addr *net.UDPAddr
go func() {
// Read a packet, and process any STUN packets before returning.
for {
var pAddr net.Addr
n, pAddr, err = c.pconn.ReadFrom(b)
n, pAddr, err := c.pconn.ReadFrom(b)
if err != nil {
select {
case c.udpRecvCh <- udpReadResult{err: err}:
@ -854,6 +853,10 @@ func (c *Conn) ReceiveIPv4(b []byte) (n int, ep conn.Endpoint, addr *net.UDPAddr
}
}()
// Once the above goroutine has started, it owns b until it writes
// to udpRecvCh. The code below must not access b until it's
// completed a successful receive on udpRecvCh.
var addrSet *AddrSet
select {
@ -863,13 +866,18 @@ func (c *Conn) ReceiveIPv4(b []byte) (n int, ep conn.Endpoint, addr *net.UDPAddr
select {
case <-c.udpRecvCh:
// It's likely an error, since we just canceled the read.
// But there's a small window where the pconn.ReadFrom could've
// succeeded but not yet sent, and we got into the derp recv path
// first. In that case this udpReadResult is a real non-err packet
// and we need to choose which to use. Currently, arbitrarily, we currently
// select DERP and discard this result entirely.
// The main point of this receive, though, is to make sure that the goroutine
// is done with our b []byte buf.
// But there's a small window where the pconn.ReadFrom
// could've succeeded but not yet sent, and we got into
// the derp recv path first. In that case this
// udpReadResult is a real non-err packet and we need to
// choose which to use. Currently, arbitrarily, we
// currently select DERP and discard this result entirely.
//
// TODO(danderson): don't just discard packets here, it
// makes the stack unreliable and harder to test.
//
// The main point of this receive, though, is to make sure
// that the goroutine is done with our b []byte buf.
c.pconn.SetReadDeadline(time.Time{})
case <-c.donec():
return 0, nil, nil, errors.New("Conn closed")