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]) + } + } +}