From 591be7f10be18b4b24250868fb61a93c2e5af3f4 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Fri, 23 Feb 2024 09:54:56 -0800 Subject: [PATCH] quic: fix UDP on big-endian Linux, tests on various architectures The following cmsgs contain a native-endian 32-bit integer: - IP_TOS, passed to sendmsg - IPV6_TCLASS, always IP_TOS received from recvmsg contains a single byte, because why not. We were inadvertently assuming little-endian integers in all cases. Add endianness conversion as appropriate. Disable tests that rely on IPv4-in-IPv6 mapped sockets on dragonfly and openbsd, which don't support this feature. (A "udp" socket cannot receive IPv6 packets on these platforms.) Disable IPv6 tests on wasm, where the simulated networking appears to generally not support IPv6. Fixes golang/go#65906 Fixes golang/go#65907 Change-Id: Ie50af12e182a1a5d685ce4fbdf008748f6aee339 Reviewed-on: https://go-review.googlesource.com/c/net/+/566296 LUCI-TryBot-Result: Go LUCI Reviewed-by: Jonathan Amsterdam Reviewed-by: Bryan Mills --- internal/quic/udp_darwin.go | 25 +++++++++++ internal/quic/udp_linux.go | 20 +++++++++ internal/quic/udp_msg.go | 89 ++++++++++++++++++------------------- internal/quic/udp_test.go | 17 +++++-- 4 files changed, 102 insertions(+), 49 deletions(-) diff --git a/internal/quic/udp_darwin.go b/internal/quic/udp_darwin.go index 3868a36a8..2eb2e9f9f 100644 --- a/internal/quic/udp_darwin.go +++ b/internal/quic/udp_darwin.go @@ -6,8 +6,33 @@ package quic +import ( + "encoding/binary" + + "golang.org/x/sys/unix" +) + // See udp.go. const ( udpECNSupport = true udpInvalidLocalAddrIsError = true ) + +// Confusingly, on Darwin the contents of the IP_TOS option differ depending on whether +// it is used as an inbound or outbound cmsg. + +func parseIPTOS(b []byte) (ecnBits, bool) { + // Single byte. The low two bits are the ECN field. + if len(b) != 1 { + return 0, false + } + return ecnBits(b[0] & ecnMask), true +} + +func appendCmsgECNv4(b []byte, ecn ecnBits) []byte { + // 32-bit integer. + // https://github.com/apple/darwin-xnu/blob/2ff845c2e033bd0ff64b5b6aa6063a1f8f65aa32/bsd/netinet/in_tclass.c#L1062-L1073 + b, data := appendCmsg(b, unix.IPPROTO_IP, unix.IP_TOS, 4) + binary.NativeEndian.PutUint32(data, uint32(ecn)) + return b +} diff --git a/internal/quic/udp_linux.go b/internal/quic/udp_linux.go index 2ba3e6f2f..6f191ed39 100644 --- a/internal/quic/udp_linux.go +++ b/internal/quic/udp_linux.go @@ -6,8 +6,28 @@ package quic +import ( + "golang.org/x/sys/unix" +) + // See udp.go. const ( udpECNSupport = true udpInvalidLocalAddrIsError = false ) + +// The IP_TOS socket option is a single byte containing the IP TOS field. +// The low two bits are the ECN field. + +func parseIPTOS(b []byte) (ecnBits, bool) { + if len(b) != 1 { + return 0, false + } + return ecnBits(b[0] & ecnMask), true +} + +func appendCmsgECNv4(b []byte, ecn ecnBits) []byte { + b, data := appendCmsg(b, unix.IPPROTO_IP, unix.IP_TOS, 1) + data[0] = byte(ecn) + return b +} diff --git a/internal/quic/udp_msg.go b/internal/quic/udp_msg.go index bdc1b710d..0b600a2b4 100644 --- a/internal/quic/udp_msg.go +++ b/internal/quic/udp_msg.go @@ -7,6 +7,7 @@ package quic import ( + "encoding/binary" "net" "net/netip" "sync" @@ -141,15 +142,11 @@ func parseControl(d *datagram, control []byte) { case unix.IPPROTO_IP: switch hdr.Type { case unix.IP_TOS, unix.IP_RECVTOS: - // Single byte containing the IP TOS field. - // The low two bits are the ECN field. - // // (Linux sets the type to IP_TOS, Darwin to IP_RECVTOS, - // jus check for both.) - if len(data) < 1 { - break + // just check for both.) + if ecn, ok := parseIPTOS(data); ok { + d.ecn = ecn } - d.ecn = ecnBits(data[0] & ecnMask) case unix.IP_PKTINFO: if a, ok := parseInPktinfo(data); ok { d.localAddr = netip.AddrPortFrom(a, d.localAddr.Port()) @@ -158,12 +155,11 @@ func parseControl(d *datagram, control []byte) { case unix.IPPROTO_IPV6: switch hdr.Type { case unix.IPV6_TCLASS: - // Single byte containing the traffic class field. + // 32-bit integer containing the traffic class field. // The low two bits are the ECN field. - if len(data) < 1 { - break + if ecn, ok := parseIPv6TCLASS(data); ok { + d.ecn = ecn } - d.ecn = ecnBits(data[0] & ecnMask) case unix.IPV6_PKTINFO: if a, ok := parseIn6Pktinfo(data); ok { d.localAddr = netip.AddrPortFrom(a, d.localAddr.Port()) @@ -173,27 +169,33 @@ func parseControl(d *datagram, control []byte) { } } -func parseInPktinfo(b []byte) (netip.Addr, bool) { - // struct in_pktinfo { - // unsigned int ipi_ifindex; /* send/recv interface index */ - // struct in_addr ipi_spec_dst; /* Local address */ - // struct in_addr ipi_addr; /* IP Header dst address */ - // }; - if len(b) != 12 { - return netip.Addr{}, false +// IPV6_TCLASS is specified by RFC 3542 as an int. + +func parseIPv6TCLASS(b []byte) (ecnBits, bool) { + if len(b) != 4 { + return 0, false } - return netip.AddrFrom4([4]byte(b[8:][:4])), true + return ecnBits(binary.NativeEndian.Uint32(b) & ecnMask), true } -func parseIn6Pktinfo(b []byte) (netip.Addr, bool) { - // struct in6_pktinfo { - // struct in6_addr ipi6_addr; /* src/dst IPv6 address */ - // unsigned int ipi6_ifindex; /* send/recv interface index */ - // }; - if len(b) != 20 { +func appendCmsgECNv6(b []byte, ecn ecnBits) []byte { + b, data := appendCmsg(b, unix.IPPROTO_IPV6, unix.IPV6_TCLASS, 4) + binary.NativeEndian.PutUint32(data, uint32(ecn)) + return b +} + +// struct in_pktinfo { +// unsigned int ipi_ifindex; /* send/recv interface index */ +// struct in_addr ipi_spec_dst; /* Local address */ +// struct in_addr ipi_addr; /* IP Header dst address */ +// }; + +// parseInPktinfo returns the destination address from an IP_PKTINFO. +func parseInPktinfo(b []byte) (dst netip.Addr, ok bool) { + if len(b) != 12 { return netip.Addr{}, false } - return netip.AddrFrom16([16]byte(b[:16])).Unmap(), true + return netip.AddrFrom4([4]byte(b[8:][:4])), true } // appendCmsgIPSourceAddrV4 appends an IP_PKTINFO setting the source address @@ -210,31 +212,28 @@ func appendCmsgIPSourceAddrV4(b []byte, src netip.Addr) []byte { return b } -// appendCmsgIPSourceAddrV6 appends an IP_PKTINFO or IPV6_PKTINFO -// setting the source address for an outbound datagram. +// struct in6_pktinfo { +// struct in6_addr ipi6_addr; /* src/dst IPv6 address */ +// unsigned int ipi6_ifindex; /* send/recv interface index */ +// }; + +// parseIn6Pktinfo returns the destination address from an IPV6_PKTINFO. +func parseIn6Pktinfo(b []byte) (netip.Addr, bool) { + if len(b) != 20 { + return netip.Addr{}, false + } + return netip.AddrFrom16([16]byte(b[:16])).Unmap(), true +} + +// appendCmsgIPSourceAddrV6 appends an IPV6_PKTINFO setting the source address +// for an outbound datagram. func appendCmsgIPSourceAddrV6(b []byte, src netip.Addr) []byte { - // struct in6_pktinfo { - // struct in6_addr ipi6_addr; /* src/dst IPv6 address */ - // unsigned int ipi6_ifindex; /* send/recv interface index */ - // }; b, data := appendCmsg(b, unix.IPPROTO_IPV6, unix.IPV6_PKTINFO, 20) ip := src.As16() copy(data[0:], ip[:]) return b } -func appendCmsgECNv4(b []byte, ecn ecnBits) []byte { - b, data := appendCmsg(b, unix.IPPROTO_IP, unix.IP_TOS, 4) - data[0] = byte(ecn) - return b -} - -func appendCmsgECNv6(b []byte, ecn ecnBits) []byte { - b, data := appendCmsg(b, unix.IPPROTO_IPV6, unix.IPV6_TCLASS, 4) - data[0] = byte(ecn) - return b -} - // appendCmsg appends a cmsg with the given level, type, and size to b. // It returns the new buffer, and the data section of the cmsg. func appendCmsg(b []byte, level, typ int32, size int) (_, data []byte) { diff --git a/internal/quic/udp_test.go b/internal/quic/udp_test.go index 450351b6b..d3732c140 100644 --- a/internal/quic/udp_test.go +++ b/internal/quic/udp_test.go @@ -16,9 +16,9 @@ import ( ) func TestUDPSourceUnspecified(t *testing.T) { - t.Skip("https://go.dev/issue/65906 - temporarily skipped pending fix") // Send datagram with no source address set. runUDPTest(t, func(t *testing.T, test udpTest) { + t.Logf("%v", test.dstAddr) data := []byte("source unspecified") if err := test.src.Write(datagram{ b: data, @@ -34,7 +34,6 @@ func TestUDPSourceUnspecified(t *testing.T) { } func TestUDPSourceSpecified(t *testing.T) { - t.Skip("https://go.dev/issue/65906 - temporarily skipped pending fix") // Send datagram with source address set. runUDPTest(t, func(t *testing.T, test udpTest) { data := []byte("source specified") @@ -53,7 +52,6 @@ func TestUDPSourceSpecified(t *testing.T) { } func TestUDPSourceInvalid(t *testing.T) { - t.Skip("https://go.dev/issue/65906 - temporarily skipped pending fix") // Send datagram with source address set to an address not associated with the connection. if !udpInvalidLocalAddrIsError { t.Skipf("%v: sending from invalid source succeeds", runtime.GOOS) @@ -77,7 +75,6 @@ func TestUDPSourceInvalid(t *testing.T) { } func TestUDPECN(t *testing.T) { - t.Skip("https://go.dev/issue/65907 - temporarily skipped pending fix") if !udpECNSupport { t.Skipf("%v: no ECN support", runtime.GOOS) } @@ -125,6 +122,18 @@ func runUDPTest(t *testing.T, f func(t *testing.T, u udpTest)) { spec = "unspec" } t.Run(fmt.Sprintf("%v/%v/%v", test.srcNet, test.dstNet, spec), func(t *testing.T) { + // See: https://go.googlesource.com/go/+/refs/tags/go1.22.0/src/net/ipsock.go#47 + // On these platforms, conns with network="udp" cannot accept IPv6. + switch runtime.GOOS { + case "dragonfly", "openbsd": + if test.srcNet == "udp6" && test.dstNet == "udp" { + t.Skipf("%v: no support for mapping IPv4 address to IPv6", runtime.GOOS) + } + } + if runtime.GOARCH == "wasm" && test.srcNet == "udp6" { + t.Skipf("%v: IPv6 tests fail when using wasm fake net", runtime.GOARCH) + } + srcAddr := netip.AddrPortFrom(netip.MustParseAddr(test.srcAddr), 0) srcConn, err := net.ListenUDP(test.srcNet, net.UDPAddrFromAddrPort(srcAddr)) if err != nil {