From effbe67fe3e30f48bd4b159ca9d37b1afea6052a Mon Sep 17 00:00:00 2001 From: Avery Pennarun Date: Wed, 15 Apr 2026 00:54:01 +0000 Subject: [PATCH] wgengine/magicsock: remove pickPort, use port 0 to avoid TOCTOU race pickPort would bind a UDP socket on :0 to get a free port, close the socket, then hope to rebind to the same port in NewConn. This is a TOCTOU race that can cause flaky test failures when another process grabs the port in between. Instead, pass Port: 0 to NewConn and let the OS assign the port atomically, then read back the assigned port via conn.LocalPort(). Fixes #19409 Change-Id: Ie44b599fb93c361e29a05f2171ad747c46f82b7a Co-authored-by: Brad Fitzpatrick Signed-off-by: Avery Pennarun --- wgengine/magicsock/magicsock_test.go | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index 412c5cf71..16d392e42 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -413,9 +413,11 @@ func TestNewConn(t *testing.T) { stunAddr, stunCleanupFn := stuntest.Serve(t) defer stunCleanupFn() - port := pickPort(t) + // Use port 0 to let the system assign a port, avoiding TOCTOU races + // from the previous pickPort approach which would close a socket and + // hope to rebind to the same port. conn, err := NewConn(Options{ - Port: port, + Port: 0, DisablePortMapper: true, EndpointsFunc: epFunc, Logf: t.Logf, @@ -427,6 +429,13 @@ func TestNewConn(t *testing.T) { t.Fatal(err) } defer conn.Close() + + // Get the actual port that was assigned + port := conn.LocalPort() + if port == 0 { + t.Fatal("LocalPort returned 0") + } + conn.SetDERPMap(stuntest.DERPMapOf(stunAddr.String())) conn.SetPrivateKey(key.NewNode()) @@ -462,16 +471,6 @@ collectEndpoints: } } -func pickPort(t testing.TB) uint16 { - t.Helper() - conn, err := net.ListenPacket("udp4", "127.0.0.1:0") - if err != nil { - t.Fatal(err) - } - defer conn.Close() - return uint16(conn.LocalAddr().(*net.UDPAddr).Port) -} - func TestPickDERPFallback(t *testing.T) { tstest.PanicOnLog() tstest.ResourceCheck(t) @@ -1470,7 +1469,6 @@ func Test32bitAlignment(t *testing.T) { // newTestConn returns a new Conn. func newTestConn(t testing.TB) *Conn { t.Helper() - port := pickPort(t) bus := eventbustest.NewBus(t) @@ -1487,7 +1485,7 @@ func newTestConn(t testing.TB) *Conn { Metrics: new(usermetric.Registry), DisablePortMapper: true, Logf: t.Logf, - Port: port, + Port: 0, TestOnlyPacketListener: localhostListener{}, EndpointsFunc: func(eps []tailcfg.Endpoint) { t.Logf("endpoints: %q", eps)