From 980badbcecc2adc49b74e3ba5090b4cad40542d0 Mon Sep 17 00:00:00 2001
From: David Crawshaw <crawshaw@tailscale.com>
Date: Tue, 10 Mar 2020 22:23:10 -0400
Subject: [PATCH] wgengine/magicsock: dial derp without holding send lock

Dialing derp was blocking conn.Send, which bubbled up into
wireguard-go, which expects Send to behave like writing to
a typical socket with no blocking. The result fired the
watchdog timer in wgengine.

Instead, create the buffer channel for the derp server and
return it immediately, dialing on another goroutine.

Fixes #137

Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
---
 wgengine/magicsock/magicsock.go | 59 +++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 18 deletions(-)

diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go
index fd0b091d1..3713c5273 100644
--- a/wgengine/magicsock/magicsock.go
+++ b/wgengine/magicsock/magicsock.go
@@ -111,7 +111,7 @@ var derpMagicIP = net.ParseIP(DerpMagicIP).To4()
 
 // activeDerp contains fields for an active DERP connection.
 type activeDerp struct {
-	c         *derphttp.Client
+	c         *derphttp.Client // Conn.derpMu must be held
 	cancel    context.CancelFunc
 	writeCh   chan<- derpWriteRequest
 	lastWrite *time.Time
@@ -412,7 +412,9 @@ func (c *Conn) setNearestDERP(derpNum int) (wantDERP bool) {
 	c.myDerp = derpNum
 	c.logf("home DERP server is now %v, %v", derpNum, c.derps.ServerByID(derpNum))
 	for i, ad := range c.activeDerp {
-		go ad.c.NotePreferred(i == c.myDerp)
+		if ad.c != nil {
+			go ad.c.NotePreferred(i == c.myDerp)
+		}
 	}
 	if derpNum != 0 && derpNum != c.myDerp {
 		// On change, start connecting to it:
@@ -707,32 +709,51 @@ func (c *Conn) derpWriteChanOfAddr(addr *net.UDPAddr) chan<- derpWriteRequest {
 			return nil
 		}
 
-		// TODO(bradfitz): don't hold derpMu here. It's slow. Release first and use singleflight to dial+re-lock to add.
-		dc, err := derphttp.NewClient(c.privateKey, "https://"+derpSrv.HostHTTPS+"/derp", c.logf)
-		if err != nil {
-			c.logf("derphttp.NewClient: port %d, host %q invalid? err: %v", addr.Port, derpSrv.HostHTTPS, err)
-			return nil
-		}
-		dc.NotePreferred(c.myDerp == addr.Port)
-		dc.DNSCache = dnscache.Get()
-		dc.TLSConfig = c.derpTLSConfig
-
 		ctx, cancel := context.WithCancel(context.Background())
 		ch := make(chan derpWriteRequest, bufferedDerpWritesBeforeDrop)
-
-		ad.c = dc
 		ad.writeCh = ch
 		ad.cancel = cancel
 		ad.lastWrite = new(time.Time)
 		c.activeDerp[addr.Port] = ad
-
-		go c.runDerpReader(ctx, addr, dc)
-		go c.runDerpWriter(ctx, addr, dc, ch)
+		go c.dialDerp(ctx, addr, &ad, ch, derpSrv)
 	}
 	*ad.lastWrite = time.Now()
 	return ad.writeCh
 }
 
+func (c *Conn) dialDerp(ctx context.Context, addr *net.UDPAddr, ad *activeDerp, ch chan derpWriteRequest, derpSrv *derpmap.Server) {
+	dc, err := derphttp.NewClient(c.privateKey, "https://"+derpSrv.HostHTTPS+"/derp", c.logf)
+	if err != nil {
+		c.logf("derphttp.NewClient: port %d, host %q invalid? err: %v", addr.Port, derpSrv.HostHTTPS, err)
+
+		c.derpMu.Lock()
+		if ctx.Err() == nil {
+			ad.cancel()
+			delete(c.activeDerp, addr.Port)
+		}
+		c.derpMu.Unlock()
+
+		return
+	}
+	dc.NotePreferred(c.myDerp == addr.Port)
+	dc.DNSCache = dnscache.Get()
+	dc.TLSConfig = c.derpTLSConfig
+
+	c.derpMu.Lock()
+	if ctx.Err() != nil {
+		// Some other dialDerp beat us to the punch.
+		c.derpMu.Unlock()
+		dc.Close()
+		return
+	}
+	ad.c = dc
+	c.activeDerp[addr.Port] = *ad
+	c.derpMu.Unlock()
+
+	go c.runDerpReader(ctx, addr, dc)
+	go c.runDerpWriter(ctx, addr, dc, ch)
+}
+
 // derpReadResult is the type sent by runDerpClient to ReceiveIPv4
 // when a DERP packet is available.
 //
@@ -1035,7 +1056,9 @@ func (c *Conn) closeAllDerpLocked() {
 func (c *Conn) closeDerpLocked(node int) {
 	if ad, ok := c.activeDerp[node]; ok {
 		c.logf("closing connection to derp%v", node)
-		go ad.c.Close()
+		if ad.c != nil {
+			go ad.c.Close()
+		}
 		ad.cancel()
 		delete(c.activeDerp, node)
 	}