diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index d80307e5f..7b451a0b7 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -1170,7 +1170,7 @@ func (c *Conn) derpWriteChanOfAddr(addr netaddr.IPPort, peer key.Public) chan<- return nil } - // Note that derphttp.NewClient does not dial the server + // Note that derphttp.NewRegionClient does not dial the server // so it is safe to do under the mu lock. dc := derphttp.NewRegionClient(c.privateKey, c.logf, func() *tailcfg.DERPRegion { if c.connCtx.Err() != nil { diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index 7aac0572a..1609de008 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -11,6 +11,7 @@ import ( "crypto/tls" "encoding/binary" "encoding/json" + "errors" "fmt" "io/ioutil" "net" @@ -542,6 +543,88 @@ func TestDeviceStartStop(t *testing.T) { dev.Close() } +type testConnClosingContext struct { + parent context.Context + mu *sync.Mutex +} + +func (c *testConnClosingContext) Deadline() (deadline time.Time, ok bool) { + d, o := c.parent.Deadline() + return d, o +} +func (c *testConnClosingContext) Done() <-chan struct{} { + return c.parent.Done() +} +func (c *testConnClosingContext) Err() error { + // Deliberately deadlock if anything grabs the lock after checking Err() + c.mu.Lock() + return errors.New("testConnClosingContext error") +} +func (c *testConnClosingContext) Value(key interface{}) interface{} { + return c.parent.Value(key) +} +func (*testConnClosingContext) String() string { + return "testConnClosingContext" +} + +func TestConnClosing(t *testing.T) { + privateKey, err := wgkey.NewPrivate() + if err != nil { + t.Fatalf("generating private key: %v", err) + } + + epCh := make(chan []string, 100) + conn, err := NewConn(Options{ + Logf: t.Logf, + PacketListener: nettype.Std{}, + EndpointsFunc: func(eps []string) { + epCh <- eps + }, + SimulatedNetwork: false, + }) + if err != nil { + t.Fatalf("constructing magicsock: %v", err) + } + + derpMap, cleanup := runDERPAndStun(t, t.Logf, nettype.Std{}, netaddr.IPv4(127, 0, 3, 1)) + defer cleanup() + + // The point of this test case is to exercise handling in derpWriteChanOfAddr() which + // returns early if connCtx.Err() returns non-nil, to avoid a deadlock on conn.mu. + // We swap in a context which always returns an error, and deliberately grabs the lock + // to cause a deadlock if magicsock.go tries to acquire the lock after calling Err(). + closingCtx := testConnClosingContext{parent: conn.connCtx, mu: &conn.mu} + conn.connCtx = &closingCtx + conn.Start() + + conn.SetDERPMap(derpMap) + if err := conn.SetPrivateKey(privateKey); err != nil { + t.Fatalf("setting private key in magicsock: %v", err) + } + + tun := tuntest.NewChannelTUN() + tsTun := tstun.WrapTUN(t.Logf, tun.TUN()) + tsTun.SetFilter(filter.NewAllowAllForTest(t.Logf)) + + dev := device.NewDevice(tsTun, &device.DeviceOptions{ + Logger: &device.Logger{ + Debug: logger.StdLogger(t.Logf), + Info: logger.StdLogger(t.Logf), + Error: logger.StdLogger(t.Logf), + }, + CreateEndpoint: conn.CreateEndpoint, + CreateBind: conn.CreateBind, + SkipBindUpdate: true, + }) + + dev.Up() + conn.WaitReady(t) + + // We don't assert any failures within the test itself. If derpWriteChanOfAddr tries to + // grab the lock it will deadlock, and conn.WaitReady(t) will call t.Fatal() after timeout. + // (verified by deliberately breaking derpWriteChanOfAddr) +} + func makeNestable(t *testing.T) (logf logger.Logf, setT func(t *testing.T)) { var mu sync.RWMutex cur := t