From 9b32127726740d10dc18eddc5cacac50e7f3dc1d Mon Sep 17 00:00:00 2001 From: James Tucker Date: Fri, 9 Oct 2026 20:27:20 +0000 Subject: [PATCH 1/2] ruby: regenerate Gemfile.lock rake 13.4.2, "ruby" platform entry, CHECKSUMS section, BUNDLED WITH 4.0.22. --- ruby/Gemfile.lock | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/ruby/Gemfile.lock b/ruby/Gemfile.lock index 6b17382..38e7234 100644 --- a/ruby/Gemfile.lock +++ b/ruby/Gemfile.lock @@ -11,13 +11,13 @@ GEM base64 (0.3.0) ffi (1.15.5) minitest (5.27.0) - rake (13.3.1) + rake (13.4.2) rake-compiler (1.2.9) rake PLATFORMS arm64-darwin-25 - x86_64-linux-gnu + ruby DEPENDENCIES base64 (~> 0.3) @@ -26,5 +26,14 @@ DEPENDENCIES rake-compiler (~> 1.2.1) tailscale! +CHECKSUMS + base64 (0.3.0) sha256=27337aeabad6ffae05c265c450490628ef3ebd4b67be58257393227588f5a97b + bundler (4.0.22) sha256=d8d5ec84c8555e0af71db63ed7aee4d1a8fb839ec46d84212d61979242a5d75a + ffi (1.15.5) sha256=6f2ed2fa68047962d6072b964420cba91d82ce6fa8ee251950c17fca6af3c2a0 + minitest (5.27.0) sha256=2d3b17f8a36fe7801c1adcffdbc38233b938eb0b4966e97a6739055a45fa77d5 + rake (13.4.2) sha256=cb825b2bd5f1f8e91ca37bddb4b9aaf345551b4731da62949be002fa89283701 + rake-compiler (1.2.9) sha256=5a3213a5dda977dfdf73e28beed6f4cd6a2cc86ac640bb662728eb7049a23607 + tailscale (0.1.0) + BUNDLED WITH - 2.4.1 + 4.0.22 From f32cf38f19f39c02ed4cbe8728b52fe2ac98a446 Mon Sep 17 00:00:00 2001 From: James Tucker Date: Fri, 9 Oct 2026 20:27:13 +0000 Subject: [PATCH 2/2] libtailscale: frame the listener fd-passing records 4e3b222 passes the remote address as variable-length sendmsg data next to the fd rights, but the length is nowhere in the protocol and the socketpair is a stream socket: nothing ties a read to exactly one record. Linux happens not to glue reads across skbs that carry fds, but that is an implementation detail, not something to build a protocol on, and a misframed record is undetectable. Frame the records explicitly. Fixed 64-byte NUL-padded records, one per connection, and TsnetAccept reads exactly one record per accept with MSG_WAITALL, asking only for the remaining bytes of the head record. The socket is a FIFO, so a read never straddles two records, under concurrent tailscale_accept calls or EINTR alike. The map stores the extracted IP string, so getremoteaddr is a plain copy. Also: - getremoteaddr now sets an error message before returning EBADF on a lookup miss; errmsg was left unset. - newConn's copy goroutines called r.Fd() after io.Copy, racing with connCleanup closing r in the other direction. Fd() on a closed os.File can return a since-reused number, so the Shutdown could hit an unrelated fd. Both copy directions now finish before connCleanup runs. - TailscaleKit: re-enable the remoteAddress assertion, disabled as flaky; the flakiness was the fd/address mixup. TestGetRemoteAddr alternates two client nodes over 200 connections, each announcing its own tailnet IP on the connection, and checks getremoteaddr against what the peer wrote. Fixes tailscale/tailscale#18310 Fixes tailscale/corp#49796 --- .../TailscaleKitTests.swift | 7 +- tailscale.go | 111 ++++++++--- tailscale_test.go | 7 + tsnetctest/tsnetctest.go | 187 ++++++++++++++++++ 4 files changed, 281 insertions(+), 31 deletions(-) diff --git a/swift/TailscaleKitXCTests/TailscaleKitTests.swift b/swift/TailscaleKitXCTests/TailscaleKitTests.swift index 8f6333e..8713f98 100644 --- a/swift/TailscaleKitXCTests/TailscaleKitTests.swift +++ b/swift/TailscaleKitXCTests/TailscaleKitTests.swift @@ -89,10 +89,9 @@ final class TailscaleKitTests: XCTestCase { let inbound = try await listener.accept() await listener.close() - // We can trust the backend here but this is slightly flaky since remoteAddress can be - // nil for legitimate reasons. - // let inboundIP = await inbound.remoteAddress - // XCTAssertEqual(inboundIP, writerAddr) + let inboundIP = await inbound.remoteAddress + let writerAddr = netType == .v4 ? ts2_addr.ip4 : ts2_addr.ip6.map { "[\($0)]" } + XCTAssertEqual(inboundIP, writerAddr) let got = try await inbound.receiveMessage(timeout: 2) print("got \(got)") diff --git a/tailscale.go b/tailscale.go index 1194630..ce7cb74 100644 --- a/tailscale.go +++ b/tailscale.go @@ -8,6 +8,7 @@ package main import "C" import ( + "bytes" "context" "encoding/json" "fmt" @@ -61,13 +62,16 @@ type listener struct { ln net.Listener fd int // go side fd of socketpair sent to C mu sync.Mutex - m map[C.int]net.Addr + // m maps the fd number C holds for an accepted connection (not our + // sender-side number; recvmsg installs a new one) to its remote IP. + m map[C.int]string } -type strAddr string - -func (s strAddr) Network() string { return "" } -func (s strAddr) String() string { return string(s) } +// listenAddrLen is the size of the fixed-size, NUL-padded record that +// carries each accepted connection's remote IP over the listener +// socketpair. Fixed size so TsnetAccept frames the stream with +// exact-size reads; 45 bytes covers the longest textual IP. +const listenAddrLen = 64 // conns tracks all the pipe(2)s allocated via tsnet_dial. var conns struct { @@ -247,7 +251,7 @@ func TsnetListen(sd C.int, network, addr *C.char, listenerOut *C.int) C.int { if listeners.m == nil { listeners.m = map[C.int]*listener{} } - listener := &listener{s: s, ln: ln, fd: sp, m: map[C.int]net.Addr{}} + listener := &listener{s: s, ln: ln, fd: sp, m: map[C.int]string{}} listeners.m[fdC] = listener listeners.mu.Unlock() @@ -289,17 +293,30 @@ func TsnetListen(sd C.int, network, addr *C.char, listenerOut *C.int) C.int { netConn.Close() continue } - addrBytes := []byte(netConn.RemoteAddr().String()) + ip := extractIP(netConn.RemoteAddr().String()) + if len(ip) >= listenAddrLen { + if s.s.Logf != nil { + s.s.Logf("libtailscale.accept: remote address %q does not fit in a %d-byte record", ip, listenAddrLen) + } + netConn.Close() + syscall.Close(int(connFd)) + continue + } + var addrRec [listenAddrLen]byte + copy(addrRec[:], ip) + rights := syscall.UnixRights(int(connFd)) - err = syscall.Sendmsg(sp, addrBytes, rights, nil, 0) + err = syscall.Sendmsg(sp, addrRec[:], rights, nil, 0) if err != nil { + // a failed sendmsg delivered nothing (sp being closed is + // handled by the read goroutine above) if s.s.Logf != nil { s.s.Logf("libtailscale.accept: sendmsg failed: %v", err) } netConn.Close() - // fallthrough to close connFd, then continue Accept()ing } - syscall.Close(int(connFd)) // sender's copy; receiver gets its own fd from recvmsg + + syscall.Close(int(connFd)) // now owned by recvmsg } }() @@ -317,14 +334,42 @@ func TsnetAccept(listenerFd C.int, connOut *C.int) C.int { return C.EBADF } - addrBuf := make([]byte, 256) - oobBuf := make([]byte, unix.CmsgLen(int(unsafe.Sizeof((C.int)(0))))) - n, oobn, _, _, err := syscall.Recvmsg(int(listenerFd), addrBuf, oobBuf, 0) - if err != nil { - return ln.s.recErr(err) + // One record per connection: the fd via SCM_RIGHTS plus its remote + // IP. The fd parsed below is the number the kernel installed, i.e. + // the number C will hold. + // + // The socketpair is a stream socket, so records are framed here: + // each read asks for the remaining bytes of the head record, with + // MSG_WAITALL. The FIFO never serves a later record's bytes first, + // so each accept consumes exactly one record, even concurrently or + // after EINTR. + data := make([]byte, listenAddrLen) + cbuf := make([]byte, unix.CmsgLen(int(unsafe.Sizeof((C.int)(0))))) + var n, oobn int + for n < len(data) { + buf := data[n:] + if n > 0 { + // the cmsg rode with the record's first byte; only the tail remains + cbuf = nil + } + nn, on, _, _, err := syscall.Recvmsg(int(listenerFd), buf, cbuf, syscall.MSG_WAITALL) + n += nn + if nn > 0 { + oobn = on + } + if err == syscall.EINTR { + continue // partial bytes stay record-aligned, keep draining + } + if err != nil { + return ln.s.recErr(err) + } + if nn == 0 { + // EOF mid-record: the listener was closed on the C side. + return ln.s.recErr(fmt.Errorf("libtailscale: listener closed mid-record: got %d of %d bytes", n, len(data))) + } } - scms, err := syscall.ParseSocketControlMessage(oobBuf[:oobn]) + scms, err := syscall.ParseSocketControlMessage(cbuf[:oobn]) if err != nil { return ln.s.recErr(err) } @@ -338,14 +383,18 @@ func TsnetAccept(listenerFd C.int, connOut *C.int) C.int { if len(fds) != 1 { return ln.s.recErr(fmt.Errorf("libtailscale: got %d FDs, want 1", len(fds))) } - fd := (C.int)(fds[0]) - *connOut = fd + fd := C.int(fds[0]) - if n > 0 { - ln.mu.Lock() - ln.m[fd] = strAddr(string(addrBuf[:n])) - ln.mu.Unlock() + // the entry must exist before C can learn this fd number + addrLen := bytes.IndexByte(data, 0) + if addrLen < 0 { + addrLen = listenAddrLen } + ln.mu.Lock() + ln.m[fd] = string(data[:addrLen]) + ln.mu.Unlock() + + *connOut = fd return 0 } @@ -382,8 +431,12 @@ func newConn(s *server, netConn net.Conn, connOut *C.int) error { r.Close() netConn.Close() } + // the Shutdowns below must precede connCleanup: r.Fd() after Close + // can return a reused fd number. Wait for both copy directions. + var copies sync.WaitGroup + copies.Add(2) go func() { - defer connCleanup() + defer copies.Done() var b [1 << 16]byte io.CopyBuffer(r, netConn, b[:]) syscall.Shutdown(int(r.Fd()), syscall.SHUT_WR) @@ -392,7 +445,7 @@ func newConn(s *server, netConn net.Conn, connOut *C.int) error { } }() go func() { - defer connCleanup() + defer copies.Done() var b [1 << 16]byte io.CopyBuffer(netConn, r, b[:]) syscall.Shutdown(int(r.Fd()), syscall.SHUT_RD) @@ -400,6 +453,10 @@ func newConn(s *server, netConn net.Conn, connOut *C.int) error { cw.CloseWrite() } }() + go func() { + copies.Wait() + connCleanup() + }() *connOut = fdC return nil @@ -424,14 +481,14 @@ func TsnetGetRemoteAddr(listener C.int, conn C.int, buf *C.char, buflen C.size_t l.mu.Lock() defer l.mu.Unlock() - addr, ok := l.m[conn] + ip, ok := l.m[conn] if !ok { + // set errmsg too: EBADF with an empty message is not debuggable + l.s.lastErr = fmt.Sprintf("libtailscale: getremoteaddr: no remote address recorded for conn %d on listener %d", conn, listener) out[0] = '\x00' return C.EBADF } - ip := extractIP(addr.String()) - n := copy(out, ip) if n >= len(out) { out[len(out)-1] = '\x00' // always NUL-terminate diff --git a/tailscale_test.go b/tailscale_test.go index e79fed9..ee8e822 100644 --- a/tailscale_test.go +++ b/tailscale_test.go @@ -59,6 +59,13 @@ func TestConn(t *testing.T) { } } +// TestGetRemoteAddr checks that tailscale_getremoteaddr reports the +// right peer address for every accepted connection while fd numbers +// are being reused across connections (tailscale/tailscale#18310). +func TestGetRemoteAddr(t *testing.T) { + tsnetctest.RunTestGetRemoteAddr(t) +} + func TestExtractIP(t *testing.T) { ipv4 := "1.23.33.4:12343" ipv6 := "[1::2234::34fc::44]:56576" diff --git a/tsnetctest/tsnetctest.go b/tsnetctest/tsnetctest.go index a2d17df..ed4e5cc 100644 --- a/tsnetctest/tsnetctest.go +++ b/tsnetctest/tsnetctest.go @@ -133,6 +133,157 @@ int close_conn() { } return 0; } + +tailscale sa, sb, sc; +char* tmpsa = NULL; +char* tmpsb = NULL; +char* tmpsc = NULL; + +// ga_up starts a tsnet node with the shared control URL and state dir. +int ga_up(tailscale s, char* dir) { + if (tailscale_set_control_url(s, control_url) != 0) { + return set_err(s, 'u'); + } + if (tailscale_set_dir(s, dir) != 0) { + return set_err(s, 'v'); + } + if (tailscale_set_logfd(s, -1) != 0) { + return set_err(s, 'w'); + } + if (tailscale_up(s) != 0) { + return set_err(s, 'x'); + } + return 0; +} + +// ga_ip4 writes s's first (IPv4) tailnet address into buf as a +// NUL-terminated string. +int ga_ip4(tailscale s, char* buf, size_t buflen) { + int ret = tailscale_getips(s, buf, buflen); + if (ret != 0) { + return ret; + } + char* comma = strchr(buf, ','); + if (comma != NULL) { + *comma = '\0'; + } + return 0; +} + +// test_getremoteaddr exercises the accept path under fd-number reuse: +// two client nodes alternate dialing a server node, each announcing +// its own tailnet IP on the connection, and the server checks that +// tailscale_getremoteaddr reports exactly the announced address for +// every accepted connection. +int test_getremoteaddr() { + int ret; + char msg[256]; + char ipbuf[128]; + + if (err == NULL) { + err = calloc(errlen, 1); + } + + sa = tailscale_new(); + sb = tailscale_new(); + sc = tailscale_new(); + if ((ret = ga_up(sa, tmpsa)) != 0) return ret; + if ((ret = ga_up(sb, tmpsb)) != 0) return ret; + if ((ret = ga_up(sc, tmpsc)) != 0) return ret; + + char serverip[64]; + if ((ret = ga_ip4(sa, serverip, sizeof serverip)) != 0) { + return set_err(sa, 'd'); + } + char* ipb; + char* ipc; + if ((ret = ga_ip4(sb, ipbuf, sizeof ipbuf)) != 0) { + return set_err(sb, 'e'); + } + ipb = strdup(ipbuf); + if ((ret = ga_ip4(sc, ipbuf, sizeof ipbuf)) != 0) { + return set_err(sc, 'f'); + } + ipc = strdup(ipbuf); + + char serveraddr[80]; + snprintf(serveraddr, sizeof serveraddr, "%s:8181", serverip); + + tailscale_listener ln; + if ((ret = tailscale_listen(sa, "tcp", ":8181", &ln)) != 0) { + return set_err(sa, 'g'); + } + + for (int i = 0; i < 200; i++) { + tailscale client = (i % 2) ? sc : sb; + char* ip = (i % 2) ? ipc : ipb; + + tailscale_conn w; + if ((ret = tailscale_dial(client, "tcp", serveraddr, &w)) != 0) { + msg[0] = '\0'; + tailscale_errmsg(client, msg, sizeof msg - 1); + snprintf(err, errlen, "conn %d: dial: %s", i, msg); + return 1; + } + + size_t iplen = strlen(ip); + if (write(w, ip, iplen) != (ssize_t)iplen) { + snprintf(err, errlen, "conn %d: short write: errno %d (%s)", i, errno, strerror(errno)); + return 1; + } + + tailscale_conn r; + if ((ret = tailscale_accept(ln, &r)) != 0) { + msg[0] = '\0'; + tailscale_errmsg(sa, msg, sizeof msg - 1); + snprintf(err, errlen, "conn %d: accept: %s", i, msg); + return 1; + } + + char got[64] = {0}; + if ((ret = tailscale_getremoteaddr(ln, r, got, sizeof got)) != 0) { + msg[0] = '\0'; + tailscale_errmsg(sa, msg, sizeof msg - 1); + snprintf(err, errlen, "conn %d: getremoteaddr: %d (%s)", i, ret, msg); + return 1; + } + + char want[64] = {0}; + size_t off = 0; + while (off < iplen) { + ssize_t n = read(r, want + off, iplen - off); + if (n <= 0) { + snprintf(err, errlen, "conn %d: short read: %zd, errno %d (%s)", i, n, errno, strerror(errno)); + return 1; + } + off += n; + } + + if (strcmp(got, want) != 0) { + snprintf(err, errlen, "conn %d: getremoteaddr returned %s, want %s", i, got, want); + return 1; + } + + if (close(w) != 0 || close(r) != 0) { + snprintf(err, errlen, "conn %d: close: errno %d (%s)", i, errno, strerror(errno)); + return 1; + } + } + + if ((ret = close(ln)) != 0) { + snprintf(err, errlen, "close listener: errno %d (%s)", errno, strerror(errno)); + return 1; + } + + free(ipb); + free(ipc); + + if (tailscale_close(sc) != 0 || tailscale_close(sb) != 0 || tailscale_close(sa) != 0) { + snprintf(err, errlen, "close nodes failed"); + return 1; + } + return 0; +} */ import "C" import ( @@ -217,3 +368,39 @@ func RunTestConn(t *testing.T) { t.Fatal(C.GoString(C.err)) } } + +// RunTestGetRemoteAddr runs the C-side test_getremoteaddr. +func RunTestGetRemoteAddr(t *testing.T) { + // Corp#4520: don't use netns for tests. + netns.SetEnabled(false) + t.Cleanup(func() { + netns.SetEnabled(true) + }) + + derpLogf := logger.Discard + if *verboseDERP { + derpLogf = t.Logf + } + derpMap := integration.RunDERPAndSTUN(t, derpLogf, "127.0.0.1") + control := &testcontrol.Server{ + DERPMap: derpMap, + } + control.HTTPTestServer = httptest.NewUnstartedServer(control) + control.HTTPTestServer.Start() + t.Cleanup(control.HTTPTestServer.Close) + t.Logf("testcontrol listening on %s", control.HTTPTestServer.URL) + + C.control_url = C.CString(control.HTTPTestServer.URL) + + tmp := t.TempDir() + for _, d := range []string{"ga", "gb", "gc"} { + os.MkdirAll(filepath.Join(tmp, d), 0755) + } + C.tmpsa = C.CString(filepath.Join(tmp, "ga")) + C.tmpsb = C.CString(filepath.Join(tmp, "gb")) + C.tmpsc = C.CString(filepath.Join(tmp, "gc")) + + if C.test_getremoteaddr() != 0 { + t.Fatal(C.GoString(C.err)) + } +}