libtailscale: key connection cleanup by the Go-owned socket

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
 }
 

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