From f8c3310a4570a1ac744300724e8a89b0f36f3131 Mon Sep 17 00:00:00 2001 From: James Yong Date: Sun, 25 Oct 2026 03:10:55 -0700 Subject: [PATCH] libtailscale: key connection cleanup by the Go-owned socket newConn registered each connection in conns under fds[0], the C side of its socketpair, and connCleanup only acts if the entry under that number still belongs to it. TsnetListen closes fds[0] right after passing it to C with SCM_RIGHTS, so the next accepted connection's socketpair usually gets the same number and overwrites the entry while the first connection is still alive. When the first connection ends, its connCleanup finds the newer entry and returns: its net.Conn is never closed, and its end of the socketpair is only closed once the os.File is garbage collected. conns still drains to empty, so TestConn cannot see it. TsnetDial has the same race when C closes its fd and the number is reused before cleanup runs. Key the entry by fds[1] instead. The Go side keeps that fd open until connCleanup deletes the entry and closes it, so its number cannot be reused while the connection is alive. newConn now returns the C fd instead of writing through a *C.int, so the new test can call it directly (test files cannot use cgo types). TestConnCleanupAfterFDReuse fails on main and passes with this change. Fixes tailscale/tailscale#21934 Co-Authored-By: Claude Opus 5.5 --- tailscale.go | 27 +++++++++++++++---------- tailscale_test.go | 50 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 10 deletions(-) diff --git a/tailscale.go b/tailscale.go index ce7cb74..af0ab5e 100644 --- a/tailscale.go +++ b/tailscale.go @@ -285,8 +285,8 @@ func TsnetListen(sd C.int, network, addr *C.char, listenerOut *C.int) C.int { if err != nil { return } - var connFd C.int - if err := newConn(s, netConn, &connFd); err != nil { + connFd, err := newConn(s, netConn) + if err != nil { if s.s.Logf != nil { s.s.Logf("libtailscale.accept: newConn: %v", err) } @@ -399,27 +399,33 @@ func TsnetAccept(listenerFd C.int, connOut *C.int) C.int { return 0 } -func newConn(s *server, netConn net.Conn, connOut *C.int) error { +// newConn pumps netConn through a socketpair and returns the C side. +func newConn(s *server, netConn net.Conn) (C.int, error) { fds, err := syscall.Socketpair(syscall.AF_LOCAL, syscall.SOCK_STREAM, 0) if err != nil { - return err + return -1, err } r := os.NewFile(uintptr(fds[1]), "socketpair-r") c := &conn{s: s.s, c: netConn, r: r} fdC := C.int(fds[0]) + // Key cleanup by the Go-owned end: it stays open until connCleanup, so + // its number cannot be reused while this connection is alive. fdC is + // handed to C, which may close it at any time (TsnetListen closes it + // right after sendmsg), and its number is then often reused at once. + fdGo := C.int(fds[1]) conns.mu.Lock() if conns.m == nil { conns.m = make(map[C.int]*conn) } - conns.m[fdC] = c + conns.m[fdGo] = c conns.mu.Unlock() connCleanup := func() { var inCleanup bool conns.mu.Lock() - if tsConn, ok := conns.m[fdC]; ok && tsConn.c == netConn { - delete(conns.m, fdC) + if tsConn, ok := conns.m[fdGo]; ok && tsConn.c == netConn { + delete(conns.m, fdGo) inCleanup = true } conns.mu.Unlock() @@ -458,8 +464,7 @@ func newConn(s *server, netConn net.Conn, connOut *C.int) error { connCleanup() }() - *connOut = fdC - return nil + return fdC, nil } //export TsnetGetRemoteAddr @@ -516,9 +521,11 @@ func TsnetDial(sd C.int, network, addr *C.char, connOut *C.int) C.int { return s.recErr(err) } s.started = true - if err := newConn(s, netConn, connOut); err != nil { + fd, err := newConn(s, netConn) + if err != nil { return s.recErr(err) } + *connOut = fd return 0 } diff --git a/tailscale_test.go b/tailscale_test.go index ee8e822..e2773c2 100644 --- a/tailscale_test.go +++ b/tailscale_test.go @@ -1,6 +1,9 @@ package main import ( + "net" + "sync" + "syscall" "testing" "time" @@ -84,3 +87,50 @@ func TestExtractIP(t *testing.T) { t.Errorf("ipv6 port stripping failed %s != %s", got6, want6) } } + +type closeTrackingConn struct { + net.Conn + once sync.Once + closed chan struct{} +} + +func (c *closeTrackingConn) Close() error { + c.once.Do(func() { close(c.closed) }) + return c.Conn.Close() +} + +// TestConnCleanupAfterFDReuse checks that every connection is cleaned up +// when the number of its C-side fd is reused while it is still alive. +// TsnetListen closes that fd right after passing it to C, so the next +// connection's socketpair usually gets the same number. +func TestConnCleanupAfterFDReuse(t *testing.T) { + const n = 8 + var tracked []*closeTrackingConn + var peers []net.Conn + var fds []int + for range n { + c, peer := net.Pipe() + tc := &closeTrackingConn{Conn: c, closed: make(chan struct{})} + tracked = append(tracked, tc) + peers = append(peers, peer) + fd, err := newConn(&server{}, tc) + if err != nil { + t.Fatal(err) + } + fds = append(fds, int(fd)) + syscall.Close(int(fd)) + } + t.Logf("C-side fds: %v", fds) + + for _, peer := range peers { + peer.Close() + } + timeout := time.After(5 * time.Second) + for i, tc := range tracked { + select { + case <-tc.closed: + case <-timeout: + t.Fatalf("connection %d (C-side fd %d) was never closed", i, fds[i]) + } + } +}