From d9b7211629272c8b4331791e9ebbde469f76b3e4 Mon Sep 17 00:00:00 2001 From: dignifiedquire Date: Wed, 29 Jul 2026 11:19:36 +0200 Subject: [PATCH 01/11] ci: run tests on arch64-musl --- .github/workflows/ci.yml | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f5bf02ef79..033afd3914 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -64,6 +64,39 @@ jobs: - name: Build and test noq-udp (posix_minimal) run: cargo test --locked -p noq-udp + test_musl_arm64: + name: Test aarch64-unknown-linux-musl + if: "github.event_name != 'pull_request' || ! contains(github.event.pull_request.labels.*.name, 'flaky-test')" + timeout-minutes: 30 + runs-on: ubuntu-24.04-arm + env: + RUSTC_WRAPPER: "sccache" + SCCACHE_GHA_ENABLED: "on" + # When cross-compiling to musl, cc-rs looks for `aarch64-linux-musl-gcc`, + # but `musl-tools` only ships the host-native `musl-gcc`. + CC_aarch64_unknown_linux_musl: musl-gcc + steps: + - uses: actions/checkout@v6 + - uses: dtolnay/rust-toolchain@stable + with: + target: aarch64-unknown-linux-musl + - uses: mozilla-actions/sccache-action@v0.0.9 + - name: Install musl toolchain + run: | + sudo apt-get update + sudo apt-get install -y musl-tools + - name: Install cargo-nextest + uses: taiki-e/install-action@v2 + with: + tool: nextest@0.9.80 + # The musl binaries are statically linked, so they run as-is on the + # aarch64 runner; no emulation or test runner needed. + - name: Run tests + run: | + cargo nextest run --locked --workspace --exclude fuzz --lib --bins --tests --target aarch64-unknown-linux-musl --profile ci + env: + RUST_LOG: ${{ runner.debug && 'TRACE' || 'DEBUG'}} + esp32_check: name: ESP32-C3 build check (noq-udp, noq-proto, noq) if: "github.event_name != 'pull_request' || ! contains(github.event.pull_request.labels.*.name, 'flaky-test')" From c64247e603d462e06238ced1855a5dd77070e3fa Mon Sep 17 00:00:00 2001 From: dignifiedquire Date: Wed, 29 Jul 2026 11:47:56 +0200 Subject: [PATCH 02/11] ci: also run the musl tests on x86_64 The cmsg alignment breakage reproduces on both musl architectures, and x86_64 is what the static `iroh-relay` builds use, so cover it too. Each target runs on a runner of its own architecture, so the statically linked test binaries run natively. --- .github/workflows/ci.yml | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 033afd3914..b60e2a1d7e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -64,22 +64,33 @@ jobs: - name: Build and test noq-udp (posix_minimal) run: cargo test --locked -p noq-udp - test_musl_arm64: - name: Test aarch64-unknown-linux-musl + test_musl: + name: Test ${{ matrix.target }} if: "github.event_name != 'pull_request' || ! contains(github.event.pull_request.labels.*.name, 'flaky-test')" timeout-minutes: 30 - runs-on: ubuntu-24.04-arm + strategy: + fail-fast: false + matrix: + include: + # Each target runs on a runner of its own architecture, so the tests + # run natively rather than under emulation. + - target: aarch64-unknown-linux-musl + runner: ubuntu-24.04-arm + - target: x86_64-unknown-linux-musl + runner: ubuntu-latest + runs-on: ${{ matrix.runner }} env: RUSTC_WRAPPER: "sccache" SCCACHE_GHA_ENABLED: "on" - # When cross-compiling to musl, cc-rs looks for `aarch64-linux-musl-gcc`, + # When cross-compiling to musl, cc-rs looks for `-linux-musl-gcc`, # but `musl-tools` only ships the host-native `musl-gcc`. CC_aarch64_unknown_linux_musl: musl-gcc + CC_x86_64_unknown_linux_musl: musl-gcc steps: - uses: actions/checkout@v6 - uses: dtolnay/rust-toolchain@stable with: - target: aarch64-unknown-linux-musl + target: ${{ matrix.target }} - uses: mozilla-actions/sccache-action@v0.0.9 - name: Install musl toolchain run: | @@ -90,10 +101,10 @@ jobs: with: tool: nextest@0.9.80 # The musl binaries are statically linked, so they run as-is on the - # aarch64 runner; no emulation or test runner needed. + # runner; no emulation or test runner needed. - name: Run tests run: | - cargo nextest run --locked --workspace --exclude fuzz --lib --bins --tests --target aarch64-unknown-linux-musl --profile ci + cargo nextest run --locked --workspace --exclude fuzz --lib --bins --tests --target ${{ matrix.target }} --profile ci env: RUST_LOG: ${{ runner.debug && 'TRACE' || 'DEBUG'}} From 1541e8c015eff10c94bc92e7935e8fbd9f389113 Mon Sep 17 00:00:00 2001 From: dignifiedquire Date: Wed, 29 Jul 2026 12:00:05 +0200 Subject: [PATCH 03/11] ci: drop a duplicated comment in the musl job --- .github/workflows/ci.yml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b60e2a1d7e..1aec54ce1c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -72,8 +72,8 @@ jobs: fail-fast: false matrix: include: - # Each target runs on a runner of its own architecture, so the tests - # run natively rather than under emulation. + # musl binaries are statically linked, and each target runs on a runner + # of its own architecture, so the tests run natively. - target: aarch64-unknown-linux-musl runner: ubuntu-24.04-arm - target: x86_64-unknown-linux-musl @@ -100,8 +100,6 @@ jobs: uses: taiki-e/install-action@v2 with: tool: nextest@0.9.80 - # The musl binaries are statically linked, so they run as-is on the - # runner; no emulation or test runner needed. - name: Run tests run: | cargo nextest run --locked --workspace --exclude fuzz --lib --bins --tests --target ${{ matrix.target }} --profile ci From b32f763ea3394a4562fad58db8fbb7b1a75f5603 Mon Sep 17 00:00:00 2001 From: dignifiedquire Date: Wed, 29 Jul 2026 11:45:32 +0200 Subject: [PATCH 04/11] fix(udp): size and align control message buffers from their payloads `cmsg::decode` asserted `align_of::() <= align_of::()`, which is not what makes reading a payload sound. musl declares `cmsghdr` with `socklen_t` and `c_int` fields (alignment 4) where glibc uses `size_t` (alignment 8), so the `SCM_TIMESTAMPNS` arm tripped it on the first packet and every musl target aborted. The layouts are identical, only the declared alignment differs. What has to hold is that the buffer is aligned for the payloads it carries, so derive it from them. `Payload` is a union of every type we put into or read out of a control message, and `ControlBuf` takes its alignment from that instead of from `repr(align(8))` plus a runtime assert that the guess was right. The size was guessed the same way, and was too small. `cmsg::LEN` was 96 bytes where a v6 socket with GRO and timestamps enabled needs 120, so the kernel set `MSG_CTRUNC` and dropped `IPV6_TCLASS`: the ECN codepoint disappeared from every coalesced receive, on glibc as much as musl. `SEND_LEN` and `RECV_LEN` now count the messages each direction carries and take their size from `libc::CMSG_SPACE`, which is a `const fn`, so the platform's own macros do the arithmetic. `PAYLOAD_ALIGN` comes from the same place, the alignment `CMSG_LEN(0)` and `CMSG_SPACE` have in common, capped by the buffer's own. That spread is wider than musl: 4 bytes on Darwin and the Solarish platforms, 8 on 64 bit Linux, 16 on NetBSD and OpenBSD on sparc64. No constant in this file covers all of them. `Encoder::push` is private now, with one method per message we send, so the set `SEND_LEN` has to cover is declared in one place. The alignment check that remains is a `const` assert: a payload a platform cannot align is a compile error rather than a panic on the first packet. `gso_with_ecn` covers the truncation, which nothing else could: `gso` sends no ECN and the `ecn_*` tests send a single segment, so neither puts enough control messages on one datagram. Fixes #774. --- noq-udp/src/apple_fast.rs | 28 ++-- noq-udp/src/cmsg/mod.rs | 45 +++++-- noq-udp/src/cmsg/unix.rs | 253 +++++++++++++++++++++++++++++++++++- noq-udp/src/cmsg/windows.rs | 163 ++++++++++++++++++++--- noq-udp/src/linux.rs | 2 +- noq-udp/src/unix.rs | 56 +++----- noq-udp/src/windows.rs | 45 ++----- noq-udp/tests/tests.rs | 30 +++++ 8 files changed, 508 insertions(+), 114 deletions(-) diff --git a/noq-udp/src/apple_fast.rs b/noq-udp/src/apple_fast.rs index ec9e23d08a..cb276178a7 100644 --- a/noq-udp/src/apple_fast.rs +++ b/noq-udp/src/apple_fast.rs @@ -33,7 +33,7 @@ fn send_via_sendmsg_x( ) -> io::Result<()> { let mut hdrs = unsafe { mem::zeroed::<[msghdr_x; BATCH_SIZE]>() }; let mut iovs = unsafe { mem::zeroed::<[libc::iovec; BATCH_SIZE]>() }; - let mut ctrls = [cmsg::Aligned([0u8; cmsg::LEN]); BATCH_SIZE]; + let mut ctrls = [cmsg::SendBuf::zeroed(); BATCH_SIZE]; let addr = socket2::SockAddr::from(transmit.destination); let segment_size = transmit.segment_size.unwrap_or(transmit.contents.len()); let mut cnt = 0; @@ -75,7 +75,7 @@ fn prepare_msg_x( dst_addr: &socket2::SockAddr, hdr: &mut msghdr_x, iov: &mut libc::iovec, - ctrl: &mut cmsg::Aligned<[u8; cmsg::LEN]>, + ctrl: &mut cmsg::SendBuf, #[allow(unused_variables)] encode_src_ip: bool, sendmsg_einval: bool, ) { @@ -89,18 +89,18 @@ fn prepare_msg_x( hdr.msg_iov = iov; hdr.msg_iovlen = 1; - hdr.msg_control = ctrl.0.as_mut_ptr() as _; - hdr.msg_controllen = cmsg::LEN as _; + hdr.msg_control = ctrl.as_mut_ptr() as _; + hdr.msg_controllen = ctrl.len() as _; let mut encoder = unsafe { cmsg::Encoder::new(hdr) }; let ecn = transmit.ecn.map_or(0, |x| x as libc::c_int); let is_ipv4 = transmit.destination.is_ipv4() || matches!(transmit.destination.ip(), IpAddr::V6(addr) if addr.to_ipv4_mapped().is_some()); if is_ipv4 { if !sendmsg_einval { - encoder.push(libc::IPPROTO_IP, libc::IP_TOS, ecn as IpTosTy); + encoder.push_ecn_v4(ecn as IpTosTy); } } else { - encoder.push(libc::IPPROTO_IPV6, libc::IPV6_TCLASS, ecn); + encoder.push_ecn_v6(ecn); } if let Some(ip) = &transmit.src_ip { @@ -110,7 +110,7 @@ fn prepare_msg_x( let addr = libc::in_addr { s_addr: u32::from_ne_bytes(v4.octets()), }; - encoder.push(libc::IPPROTO_IP, libc::IP_RECVDSTADDR, addr); + encoder.push_src_addr_v4(addr); } } IpAddr::V6(v6) => { @@ -120,7 +120,7 @@ fn prepare_msg_x( s6_addr: v6.octets(), }, }; - encoder.push(libc::IPPROTO_IPV6, libc::IPV6_PKTINFO, pktinfo); + encoder.push_pktinfo_v6(pktinfo); } } } @@ -156,7 +156,7 @@ pub(crate) fn recv_via_recvmsg_x( // uninitialized memory, do not use `MaybeUninit` for `ctrls`, instead // initialize `ctrls` with `0`s. A control message of all `0`s is // automatically skipped by `libc::CMSG_NXTHDR`. - let mut ctrls = [cmsg::Aligned([0u8; cmsg::LEN]); BATCH_SIZE]; + let mut ctrls = [cmsg::RecvBuf::zeroed(); BATCH_SIZE]; let mut hdrs = unsafe { mem::zeroed::<[msghdr_x; BATCH_SIZE]>() }; let max_msg_count = bufs.len().min(BATCH_SIZE); for i in 0..max_msg_count { @@ -178,15 +178,15 @@ pub(crate) fn recv_via_recvmsg_x( fn prepare_recv_x( buf: &mut IoSliceMut<'_>, name: &mut MaybeUninit, - ctrl: &mut cmsg::Aligned<[u8; cmsg::LEN]>, + ctrl: &mut cmsg::RecvBuf, hdr: &mut msghdr_x, ) { hdr.msg_name = name.as_mut_ptr() as _; hdr.msg_namelen = size_of::() as _; hdr.msg_iov = buf as *mut IoSliceMut<'_> as *mut libc::iovec; hdr.msg_iovlen = 1; - hdr.msg_control = ctrl.0.as_mut_ptr() as _; - hdr.msg_controllen = cmsg::LEN as _; + hdr.msg_control = ctrl.as_mut_ptr() as _; + hdr.msg_controllen = ctrl.len() as _; hdr.msg_flags = 0; hdr.msg_datalen = buf.len(); } @@ -248,4 +248,8 @@ impl MsgHdr for msghdr_x { fn control_len(&self) -> usize { self.msg_controllen as _ } + + fn recv_flags(&self) -> libc::c_int { + self.msg_flags + } } diff --git a/noq-udp/src/cmsg/mod.rs b/noq-udp/src/cmsg/mod.rs index 3e82cbabf7..dac77aed4b 100644 --- a/noq-udp/src/cmsg/mod.rs +++ b/noq-udp/src/cmsg/mod.rs @@ -1,6 +1,7 @@ use std::{ ffi::{c_int, c_uchar}, ptr, + sync::atomic::{AtomicBool, Ordering}, }; #[cfg(unix)] @@ -11,7 +12,7 @@ mod imp; #[path = "windows.rs"] mod imp; -pub(crate) use imp::Aligned; +pub(crate) use imp::{PAYLOAD_ALIGN, RecvBuf, SendBuf}; /// Helper to encode a series of control messages (native "cmsgs") to a buffer for use in `sendmsg` // like API. @@ -39,11 +40,18 @@ impl<'a, M: MsgHdr> Encoder<'a, M> { /// Append a control message to the buffer. /// + /// Private: each message we send has its own method, declared next to the buffer size + /// covering it. + /// /// # Panics /// - If insufficient buffer space remains. - /// - If `T` has stricter alignment requirements than `M::ControlMessage` - pub(crate) fn push(&mut self, level: c_int, ty: c_int, value: T) { - assert!(align_of::() <= align_of::()); + fn push(&mut self, level: c_int, ty: c_int, value: T) { + const { + assert!( + align_of::() <= PAYLOAD_ALIGN, + "control message payload is more aligned than a control message buffer can be", + ); + } let space = M::ControlMessage::cmsg_space(size_of_val(&value)); assert!( self.hdr.control_len() >= self.len + space, @@ -74,11 +82,32 @@ impl Drop for Encoder<'_, M> { } } +/// Warns once if the kernel had more to say about a datagram than the buffer could hold. +/// +/// Dropped control messages mean lost metadata, in the worst case the GRO segment size, +/// which leaves a coalesced datagram looking like a single one. `RECV_LEN` covers every +/// option we enable, so this means either a new one is unaccounted for, or the caller +/// enabled one of their own on the socket they gave us. +pub(crate) fn warn_if_control_truncated(hdr: &impl MsgHdr) { + static WARNED: AtomicBool = AtomicBool::new(false); + + if hdr.recv_flags() & imp::MSG_CTRUNC != 0 && !WARNED.swap(true, Ordering::Relaxed) { + crate::log::warn!( + "control messages truncated on receive, some datagram metadata was dropped" + ); + } +} + /// # Safety /// /// `cmsg` must refer to a native cmsg containing a payload of type `T` pub(crate) unsafe fn decode(cmsg: &impl CMsgHdr) -> T { - assert!(align_of::() <= align_of::()); + const { + assert!( + align_of::() <= PAYLOAD_ALIGN, + "control message payload is more aligned than a control message buffer can be", + ); + } debug_assert_eq!(cmsg.len(), C::cmsg_len(size_of::())); unsafe { ptr::read(cmsg.cmsg_data() as *const T) } } @@ -138,6 +167,9 @@ pub(crate) trait MsgHdr { fn set_control_len(&mut self, len: usize); fn control_len(&self) -> usize; + + /// The flags the kernel set on a received message, i.e. `msg_flags`. + fn recv_flags(&self) -> c_int; } pub(crate) trait CMsgHdr { @@ -151,6 +183,3 @@ pub(crate) trait CMsgHdr { fn len(&self) -> usize; } - -#[cfg(unix)] -pub(crate) const LEN: usize = 96; diff --git a/noq-udp/src/cmsg/unix.rs b/noq-udp/src/cmsg/unix.rs index 112bd5ebef..c8d4f86fa2 100644 --- a/noq-udp/src/cmsg/unix.rs +++ b/noq-udp/src/cmsg/unix.rs @@ -1,10 +1,166 @@ -use std::ffi::{c_int, c_uchar}; +use std::{ + ffi::{c_int, c_uchar}, + mem::MaybeUninit, +}; -use super::{CMsgHdr, MsgHdr}; +use super::{CMsgHdr, Encoder, MsgHdr}; +// netbsd sends no IP_TOS control message, so it has no payload type for one. +#[cfg(not(target_os = "netbsd"))] +use crate::imp::IpTosTy; +/// Every payload we put into, or read out of, a control message on this platform. +/// +/// A control message buffer must be aligned for the payloads it carries, and a union's +/// alignment is the strictest of its fields. [`libc::cmsghdr`] on its own is not enough: +/// musl declares it with `socklen_t` and `c_int` fields (alignment 4) where glibc uses +/// `size_t` (alignment 8), yet both carry payloads like [`libc::timespec`] needing 8. #[derive(Copy, Clone)] -#[repr(align(8))] // Conservative bound for align_of -pub(crate) struct Aligned(pub(crate) T); +#[repr(C)] +#[allow(dead_code)] // the fields are here for their alignment, nothing reads them +pub(crate) union Payload { + hdr: libc::cmsghdr, + #[cfg(not(target_os = "netbsd"))] + ecn_v4: IpTosTy, + ecn_v6: c_int, + segment_size: u16, + #[cfg(not(target_os = "redox"))] + pktinfo_v6: libc::in6_pktinfo, + #[cfg(any(target_os = "linux", target_os = "android"))] + pktinfo_v4: libc::in_pktinfo, + #[cfg(any(bsd, apple, solarish))] + dst_addr_v4: libc::in_addr, + #[cfg(any(target_os = "linux", target_os = "android"))] + timestamp: libc::timespec, +} + +/// Set in `msg_flags` when control messages did not fit in the buffer. +pub(crate) const MSG_CTRUNC: c_int = libc::MSG_CTRUNC; + +/// The buffer space one control message with a payload of this size takes up. +const fn cmsg_space(payload_len: usize) -> usize { + unsafe { libc::CMSG_SPACE(payload_len as _) as usize } +} + +/// The weaker of two alignments, i.e. the largest power of two dividing both. +const fn common_align(a: usize, b: usize) -> usize { + // Whichever of the two has its lowest set bit first decides the trailing zeros of the + // OR, and both are offsets a payload can sit at. + 1 << (a | b).trailing_zeros() +} + +/// The alignment a control message payload is guaranteed to have. +/// +/// A payload sits `CMSG_LEN(0)` bytes into its message, and messages sit a sum of +/// `CMSG_SPACE` values into the buffer, so a payload can only be assumed to have the +/// alignment those offsets share, and no more than the buffer itself has. The platforms +/// spread wider than they look, from 4 bytes on Darwin and the Solarish platforms to 16 on +/// NetBSD and OpenBSD on sparc64, so this takes it from their own macros rather than +/// restating it here. Checked against real pointers by `payloads_are_aligned`. +pub(crate) const PAYLOAD_ALIGN: usize = common_align( + common_align(unsafe { libc::CMSG_LEN(0) } as usize, cmsg_space(1)), + align_of::(), +); + +/// Space for one control message carrying any of our payloads. +const MESSAGE_LEN: usize = cmsg_space(size_of::()); + +/// Space for the control messages one `sendmsg` can carry. +/// +/// ECN, the GSO segment size and the source address, one each: the IPv4 and IPv6 forms +/// are mutually exclusive. +pub(crate) const SEND_LEN: usize = 3 * MESSAGE_LEN; + +/// Space for the control messages the kernel can attach to one received datagram. +/// +/// The TOS or traffic class, the packet info, the GRO segment size and the receive +/// timestamp, one each, matching the socket options `UdpSocketState::new` enables. +pub(crate) const RECV_LEN: usize = 4 * MESSAGE_LEN; + +/// A control message buffer of `N` bytes, aligned for every [`Payload`]. +#[derive(Copy, Clone)] +#[repr(C)] +pub(crate) struct ControlBuf { + /// Zero sized, present only to give the buffer [`Payload`]'s alignment. + _align: [Payload; 0], + bytes: [MaybeUninit; N], +} + +/// Control message buffer for one `sendmsg`. +pub(crate) type SendBuf = ControlBuf; + +/// Control message buffer for one `recvmsg`. +pub(crate) type RecvBuf = ControlBuf; + +impl ControlBuf { + /// A zeroed buffer, for sending. + pub(crate) const fn zeroed() -> Self { + Self { + _align: [], + bytes: [MaybeUninit::new(0); N], + } + } + + /// An uninitialised buffer, for receiving: the kernel initialises what it uses. + pub(crate) const fn uninit() -> Self { + Self { + _align: [], + bytes: [MaybeUninit::uninit(); N], + } + } + + pub(crate) fn as_mut_ptr(&mut self) -> *mut u8 { + self.bytes.as_mut_ptr().cast() + } + + /// The size of the buffer, for `msg_controllen`. + pub(crate) const fn len(&self) -> usize { + N + } +} + +/// The control messages we send. +/// +/// One method each rather than a generic `push`, so the set stays next to the [`SEND_LEN`] +/// that has to cover it. +impl> Encoder<'_, M> { + /// Sets the ECN codepoint of an IPv4 or IPv4-mapped datagram. + #[cfg(not(target_os = "netbsd"))] + pub(crate) fn push_ecn_v4(&mut self, ecn: IpTosTy) { + self.push(libc::IPPROTO_IP, libc::IP_TOS, ecn); + } + + /// Sets the IPv6 traffic class, which carries the ECN codepoint. + #[cfg(not(target_os = "redox"))] + pub(crate) fn push_ecn_v6(&mut self, ecn: c_int) { + self.push(libc::IPPROTO_IPV6, libc::IPV6_TCLASS, ecn); + } + + /// Sets the GSO segment size the kernel splits an oversized datagram into. + #[cfg(any(target_os = "linux", target_os = "android"))] + pub(crate) fn push_segment_size(&mut self, segment_size: u16) { + self.push(libc::SOL_UDP, libc::UDP_SEGMENT, segment_size); + } + + /// Sets the source address of an IPv4 datagram. + #[cfg(any(target_os = "linux", target_os = "android"))] + pub(crate) fn push_pktinfo_v4(&mut self, pktinfo: libc::in_pktinfo) { + self.push(libc::IPPROTO_IP, libc::IP_PKTINFO, pktinfo); + } + + /// Sets the source address of an IPv4 datagram. + /// + /// `IP_RECVDSTADDR` is `IP_SENDSRCADDR` on FreeBSD, the two have the same value. + #[cfg(any(bsd, apple, solarish))] + pub(crate) fn push_src_addr_v4(&mut self, addr: libc::in_addr) { + self.push(libc::IPPROTO_IP, libc::IP_RECVDSTADDR, addr); + } + + /// Sets the source address of an IPv6 datagram. + #[cfg(not(target_os = "redox"))] + pub(crate) fn push_pktinfo_v6(&mut self, pktinfo: libc::in6_pktinfo) { + self.push(libc::IPPROTO_IPV6, libc::IPV6_PKTINFO, pktinfo); + } +} /// Helpers for [`libc::msghdr`] impl MsgHdr for libc::msghdr { @@ -30,6 +186,10 @@ impl MsgHdr for libc::msghdr { fn control_len(&self) -> usize { self.msg_controllen as _ } + + fn recv_flags(&self) -> c_int { + self.msg_flags + } } /// Helpers for [`libc::cmsghdr`] @@ -56,3 +216,88 @@ impl CMsgHdr for libc::cmsghdr { self.cmsg_len as _ } } + +#[cfg(test)] +mod tests { + use std::mem; + + use super::*; + + /// The payload of every control message we can send in one `sendmsg`. + /// + /// `IpTosTy` is `c_int` or smaller everywhere it exists, so `c_int` stands in for it. + fn sent_payload_lens() -> Vec { + vec![ + size_of::(), // IP_TOS or IPV6_TCLASS + size_of::(), // UDP_SEGMENT + // IP_PKTINFO, IP_RECVDSTADDR or IPV6_PKTINFO + size_of::(), + ] + } + + /// The payload of every control message the kernel can attach to one datagram. + fn received_payload_lens() -> Vec { + vec![ + size_of::(), // IP_TOS or IPV6_TCLASS + size_of::(), // IP_PKTINFO or IPV6_PKTINFO + size_of::(), // UDP_GRO + #[cfg(any(target_os = "linux", target_os = "android"))] + size_of::(), // SCM_TIMESTAMPNS + ] + } + + fn libc_cmsg_space(payload_lens: &[usize]) -> usize { + payload_lens + .iter() + .map(|len| unsafe { libc::CMSG_SPACE(*len as _) as usize }) + .sum() + } + + /// The buffers hold every control message they have to. + /// + /// [`SEND_LEN`] and [`RECV_LEN`] count messages and assume the largest payload; this + /// adds up the real ones, so a message we forgot to count shows up here rather than as + /// a truncated datagram. + #[test] + fn control_len_covers_libc() { + let sent = libc_cmsg_space(&sent_payload_lens()); + assert!(SEND_LEN >= sent, "SEND_LEN is {SEND_LEN}, need {sent}"); + + let received = libc_cmsg_space(&received_payload_lens()); + assert!( + RECV_LEN >= received, + "RECV_LEN is {RECV_LEN}, need {received}" + ); + } + + /// Every payload in a full buffer is aligned for the type read out of it. + /// + /// What `cmsg::decode` relies on, and what breaks on musl if the buffer takes its + /// alignment from `libc::cmsghdr` rather than from the payloads. + #[test] + fn payloads_are_aligned() { + let mut buf = RecvBuf::zeroed(); + let mut hdr: libc::msghdr = unsafe { mem::zeroed() }; + hdr.msg_control = buf.as_mut_ptr().cast(); + hdr.msg_controllen = buf.len() as _; + + // The largest payload we use, so the messages after the first sit where a real + // receive would put them. + let mut encoder = unsafe { Encoder::new(&mut hdr) }; + for _ in 0..received_payload_lens().len() { + encoder.push(libc::SOL_SOCKET, 0, Payload { ecn_v6: 0 }); + } + encoder.finish(); + + let mut count = 0; + for cmsg in unsafe { super::super::Iter::new(&hdr) } { + assert_eq!( + cmsg.cmsg_data() as usize % PAYLOAD_ALIGN, + 0, + "payload {count} is not aligned to {PAYLOAD_ALIGN}", + ); + count += 1; + } + assert_eq!(count, received_payload_lens().len()); + } +} diff --git a/noq-udp/src/cmsg/windows.rs b/noq-udp/src/cmsg/windows.rs index f4fecca439..02eea278b5 100644 --- a/noq-udp/src/cmsg/windows.rs +++ b/noq-udp/src/cmsg/windows.rs @@ -1,15 +1,144 @@ use std::{ ffi::{c_int, c_uchar}, - mem, ptr, + mem::MaybeUninit, }; use windows_sys::Win32::Networking::WinSock; -use super::{CMsgHdr, MsgHdr}; +use super::{CMsgHdr, Encoder, MsgHdr}; +/// Every payload we put into, or read out of, a control message on this platform. +/// +/// A control message buffer must be aligned for the payloads it carries, and a union's +/// alignment is the strictest of its fields. #[derive(Copy, Clone)] -#[repr(align(8))] // Conservative bound for align_of -pub(crate) struct Aligned(pub(crate) T); +#[repr(C)] +#[allow(dead_code)] // the fields are here for their alignment, nothing reads them +pub(crate) union Payload { + hdr: WinSock::CMSGHDR, + ecn: c_int, + segment_size: u32, + pktinfo_v4: WinSock::IN_PKTINFO, + pktinfo_v6: WinSock::IN6_PKTINFO, +} + +/// The alignment a control message payload is guaranteed to have. +/// +/// `WSA_CMSG_DATA` rounds the header size up to this and `WSA_CMSG_SPACE` keeps every +/// following header at a multiple of it, so payloads are this aligned as long as the +/// buffer is, which [`Payload`] ensures. +pub(crate) const PAYLOAD_ALIGN: usize = align_of::(); + +/// Set in `dwFlags` when control messages did not fit in the buffer. +pub(crate) const MSG_CTRUNC: c_int = WinSock::MSG_CTRUNC as c_int; + +// The four functions below follow the C macros in +// https://github.com/microsoft/win32metadata/blob/main/generation/WinSDK/RecompiledIdlHeaders/shared/ws2def.h#L741 + +/// `WSA_CMSG_ALIGN`, which control message headers are aligned to. +const fn cmsghdr_align(len: usize) -> usize { + (len + align_of::() - 1) & !(align_of::() - 1) +} + +/// `WSA_CMSGDATA_ALIGN`, which control message payloads are aligned to. +const fn cmsgdata_align(len: usize) -> usize { + (len + PAYLOAD_ALIGN - 1) & !(PAYLOAD_ALIGN - 1) +} + +/// `WSA_CMSG_LEN`, the value of `cmsg_len` for a payload of `payload_len` bytes. +const fn cmsg_len(payload_len: usize) -> usize { + cmsgdata_align(size_of::()) + payload_len +} + +/// `WSA_CMSG_SPACE`, the buffer space one control message with this payload takes up. +const fn cmsg_space(payload_len: usize) -> usize { + cmsgdata_align(size_of::() + cmsghdr_align(payload_len)) +} + +/// Space for one control message carrying any of our payloads. +const MESSAGE_LEN: usize = cmsg_space(size_of::()); + +/// Space for the control messages one `WSASendMsg` can carry. +/// +/// The ECN codepoint, the source address and the segment size, one each: the IPv4 and +/// IPv6 forms are mutually exclusive. +pub(crate) const SEND_LEN: usize = 3 * MESSAGE_LEN; + +/// Space for the control messages `WSARecvMsg` can return for one datagram. +/// +/// The ECN codepoint, the packet info and the URO coalesced size, one each. +pub(crate) const RECV_LEN: usize = 3 * MESSAGE_LEN; + +/// A control message buffer of `N` bytes, aligned for every [`Payload`]. +#[derive(Copy, Clone)] +#[repr(C)] +pub(crate) struct ControlBuf { + /// Zero sized, present only to give the buffer [`Payload`]'s alignment. + _align: [Payload; 0], + bytes: [MaybeUninit; N], +} + +/// Control message buffer for one `WSASendMsg`. +pub(crate) type SendBuf = ControlBuf; + +/// Control message buffer for one `WSARecvMsg`. +pub(crate) type RecvBuf = ControlBuf; + +impl ControlBuf { + /// A zeroed buffer. + pub(crate) const fn zeroed() -> Self { + Self { + _align: [], + bytes: [MaybeUninit::new(0); N], + } + } + + pub(crate) fn as_mut_ptr(&mut self) -> *mut u8 { + self.bytes.as_mut_ptr().cast() + } + + /// The size of the buffer, for `Control.len`. + pub(crate) const fn len(&self) -> usize { + N + } +} + +/// The control messages we send. +/// +/// One method each rather than a generic `push`, so the set stays next to the [`SEND_LEN`] +/// that has to cover it. +impl> Encoder<'_, M> { + /// Sets the ECN codepoint of an IPv4 datagram. + pub(crate) fn push_ecn_v4(&mut self, ecn: c_int) { + self.push(WinSock::IPPROTO_IP, WinSock::IP_ECN, ecn); + } + + /// Sets the ECN codepoint of an IPv6 datagram. + pub(crate) fn push_ecn_v6(&mut self, ecn: c_int) { + self.push(WinSock::IPPROTO_IPV6, WinSock::IPV6_ECN, ecn); + } + + /// Sets the segment size the stack splits an oversized datagram into. + /// + /// + pub(crate) fn push_segment_size(&mut self, segment_size: u32) { + self.push( + WinSock::IPPROTO_UDP, + WinSock::UDP_SEND_MSG_SIZE, + segment_size, + ); + } + + /// Sets the source address of an IPv4 datagram. + pub(crate) fn push_pktinfo_v4(&mut self, pktinfo: WinSock::IN_PKTINFO) { + self.push(WinSock::IPPROTO_IP, WinSock::IP_PKTINFO, pktinfo); + } + + /// Sets the source address of an IPv6 datagram. + pub(crate) fn push_pktinfo_v6(&mut self, pktinfo: WinSock::IN6_PKTINFO) { + self.push(WinSock::IPPROTO_IPV6, WinSock::IPV6_PKTINFO, pktinfo); + } +} /// Helpers for [`WinSock::WSAMSG`] // https://learn.microsoft.com/en-us/windows/win32/api/ws2def/ns-ws2def-wsamsg @@ -18,10 +147,10 @@ impl MsgHdr for WinSock::WSAMSG { type ControlMessage = WinSock::CMSGHDR; fn cmsg_first_hdr(&self) -> *mut Self::ControlMessage { - if self.Control.len as usize >= mem::size_of::() { + if self.Control.len as usize >= size_of::() { self.Control.buf as *mut WinSock::CMSGHDR } else { - ptr::null_mut::() + std::ptr::null_mut::() } } @@ -30,7 +159,7 @@ impl MsgHdr for WinSock::WSAMSG { (cmsg as *const _ as usize + cmsghdr_align(cmsg.cmsg_len)) as *mut WinSock::CMSGHDR; let max = self.Control.buf as usize + self.Control.len as usize; if unsafe { next.offset(1) } as usize > max { - ptr::null_mut() + std::ptr::null_mut() } else { next } @@ -43,6 +172,10 @@ impl MsgHdr for WinSock::WSAMSG { fn control_len(&self) -> usize { self.Control.len as _ } + + fn recv_flags(&self) -> c_int { + self.dwFlags as _ + } } /// Helpers for [`WinSock::CMSGHDR`] @@ -50,15 +183,15 @@ impl MsgHdr for WinSock::WSAMSG { // https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/Networking/WinSock/struct.CMSGHDR.html impl CMsgHdr for WinSock::CMSGHDR { fn cmsg_len(length: usize) -> usize { - cmsgdata_align(mem::size_of::()) + length + cmsg_len(length) } fn cmsg_space(length: usize) -> usize { - cmsgdata_align(mem::size_of::() + cmsghdr_align(length)) + cmsg_space(length) } fn cmsg_data(&self) -> *mut c_uchar { - (self as *const _ as usize + cmsgdata_align(mem::size_of::())) as *mut c_uchar + (self as *const _ as usize + cmsgdata_align(size_of::())) as *mut c_uchar } fn set(&mut self, level: c_int, ty: c_int, len: usize) { @@ -71,13 +204,3 @@ impl CMsgHdr for WinSock::CMSGHDR { self.cmsg_len as _ } } - -// Helpers functions for `WinSock::WSAMSG` and `WinSock::CMSGHDR` are based on C macros from -// https://github.com/microsoft/win32metadata/blob/main/generation/WinSDK/RecompiledIdlHeaders/shared/ws2def.h#L741 -fn cmsghdr_align(length: usize) -> usize { - (length + mem::align_of::() - 1) & !(mem::align_of::() - 1) -} - -fn cmsgdata_align(length: usize) -> usize { - (length + mem::align_of::() - 1) & !(mem::align_of::() - 1) -} diff --git a/noq-udp/src/linux.rs b/noq-udp/src/linux.rs index 7d72a14aeb..13f8f3deb4 100644 --- a/noq-udp/src/linux.rs +++ b/noq-udp/src/linux.rs @@ -49,7 +49,7 @@ pub(super) mod gso { encoder: &mut cmsg::Encoder<'_, libc::msghdr>, segment_size: u16, ) { - encoder.push(libc::SOL_UDP, libc::UDP_SEGMENT, segment_size); + encoder.push_segment_size(segment_size); } // Avoid calling `supported_by_current_kernel` for each socket by using `OnceLock`. diff --git a/noq-udp/src/unix.rs b/noq-udp/src/unix.rs index c53e3448c3..3e17dd8bbc 100644 --- a/noq-udp/src/unix.rs +++ b/noq-udp/src/unix.rs @@ -53,28 +53,6 @@ pub struct UdpSocketState { impl UdpSocketState { pub fn new(sock: UdpSockRef<'_>) -> io::Result { let io = sock.0; - let mut cmsg_platform_space = 0; - #[cfg(not(target_os = "redox"))] - if cfg!(target_os = "linux") - || cfg!(bsd) - || cfg!(apple) - || cfg!(target_os = "android") - || cfg!(solarish) - { - cmsg_platform_space += - unsafe { libc::CMSG_SPACE(size_of::() as _) as usize }; - } - - assert!( - cmsg::LEN - >= unsafe { libc::CMSG_SPACE(size_of::() as _) as usize } - + cmsg_platform_space - ); - assert!( - align_of::() <= align_of::>(), - "control message buffers will be misaligned" - ); - io.set_nonblocking(true)?; let addr = io.local_addr()?; @@ -402,7 +380,7 @@ fn send( } let mut msg_hdr: libc::msghdr = unsafe { mem::zeroed() }; let mut iovec: libc::iovec = unsafe { mem::zeroed() }; - let mut cmsgs = cmsg::Aligned([0u8; cmsg::LEN]); + let mut cmsgs = cmsg::SendBuf::zeroed(); let dst_addr = socket2::SockAddr::from(transmit.destination); prepare_msg( transmit, @@ -478,7 +456,7 @@ pub(crate) fn send_single( ) -> io::Result<()> { let mut hdr: libc::msghdr = unsafe { mem::zeroed() }; let mut iov: libc::iovec = unsafe { mem::zeroed() }; - let mut ctrl = cmsg::Aligned([0u8; cmsg::LEN]); + let mut ctrl = cmsg::SendBuf::zeroed(); let addr = socket2::SockAddr::from(transmit.destination); prepare_msg( transmit, @@ -508,7 +486,7 @@ fn recv_via_recvmmsg( meta: &mut [RecvMeta], ) -> io::Result { let mut names = [MaybeUninit::::uninit(); BATCH_SIZE]; - let mut ctrls = [cmsg::Aligned(MaybeUninit::<[u8; cmsg::LEN]>::uninit()); BATCH_SIZE]; + let mut ctrls = [cmsg::RecvBuf::uninit(); BATCH_SIZE]; let mut hdrs = unsafe { mem::zeroed::<[libc::mmsghdr; BATCH_SIZE]>() }; let max_msg_count = bufs.len().min(BATCH_SIZE); for i in 0..max_msg_count { @@ -549,7 +527,7 @@ pub(crate) fn recv_single( meta: &mut [RecvMeta], ) -> io::Result { let mut name = MaybeUninit::::uninit(); - let mut ctrl = cmsg::Aligned(MaybeUninit::<[u8; cmsg::LEN]>::uninit()); + let mut ctrl = cmsg::RecvBuf::uninit(); let mut hdr = unsafe { mem::zeroed::() }; prepare_recv(&mut bufs[0], &mut name, &mut ctrl, &mut hdr); let n = loop { @@ -580,7 +558,7 @@ fn prepare_msg( dst_addr: &socket2::SockAddr, hdr: &mut libc::msghdr, iov: &mut libc::iovec, - ctrl: &mut cmsg::Aligned<[u8; cmsg::LEN]>, + ctrl: &mut cmsg::SendBuf, #[allow(unused_variables)] // only used on FreeBSD & macOS encode_src_ip: bool, sendmsg_einval: bool, @@ -600,8 +578,8 @@ fn prepare_msg( hdr.msg_iov = iov; hdr.msg_iovlen = 1; - hdr.msg_control = ctrl.0.as_mut_ptr() as _; - hdr.msg_controllen = cmsg::LEN as _; + hdr.msg_control = ctrl.as_mut_ptr() as _; + hdr.msg_controllen = ctrl.len() as _; let mut encoder = unsafe { cmsg::Encoder::new(hdr) }; let ecn = transmit.ecn.map_or(0, |x| x as libc::c_int); // True for IPv4 or IPv4-Mapped IPv6 @@ -611,12 +589,12 @@ fn prepare_msg( if !sendmsg_einval { #[cfg(not(target_os = "netbsd"))] { - encoder.push(libc::IPPROTO_IP, libc::IP_TOS, ecn as IpTosTy); + encoder.push_ecn_v4(ecn as IpTosTy); } } } else { #[cfg(not(target_os = "redox"))] - encoder.push(libc::IPPROTO_IPV6, libc::IPV6_TCLASS, ecn); + encoder.push_ecn_v6(ecn); } // On apple_fast, prepare_msg is only compiled for send_single (fallback path), while the main @@ -639,7 +617,7 @@ fn prepare_msg( }, ipi_addr: libc::in_addr { s_addr: 0 }, }; - encoder.push(libc::IPPROTO_IP, libc::IP_PKTINFO, pktinfo); + encoder.push_pktinfo_v4(pktinfo); } #[cfg(any(bsd, apple, solarish))] { @@ -647,7 +625,7 @@ fn prepare_msg( let addr = libc::in_addr { s_addr: u32::from_ne_bytes(v4.octets()), }; - encoder.push(libc::IPPROTO_IP, libc::IP_RECVDSTADDR, addr); + encoder.push_src_addr_v4(addr); } } } @@ -661,7 +639,7 @@ fn prepare_msg( s6_addr: v6.octets(), }, }; - encoder.push(libc::IPPROTO_IPV6, libc::IPV6_PKTINFO, pktinfo); + encoder.push_pktinfo_v6(pktinfo); } } } @@ -673,15 +651,15 @@ fn prepare_msg( fn prepare_recv( buf: &mut IoSliceMut<'_>, name: &mut MaybeUninit, - ctrl: &mut cmsg::Aligned>, + ctrl: &mut cmsg::RecvBuf, hdr: &mut libc::msghdr, ) { hdr.msg_name = name.as_mut_ptr() as _; hdr.msg_namelen = size_of::() as _; hdr.msg_iov = buf as *mut IoSliceMut<'_> as *mut libc::iovec; hdr.msg_iovlen = 1; - hdr.msg_control = ctrl.0.as_mut_ptr() as _; - hdr.msg_controllen = cmsg::LEN as _; + hdr.msg_control = ctrl.as_mut_ptr() as _; + hdr.msg_controllen = ctrl.len() as _; hdr.msg_flags = 0; } @@ -699,6 +677,8 @@ pub(crate) fn decode_recv>( timestamp: None, }; + cmsg::warn_if_control_truncated(hdr); + let cmsg_iter = unsafe { cmsg::Iter::new(hdr) }; for cmsg in cmsg_iter { ctrl.decode(cmsg); @@ -851,7 +831,7 @@ mod gso { } #[cfg(target_os = "freebsd")] -type IpTosTy = libc::c_uchar; +pub(crate) type IpTosTy = libc::c_uchar; #[cfg(not(any(target_os = "freebsd", target_os = "netbsd")))] pub(crate) type IpTosTy = libc::c_int; diff --git a/noq-udp/src/windows.rs b/noq-udp/src/windows.rs index d96eacad7b..c333d309d3 100644 --- a/noq-udp/src/windows.rs +++ b/noq-udp/src/windows.rs @@ -16,9 +16,7 @@ use libc::{c_int, c_uint}; use windows_sys::Win32::Networking::WinSock; use crate::{ - EcnCodepoint, IO_ERROR_LOG_INTERVAL, RecvMeta, Transmit, UdpSockRef, - cmsg::{self, CMsgHdr}, - log::debug, + EcnCodepoint, IO_ERROR_LOG_INTERVAL, RecvMeta, Transmit, UdpSockRef, cmsg, log::debug, log_sendmsg_error, }; @@ -38,17 +36,6 @@ pub struct UdpSocketState { impl UdpSocketState { pub fn new(socket: UdpSockRef<'_>) -> io::Result { - assert!( - CMSG_LEN - >= WinSock::CMSGHDR::cmsg_space(size_of::()) - + WinSock::CMSGHDR::cmsg_space(size_of::()) - + WinSock::CMSGHDR::cmsg_space(size_of::()) - ); - assert!( - align_of::() <= align_of::>(), - "control message buffers will be misaligned" - ); - socket.0.set_nonblocking(true)?; // Stop Windows from failing the next recv with WSAECONNRESET or WSAENETRESET when a @@ -277,7 +264,7 @@ impl UdpSocketState { let wsa_recvmsg_ptr = WSARECVMSG_PTR.expect("valid function pointer for WSARecvMsg"); // we cannot use [`socket2::MsgHdrMut`] as we do not have access to inner field which holds the WSAMSG - let mut ctrl_buf = cmsg::Aligned([0; CMSG_LEN]); + let mut ctrl_buf = cmsg::RecvBuf::zeroed(); let mut source: WinSock::SOCKADDR_INET = unsafe { mem::zeroed() }; let mut data = WinSock::WSABUF { buf: bufs[0].as_mut_ptr(), @@ -285,8 +272,8 @@ impl UdpSocketState { }; let ctrl = WinSock::WSABUF { - buf: ctrl_buf.0.as_mut_ptr(), - len: ctrl_buf.0.len() as _, + buf: ctrl_buf.as_mut_ptr(), + len: ctrl_buf.len() as _, }; let mut wsa_msg = WinSock::WSAMSG { @@ -312,6 +299,8 @@ impl UdpSocketState { } } + cmsg::warn_if_control_truncated(&wsa_msg); + let addr = unsafe { let (_, addr) = socket2::SockAddr::try_init(|addr_storage, len| { *len = size_of_val(&source) as _; @@ -447,7 +436,7 @@ fn is_unsupported_error(e: &io::Error) -> bool { fn send(state: &UdpSocketState, socket: UdpSockRef<'_>, transmit: &Transmit<'_>) -> io::Result<()> { // we cannot use [`socket2::sendmsg()`] and [`socket2::MsgHdr`] as we do not have access // to the inner field which holds the WSAMSG - let mut ctrl_buf = cmsg::Aligned([0; CMSG_LEN]); + let mut ctrl_buf = cmsg::SendBuf::zeroed(); let daddr = socket2::SockAddr::from(transmit.destination); let mut data = WinSock::WSABUF { @@ -456,8 +445,8 @@ fn send(state: &UdpSocketState, socket: UdpSockRef<'_>, transmit: &Transmit<'_>) }; let ctrl = WinSock::WSABUF { - buf: ctrl_buf.0.as_mut_ptr(), - len: ctrl_buf.0.len() as _, + buf: ctrl_buf.as_mut_ptr(), + len: ctrl_buf.len() as _, }; let mut wsa_msg = WinSock::WSAMSG { @@ -485,7 +474,7 @@ fn send(state: &UdpSocketState, socket: UdpSockRef<'_>, transmit: &Transmit<'_>) ipi_addr: src_ip.sin_addr, ipi_ifindex: 0, }; - encoder.push(WinSock::IPPROTO_IP, WinSock::IP_PKTINFO, pktinfo); + encoder.push_pktinfo_v4(pktinfo); } WinSock::AF_INET6 if state.pktinfo_v6_enabled.load(Ordering::Relaxed) => { let src_ip = unsafe { ptr::read(ip.as_ptr() as *const WinSock::SOCKADDR_IN6) }; @@ -493,7 +482,7 @@ fn send(state: &UdpSocketState, socket: UdpSockRef<'_>, transmit: &Transmit<'_>) ipi6_addr: src_ip.sin6_addr, ipi6_ifindex: unsafe { src_ip.Anonymous.sin6_scope_id }, }; - encoder.push(WinSock::IPPROTO_IPV6, WinSock::IPV6_PKTINFO, pktinfo); + encoder.push_pktinfo_v6(pktinfo); } WinSock::AF_INET | WinSock::AF_INET6 => {} _ => { @@ -505,21 +494,17 @@ fn send(state: &UdpSocketState, socket: UdpSockRef<'_>, transmit: &Transmit<'_>) let ecn = transmit.ecn.map_or(0, |x| x as c_int); if is_ipv4 { if state.ecn_v4_enabled.load(Ordering::Relaxed) { - encoder.push(WinSock::IPPROTO_IP, WinSock::IP_ECN, ecn); + encoder.push_ecn_v4(ecn); } } else { if state.ecn_v6_enabled.load(Ordering::Relaxed) { - encoder.push(WinSock::IPPROTO_IPV6, WinSock::IPV6_ECN, ecn); + encoder.push_ecn_v6(ecn); } } // Segment size is a u32 https://learn.microsoft.com/en-us/windows/win32/api/ws2tcpip/nf-ws2tcpip-wsasetudpsendmessagesize if let Some(segment_size) = transmit.effective_segment_size() { - encoder.push( - WinSock::IPPROTO_UDP, - WinSock::UDP_SEND_MSG_SIZE, - segment_size as u32, - ); + encoder.push_segment_size(segment_size as u32); } encoder.finish(); @@ -607,8 +592,6 @@ fn set_socket_option( } pub(crate) const BATCH_SIZE: usize = 1; -// Enough to store max(IP_PKTINFO + IP_ECN, IPV6_PKTINFO + IPV6_ECN) + max(UDP_SEND_MSG_SIZE, UDP_COALESCED_INFO) bytes (header + data) and some extra margin -const CMSG_LEN: usize = 128; const OPTION_ON: u32 = 1; static WSARECVMSG_PTR: LazyLock = LazyLock::new(|| { diff --git a/noq-udp/tests/tests.rs b/noq-udp/tests/tests.rs index 6d8601bf1f..d6b9b66c77 100644 --- a/noq-udp/tests/tests.rs +++ b/noq-udp/tests/tests.rs @@ -244,6 +244,36 @@ fn gso() { ); } +/// A datagram that is both segmented and ECN marked. +/// +/// The traffic class arrives after the GRO segment size and the timestamp, so a receive +/// buffer that is too small drops it and ECN feedback silently disappears. `gso` sends no +/// ECN and the `ecn_*` tests send a single segment, so neither puts enough control +/// messages on one datagram to notice. +#[test] +#[cfg_attr(not(any(target_os = "linux", target_os = "android")), ignore)] +fn gso_with_ecn() { + let send = UdpSocket::bind((Ipv6Addr::LOCALHOST, 0)).unwrap(); + let recv = UdpSocket::bind((Ipv6Addr::LOCALHOST, 0)).unwrap(); + let max_segments = UdpSocketState::new((&send).into()) + .unwrap() + .max_gso_segments(); + let dst_addr = recv.local_addr().unwrap(); + const SEGMENT_SIZE: usize = 128; + let msg = vec![0xAB; SEGMENT_SIZE * max_segments.get()]; + test_send_recv( + &send.into(), + &recv.into(), + Transmit { + destination: dst_addr, + ecn: Some(EcnCodepoint::Ect0), + contents: &msg, + segment_size: Some(SEGMENT_SIZE), + src_ip: None, + }, + ); +} + #[test] fn socket_buffers() { const BUFFER_SIZE: usize = 123456; From e75075c71c40780d6e654dde893da4a890014b2a Mon Sep 17 00:00:00 2001 From: dignifiedquire Date: Wed, 29 Jul 2026 13:00:43 +0200 Subject: [PATCH 05/11] style(udp): keep the `mem::` and `ptr::` prefixes in `cmsg::windows` Rewriting the module dropped them for the prelude and `std::` paths; the file used the imports throughout, so put them back. --- noq-udp/src/cmsg/windows.rs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/noq-udp/src/cmsg/windows.rs b/noq-udp/src/cmsg/windows.rs index 02eea278b5..b59ba69450 100644 --- a/noq-udp/src/cmsg/windows.rs +++ b/noq-udp/src/cmsg/windows.rs @@ -1,6 +1,7 @@ use std::{ ffi::{c_int, c_uchar}, - mem::MaybeUninit, + mem::{self, MaybeUninit}, + ptr, }; use windows_sys::Win32::Networking::WinSock; @@ -27,7 +28,7 @@ pub(crate) union Payload { /// `WSA_CMSG_DATA` rounds the header size up to this and `WSA_CMSG_SPACE` keeps every /// following header at a multiple of it, so payloads are this aligned as long as the /// buffer is, which [`Payload`] ensures. -pub(crate) const PAYLOAD_ALIGN: usize = align_of::(); +pub(crate) const PAYLOAD_ALIGN: usize = mem::align_of::(); /// Set in `dwFlags` when control messages did not fit in the buffer. pub(crate) const MSG_CTRUNC: c_int = WinSock::MSG_CTRUNC as c_int; @@ -37,7 +38,7 @@ pub(crate) const MSG_CTRUNC: c_int = WinSock::MSG_CTRUNC as c_int; /// `WSA_CMSG_ALIGN`, which control message headers are aligned to. const fn cmsghdr_align(len: usize) -> usize { - (len + align_of::() - 1) & !(align_of::() - 1) + (len + mem::align_of::() - 1) & !(mem::align_of::() - 1) } /// `WSA_CMSGDATA_ALIGN`, which control message payloads are aligned to. @@ -47,16 +48,16 @@ const fn cmsgdata_align(len: usize) -> usize { /// `WSA_CMSG_LEN`, the value of `cmsg_len` for a payload of `payload_len` bytes. const fn cmsg_len(payload_len: usize) -> usize { - cmsgdata_align(size_of::()) + payload_len + cmsgdata_align(mem::size_of::()) + payload_len } /// `WSA_CMSG_SPACE`, the buffer space one control message with this payload takes up. const fn cmsg_space(payload_len: usize) -> usize { - cmsgdata_align(size_of::() + cmsghdr_align(payload_len)) + cmsgdata_align(mem::size_of::() + cmsghdr_align(payload_len)) } /// Space for one control message carrying any of our payloads. -const MESSAGE_LEN: usize = cmsg_space(size_of::()); +const MESSAGE_LEN: usize = cmsg_space(mem::size_of::()); /// Space for the control messages one `WSASendMsg` can carry. /// @@ -147,10 +148,10 @@ impl MsgHdr for WinSock::WSAMSG { type ControlMessage = WinSock::CMSGHDR; fn cmsg_first_hdr(&self) -> *mut Self::ControlMessage { - if self.Control.len as usize >= size_of::() { + if self.Control.len as usize >= mem::size_of::() { self.Control.buf as *mut WinSock::CMSGHDR } else { - std::ptr::null_mut::() + ptr::null_mut::() } } @@ -159,7 +160,7 @@ impl MsgHdr for WinSock::WSAMSG { (cmsg as *const _ as usize + cmsghdr_align(cmsg.cmsg_len)) as *mut WinSock::CMSGHDR; let max = self.Control.buf as usize + self.Control.len as usize; if unsafe { next.offset(1) } as usize > max { - std::ptr::null_mut() + ptr::null_mut() } else { next } @@ -191,7 +192,7 @@ impl CMsgHdr for WinSock::CMSGHDR { } fn cmsg_data(&self) -> *mut c_uchar { - (self as *const _ as usize + cmsgdata_align(size_of::())) as *mut c_uchar + (self as *const _ as usize + cmsgdata_align(mem::size_of::())) as *mut c_uchar } fn set(&mut self, level: c_int, ty: c_int, len: usize) { From 1da92dbc9001529a4a48c181af0c3f292ddeeccb Mon Sep 17 00:00:00 2001 From: dignifiedquire Date: Wed, 29 Jul 2026 13:24:31 +0200 Subject: [PATCH 06/11] test(udp): cover the Windows control message buffer The unix side has `payloads_are_aligned`; Windows had nothing checking that its buffer is sized and aligned for what the send path puts in it. Encoding the full set also exercises `SEND_LEN`, since `push` panics rather than overrun. Also adds the byte `IP_TOS` is decoded as to `Payload`, which listed every other payload but that one. It is smaller and less aligned than the rest, so neither constant moves. --- noq-udp/src/cmsg/unix.rs | 2 ++ noq-udp/src/cmsg/windows.rs | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/noq-udp/src/cmsg/unix.rs b/noq-udp/src/cmsg/unix.rs index c8d4f86fa2..1c760b67ad 100644 --- a/noq-udp/src/cmsg/unix.rs +++ b/noq-udp/src/cmsg/unix.rs @@ -22,6 +22,8 @@ pub(crate) union Payload { #[cfg(not(target_os = "netbsd"))] ecn_v4: IpTosTy, ecn_v6: c_int, + /// `IP_TOS` and, on Darwin, `IPV6_TCLASS` come back as a single byte. + ecn_byte: u8, segment_size: u16, #[cfg(not(target_os = "redox"))] pktinfo_v6: libc::in6_pktinfo, diff --git a/noq-udp/src/cmsg/windows.rs b/noq-udp/src/cmsg/windows.rs index b59ba69450..023032c0be 100644 --- a/noq-udp/src/cmsg/windows.rs +++ b/noq-udp/src/cmsg/windows.rs @@ -205,3 +205,39 @@ impl CMsgHdr for WinSock::CMSGHDR { self.cmsg_len as _ } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Every payload in a full buffer is aligned for the type read out of it. + /// + /// Encoding the whole set the send path can produce also proves [`SEND_LEN`] covers it, + /// since [`Encoder::push`] panics rather than overrun the buffer. + #[test] + fn payloads_are_aligned() { + let mut buf = SendBuf::zeroed(); + let mut msg: WinSock::WSAMSG = unsafe { mem::zeroed() }; + msg.Control = WinSock::WSABUF { + buf: buf.as_mut_ptr(), + len: buf.len() as _, + }; + + let mut encoder = unsafe { Encoder::new(&mut msg) }; + encoder.push_pktinfo_v6(unsafe { mem::zeroed() }); + encoder.push_ecn_v6(0); + encoder.push_segment_size(1200); + encoder.finish(); + + let mut count = 0; + for cmsg in unsafe { super::super::Iter::new(&msg) } { + assert_eq!( + cmsg.cmsg_data() as usize % PAYLOAD_ALIGN, + 0, + "payload {count} is not aligned to {PAYLOAD_ALIGN}", + ); + count += 1; + } + assert_eq!(count, 3); + } +} From d3b7412c383e25d09c986cc86935425f8bec07fc Mon Sep 17 00:00:00 2001 From: dignifiedquire Date: Wed, 29 Jul 2026 16:30:53 +0200 Subject: [PATCH 07/11] refactor(udp): take the buffer alignment from the cmsg macros, not the payloads The payload union was doing two jobs: sizing a message slot, and aligning the buffer. Only the first is its own. What a buffer has to be aligned to is where the `CMSG_*` macros lay messages out, which is a property of the platform, so `PAYLOAD_ALIGN` now comes from `CMSG_LEN` and `CMSG_SPACE` alone and the buffer carries a plain `repr(align(16))`, asserted at compile time to be at least that. 16 is a literal because `repr(align)` takes no expression; NetBSD and OpenBSD on sparc64 are the strictest at 16, and a platform wanting more is a compile error rather than a silent misalignment. Taking the alignment from the payloads was sound but understated it: on FreeBSD and NetBSD every payload we use is 4 byte aligned, so the buffer claimed 4 where the platform lays messages out at 8. The union keeps its real job, which is telling us how large a payload slot has to be. --- noq-udp/src/cmsg/unix.rs | 51 ++++++++++++++++++++----------------- noq-udp/src/cmsg/windows.rs | 28 ++++++++++++-------- 2 files changed, 44 insertions(+), 35 deletions(-) diff --git a/noq-udp/src/cmsg/unix.rs b/noq-udp/src/cmsg/unix.rs index 1c760b67ad..1d5807c7a2 100644 --- a/noq-udp/src/cmsg/unix.rs +++ b/noq-udp/src/cmsg/unix.rs @@ -10,13 +10,11 @@ use crate::imp::IpTosTy; /// Every payload we put into, or read out of, a control message on this platform. /// -/// A control message buffer must be aligned for the payloads it carries, and a union's -/// alignment is the strictest of its fields. [`libc::cmsghdr`] on its own is not enough: -/// musl declares it with `socklen_t` and `c_int` fields (alignment 4) where glibc uses -/// `size_t` (alignment 8), yet both carry payloads like [`libc::timespec`] needing 8. +/// A payload slot has to hold any one of these, so the largest of them is what sizes a +/// message. Listing them as a union is what lets the compiler work that out. #[derive(Copy, Clone)] #[repr(C)] -#[allow(dead_code)] // the fields are here for their alignment, nothing reads them +#[allow(dead_code)] // the fields are here for their size, nothing reads them pub(crate) union Payload { hdr: libc::cmsghdr, #[cfg(not(target_os = "netbsd"))] @@ -50,18 +48,16 @@ const fn common_align(a: usize, b: usize) -> usize { 1 << (a | b).trailing_zeros() } -/// The alignment a control message payload is guaranteed to have. +/// The alignment control messages are laid out at. /// -/// A payload sits `CMSG_LEN(0)` bytes into its message, and messages sit a sum of -/// `CMSG_SPACE` values into the buffer, so a payload can only be assumed to have the -/// alignment those offsets share, and no more than the buffer itself has. The platforms -/// spread wider than they look, from 4 bytes on Darwin and the Solarish platforms to 16 on -/// NetBSD and OpenBSD on sparc64, so this takes it from their own macros rather than -/// restating it here. Checked against real pointers by `payloads_are_aligned`. -pub(crate) const PAYLOAD_ALIGN: usize = common_align( - common_align(unsafe { libc::CMSG_LEN(0) } as usize, cmsg_space(1)), - align_of::(), -); +/// A payload sits `CMSG_LEN(0)` bytes into its message and messages sit a sum of +/// `CMSG_SPACE` values into the buffer, so this is the alignment those offsets share, and +/// what a payload has given a buffer aligned to at least as much. It is a property of the +/// platform, not of our payloads: the spread runs from 4 bytes on Darwin and the Solarish +/// platforms to 16 on NetBSD and OpenBSD on sparc64, so take it from their own macros +/// rather than restating it here. Checked against real pointers by `payloads_are_aligned`. +pub(crate) const PAYLOAD_ALIGN: usize = + common_align(unsafe { libc::CMSG_LEN(0) } as usize, cmsg_space(1)); /// Space for one control message carrying any of our payloads. const MESSAGE_LEN: usize = cmsg_space(size_of::()); @@ -78,15 +74,23 @@ pub(crate) const SEND_LEN: usize = 3 * MESSAGE_LEN; /// timestamp, one each, matching the socket options `UdpSocketState::new` enables. pub(crate) const RECV_LEN: usize = 4 * MESSAGE_LEN; -/// A control message buffer of `N` bytes, aligned for every [`Payload`]. +/// A control message buffer of `N` bytes. +/// +/// The alignment is what the `CMSG_*` macros lay messages out at, i.e. [`PAYLOAD_ALIGN`]; +/// 16 covers every platform we support, NetBSD and OpenBSD on sparc64 being the strictest. +/// It is a literal because `repr(align)` takes no expression, so the assertion below is +/// what keeps it honest. #[derive(Copy, Clone)] -#[repr(C)] +#[repr(align(16))] pub(crate) struct ControlBuf { - /// Zero sized, present only to give the buffer [`Payload`]'s alignment. - _align: [Payload; 0], bytes: [MaybeUninit; N], } +const _: () = assert!( + align_of::>() >= PAYLOAD_ALIGN, + "control message buffers are less aligned than the platform lays messages out at", +); + /// Control message buffer for one `sendmsg`. pub(crate) type SendBuf = ControlBuf; @@ -97,7 +101,6 @@ impl ControlBuf { /// A zeroed buffer, for sending. pub(crate) const fn zeroed() -> Self { Self { - _align: [], bytes: [MaybeUninit::new(0); N], } } @@ -105,7 +108,6 @@ impl ControlBuf { /// An uninitialised buffer, for receiving: the kernel initialises what it uses. pub(crate) const fn uninit() -> Self { Self { - _align: [], bytes: [MaybeUninit::uninit(); N], } } @@ -274,8 +276,9 @@ mod tests { /// Every payload in a full buffer is aligned for the type read out of it. /// - /// What `cmsg::decode` relies on, and what breaks on musl if the buffer takes its - /// alignment from `libc::cmsghdr` rather than from the payloads. + /// What `cmsg::decode` relies on, and what breaks on musl if the buffer is aligned for + /// `libc::cmsghdr` (4 bytes there, 8 on glibc) rather than for the layout the `CMSG_*` + /// macros use. #[test] fn payloads_are_aligned() { let mut buf = RecvBuf::zeroed(); diff --git a/noq-udp/src/cmsg/windows.rs b/noq-udp/src/cmsg/windows.rs index 023032c0be..1e0dd17b26 100644 --- a/noq-udp/src/cmsg/windows.rs +++ b/noq-udp/src/cmsg/windows.rs @@ -10,11 +10,11 @@ use super::{CMsgHdr, Encoder, MsgHdr}; /// Every payload we put into, or read out of, a control message on this platform. /// -/// A control message buffer must be aligned for the payloads it carries, and a union's -/// alignment is the strictest of its fields. +/// A payload slot has to hold any one of these, so the largest of them is what sizes a +/// message. Listing them as a union is what lets the compiler work that out. #[derive(Copy, Clone)] #[repr(C)] -#[allow(dead_code)] // the fields are here for their alignment, nothing reads them +#[allow(dead_code)] // the fields are here for their size, nothing reads them pub(crate) union Payload { hdr: WinSock::CMSGHDR, ecn: c_int, @@ -23,11 +23,11 @@ pub(crate) union Payload { pktinfo_v6: WinSock::IN6_PKTINFO, } -/// The alignment a control message payload is guaranteed to have. +/// The alignment control messages are laid out at. /// /// `WSA_CMSG_DATA` rounds the header size up to this and `WSA_CMSG_SPACE` keeps every -/// following header at a multiple of it, so payloads are this aligned as long as the -/// buffer is, which [`Payload`] ensures. +/// following header at a multiple of it, so this is what a payload has given a buffer +/// aligned to at least as much. pub(crate) const PAYLOAD_ALIGN: usize = mem::align_of::(); /// Set in `dwFlags` when control messages did not fit in the buffer. @@ -70,15 +70,22 @@ pub(crate) const SEND_LEN: usize = 3 * MESSAGE_LEN; /// The ECN codepoint, the packet info and the URO coalesced size, one each. pub(crate) const RECV_LEN: usize = 3 * MESSAGE_LEN; -/// A control message buffer of `N` bytes, aligned for every [`Payload`]. +/// A control message buffer of `N` bytes. +/// +/// The alignment is what the `WSA_CMSG_*` macros lay messages out at, i.e. +/// [`PAYLOAD_ALIGN`]. It is a literal because `repr(align)` takes no expression, so the +/// assertion below is what keeps it honest. #[derive(Copy, Clone)] -#[repr(C)] +#[repr(align(16))] pub(crate) struct ControlBuf { - /// Zero sized, present only to give the buffer [`Payload`]'s alignment. - _align: [Payload; 0], bytes: [MaybeUninit; N], } +const _: () = assert!( + mem::align_of::>() >= PAYLOAD_ALIGN, + "control message buffers are less aligned than the platform lays messages out at", +); + /// Control message buffer for one `WSASendMsg`. pub(crate) type SendBuf = ControlBuf; @@ -89,7 +96,6 @@ impl ControlBuf { /// A zeroed buffer. pub(crate) const fn zeroed() -> Self { Self { - _align: [], bytes: [MaybeUninit::new(0); N], } } From b0cc69b557476eefec226cfde09549a107448580 Mon Sep 17 00:00:00 2001 From: dignifiedquire Date: Wed, 29 Jul 2026 16:36:20 +0200 Subject: [PATCH 08/11] refactor(udp): align control message buffers like a `size_t` `repr(align(16))` covered the strictest platform but overshot everywhere else, and put a literal back in the file. Borrowing the alignment from a `usize`, what the `CMSG_*` macros round their offsets to, gives the same `PAYLOAD_ALIGN` on every target without over-aligning: the two 16 byte ABIs simply cap the guarantee at 8, which is more than any payload there needs. --- noq-udp/src/cmsg/unix.rs | 35 +++++++++++++++++------------------ noq-udp/src/cmsg/windows.rs | 15 ++++++--------- 2 files changed, 23 insertions(+), 27 deletions(-) diff --git a/noq-udp/src/cmsg/unix.rs b/noq-udp/src/cmsg/unix.rs index 1d5807c7a2..b7b3aabd63 100644 --- a/noq-udp/src/cmsg/unix.rs +++ b/noq-udp/src/cmsg/unix.rs @@ -48,16 +48,18 @@ const fn common_align(a: usize, b: usize) -> usize { 1 << (a | b).trailing_zeros() } -/// The alignment control messages are laid out at. +/// The alignment a control message payload is guaranteed to have. /// /// A payload sits `CMSG_LEN(0)` bytes into its message and messages sit a sum of -/// `CMSG_SPACE` values into the buffer, so this is the alignment those offsets share, and -/// what a payload has given a buffer aligned to at least as much. It is a property of the -/// platform, not of our payloads: the spread runs from 4 bytes on Darwin and the Solarish -/// platforms to 16 on NetBSD and OpenBSD on sparc64, so take it from their own macros -/// rather than restating it here. Checked against real pointers by `payloads_are_aligned`. -pub(crate) const PAYLOAD_ALIGN: usize = - common_align(unsafe { libc::CMSG_LEN(0) } as usize, cmsg_space(1)); +/// `CMSG_SPACE` values into the buffer, so it is the alignment those offsets share, and no +/// more than [`ControlBuf`] itself has. Both come from the platform rather than from our +/// payloads: the macros round to anything from 4 bytes on Darwin and the Solarish +/// platforms to 16 on NetBSD and OpenBSD on sparc64, where the buffer is the weaker of the +/// two. Checked against real pointers by `payloads_are_aligned`. +pub(crate) const PAYLOAD_ALIGN: usize = common_align( + common_align(unsafe { libc::CMSG_LEN(0) } as usize, cmsg_space(1)), + align_of::>(), +); /// Space for one control message carrying any of our payloads. const MESSAGE_LEN: usize = cmsg_space(size_of::()); @@ -76,21 +78,16 @@ pub(crate) const RECV_LEN: usize = 4 * MESSAGE_LEN; /// A control message buffer of `N` bytes. /// -/// The alignment is what the `CMSG_*` macros lay messages out at, i.e. [`PAYLOAD_ALIGN`]; -/// 16 covers every platform we support, NetBSD and OpenBSD on sparc64 being the strictest. -/// It is a literal because `repr(align)` takes no expression, so the assertion below is -/// what keeps it honest. +/// Aligned like a `size_t`, which is what the `CMSG_*` macros round their offsets to. The +/// zero sized field is how a type borrows another's alignment, `repr(align)` taking a +/// literal rather than an expression. #[derive(Copy, Clone)] -#[repr(align(16))] +#[repr(C)] pub(crate) struct ControlBuf { + _align: [usize; 0], bytes: [MaybeUninit; N], } -const _: () = assert!( - align_of::>() >= PAYLOAD_ALIGN, - "control message buffers are less aligned than the platform lays messages out at", -); - /// Control message buffer for one `sendmsg`. pub(crate) type SendBuf = ControlBuf; @@ -101,6 +98,7 @@ impl ControlBuf { /// A zeroed buffer, for sending. pub(crate) const fn zeroed() -> Self { Self { + _align: [], bytes: [MaybeUninit::new(0); N], } } @@ -108,6 +106,7 @@ impl ControlBuf { /// An uninitialised buffer, for receiving: the kernel initialises what it uses. pub(crate) const fn uninit() -> Self { Self { + _align: [], bytes: [MaybeUninit::uninit(); N], } } diff --git a/noq-udp/src/cmsg/windows.rs b/noq-udp/src/cmsg/windows.rs index 1e0dd17b26..04eab0ca2d 100644 --- a/noq-udp/src/cmsg/windows.rs +++ b/noq-udp/src/cmsg/windows.rs @@ -72,20 +72,16 @@ pub(crate) const RECV_LEN: usize = 3 * MESSAGE_LEN; /// A control message buffer of `N` bytes. /// -/// The alignment is what the `WSA_CMSG_*` macros lay messages out at, i.e. -/// [`PAYLOAD_ALIGN`]. It is a literal because `repr(align)` takes no expression, so the -/// assertion below is what keeps it honest. +/// Aligned like a `usize`, which is what `WSA_CMSGDATA_ALIGN` rounds to, i.e. +/// [`PAYLOAD_ALIGN`]. The zero sized field is how a type borrows another's alignment, +/// `repr(align)` taking a literal rather than an expression. #[derive(Copy, Clone)] -#[repr(align(16))] +#[repr(C)] pub(crate) struct ControlBuf { + _align: [usize; 0], bytes: [MaybeUninit; N], } -const _: () = assert!( - mem::align_of::>() >= PAYLOAD_ALIGN, - "control message buffers are less aligned than the platform lays messages out at", -); - /// Control message buffer for one `WSASendMsg`. pub(crate) type SendBuf = ControlBuf; @@ -96,6 +92,7 @@ impl ControlBuf { /// A zeroed buffer. pub(crate) const fn zeroed() -> Self { Self { + _align: [], bytes: [MaybeUninit::new(0); N], } } From 765b1207b5b14795e584e0eb7945d1357446a16d Mon Sep 17 00:00:00 2001 From: dignifiedquire Date: Wed, 29 Jul 2026 16:43:20 +0200 Subject: [PATCH 09/11] docs(udp): tighten the cmsg comments and cite the headers They had grown into prose. Cut to the point in each, and reference the headers behind the claims the way the rest of the crate does: musl's and glibc's `cmsghdr` for the alignment they differ on, cmsg(3) for the macros, and the Winsock pktinfo structs. --- noq-udp/src/cmsg/mod.rs | 10 ++++----- noq-udp/src/cmsg/unix.rs | 44 +++++++++++++++---------------------- noq-udp/src/cmsg/windows.rs | 26 ++++++++++------------ noq-udp/tests/tests.rs | 7 +++--- 4 files changed, 37 insertions(+), 50 deletions(-) diff --git a/noq-udp/src/cmsg/mod.rs b/noq-udp/src/cmsg/mod.rs index dac77aed4b..58c6c0934d 100644 --- a/noq-udp/src/cmsg/mod.rs +++ b/noq-udp/src/cmsg/mod.rs @@ -40,8 +40,7 @@ impl<'a, M: MsgHdr> Encoder<'a, M> { /// Append a control message to the buffer. /// - /// Private: each message we send has its own method, declared next to the buffer size - /// covering it. + /// Private: each message we send has its own method, next to the size covering it. /// /// # Panics /// - If insufficient buffer space remains. @@ -84,10 +83,9 @@ impl Drop for Encoder<'_, M> { /// Warns once if the kernel had more to say about a datagram than the buffer could hold. /// -/// Dropped control messages mean lost metadata, in the worst case the GRO segment size, -/// which leaves a coalesced datagram looking like a single one. `RECV_LEN` covers every -/// option we enable, so this means either a new one is unaccounted for, or the caller -/// enabled one of their own on the socket they gave us. +/// The dropped messages cost us metadata, at worst the GRO segment size. `RECV_LEN` covers +/// every option we enable, so one is unaccounted for, or the caller enabled their own on +/// the socket they gave us. pub(crate) fn warn_if_control_truncated(hdr: &impl MsgHdr) { static WARNED: AtomicBool = AtomicBool::new(false); diff --git a/noq-udp/src/cmsg/unix.rs b/noq-udp/src/cmsg/unix.rs index b7b3aabd63..44cd938fe5 100644 --- a/noq-udp/src/cmsg/unix.rs +++ b/noq-udp/src/cmsg/unix.rs @@ -10,8 +10,7 @@ use crate::imp::IpTosTy; /// Every payload we put into, or read out of, a control message on this platform. /// -/// A payload slot has to hold any one of these, so the largest of them is what sizes a -/// message. Listing them as a union is what lets the compiler work that out. +/// A payload slot holds any one of these, so the largest of them sizes a message. #[derive(Copy, Clone)] #[repr(C)] #[allow(dead_code)] // the fields are here for their size, nothing reads them @@ -37,25 +36,21 @@ pub(crate) union Payload { pub(crate) const MSG_CTRUNC: c_int = libc::MSG_CTRUNC; /// The buffer space one control message with a payload of this size takes up. +// https://man7.org/linux/man-pages/man3/cmsg.3.html const fn cmsg_space(payload_len: usize) -> usize { unsafe { libc::CMSG_SPACE(payload_len as _) as usize } } /// The weaker of two alignments, i.e. the largest power of two dividing both. const fn common_align(a: usize, b: usize) -> usize { - // Whichever of the two has its lowest set bit first decides the trailing zeros of the - // OR, and both are offsets a payload can sit at. + // The lower of the two lowest set bits decides the trailing zeros of the OR. 1 << (a | b).trailing_zeros() } /// The alignment a control message payload is guaranteed to have. /// -/// A payload sits `CMSG_LEN(0)` bytes into its message and messages sit a sum of -/// `CMSG_SPACE` values into the buffer, so it is the alignment those offsets share, and no -/// more than [`ControlBuf`] itself has. Both come from the platform rather than from our -/// payloads: the macros round to anything from 4 bytes on Darwin and the Solarish -/// platforms to 16 on NetBSD and OpenBSD on sparc64, where the buffer is the weaker of the -/// two. Checked against real pointers by `payloads_are_aligned`. +/// Payloads sit `CMSG_LEN(0)` into their message and messages a sum of `CMSG_SPACE`s into +/// the buffer, so it is what those offsets and [`ControlBuf`]'s alignment share. pub(crate) const PAYLOAD_ALIGN: usize = common_align( common_align(unsafe { libc::CMSG_LEN(0) } as usize, cmsg_space(1)), align_of::>(), @@ -66,24 +61,21 @@ const MESSAGE_LEN: usize = cmsg_space(size_of::()); /// Space for the control messages one `sendmsg` can carry. /// -/// ECN, the GSO segment size and the source address, one each: the IPv4 and IPv6 forms -/// are mutually exclusive. +/// ECN, GSO segment size and source address, one each; the v4 and v6 forms are exclusive. pub(crate) const SEND_LEN: usize = 3 * MESSAGE_LEN; /// Space for the control messages the kernel can attach to one received datagram. /// -/// The TOS or traffic class, the packet info, the GRO segment size and the receive -/// timestamp, one each, matching the socket options `UdpSocketState::new` enables. +/// TOS or traffic class, packet info, GRO segment size and receive timestamp, one each, +/// matching the options `UdpSocketState::new` enables. pub(crate) const RECV_LEN: usize = 4 * MESSAGE_LEN; /// A control message buffer of `N` bytes. -/// -/// Aligned like a `size_t`, which is what the `CMSG_*` macros round their offsets to. The -/// zero sized field is how a type borrows another's alignment, `repr(align)` taking a -/// literal rather than an expression. #[derive(Copy, Clone)] #[repr(C)] pub(crate) struct ControlBuf { + /// Aligns the buffer like the `size_t` the `CMSG_*` macros round offsets to. + /// Zero sized: `repr(align)` takes a literal, not an expression. _align: [usize; 0], bytes: [MaybeUninit; N], } @@ -123,8 +115,8 @@ impl ControlBuf { /// The control messages we send. /// -/// One method each rather than a generic `push`, so the set stays next to the [`SEND_LEN`] -/// that has to cover it. +/// One method each rather than a generic `push`, keeping the set next to the [`SEND_LEN`] +/// covering it. impl> Encoder<'_, M> { /// Sets the ECN codepoint of an IPv4 or IPv4-mapped datagram. #[cfg(not(target_os = "netbsd"))] @@ -258,9 +250,8 @@ mod tests { /// The buffers hold every control message they have to. /// - /// [`SEND_LEN`] and [`RECV_LEN`] count messages and assume the largest payload; this - /// adds up the real ones, so a message we forgot to count shows up here rather than as - /// a truncated datagram. + /// The constants count messages and assume the largest payload; this adds up the real + /// ones, so a message we failed to count shows up here, not as a truncated datagram. #[test] fn control_len_covers_libc() { let sent = libc_cmsg_space(&sent_payload_lens()); @@ -275,9 +266,10 @@ mod tests { /// Every payload in a full buffer is aligned for the type read out of it. /// - /// What `cmsg::decode` relies on, and what breaks on musl if the buffer is aligned for - /// `libc::cmsghdr` (4 bytes there, 8 on glibc) rather than for the layout the `CMSG_*` - /// macros use. + /// What `cmsg::decode` relies on. musl aligns `cmsghdr` to 4 where glibc aligns it to + /// 8, so aligning the buffer for it rather than for the macros breaks there. + // https://github.com/kraj/musl/blob/master/include/sys/socket.h#L44 + // https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/bits/socket.h#L283 #[test] fn payloads_are_aligned() { let mut buf = RecvBuf::zeroed(); diff --git a/noq-udp/src/cmsg/windows.rs b/noq-udp/src/cmsg/windows.rs index 04eab0ca2d..379b30ae57 100644 --- a/noq-udp/src/cmsg/windows.rs +++ b/noq-udp/src/cmsg/windows.rs @@ -10,8 +10,9 @@ use super::{CMsgHdr, Encoder, MsgHdr}; /// Every payload we put into, or read out of, a control message on this platform. /// -/// A payload slot has to hold any one of these, so the largest of them is what sizes a -/// message. Listing them as a union is what lets the compiler work that out. +/// A payload slot holds any one of these, so the largest of them sizes a message. +// https://learn.microsoft.com/en-us/windows/win32/api/ws2ipdef/ns-ws2ipdef-in_pktinfo +// https://learn.microsoft.com/en-us/windows/win32/api/ws2ipdef/ns-ws2ipdef-in6_pktinfo #[derive(Copy, Clone)] #[repr(C)] #[allow(dead_code)] // the fields are here for their size, nothing reads them @@ -23,11 +24,10 @@ pub(crate) union Payload { pktinfo_v6: WinSock::IN6_PKTINFO, } -/// The alignment control messages are laid out at. +/// The alignment a control message payload is guaranteed to have. /// -/// `WSA_CMSG_DATA` rounds the header size up to this and `WSA_CMSG_SPACE` keeps every -/// following header at a multiple of it, so this is what a payload has given a buffer -/// aligned to at least as much. +/// `WSA_CMSG_DATA` rounds the header size up to it and `WSA_CMSG_SPACE` keeps every +/// following header at a multiple of it, [`ControlBuf`] having at least as much. pub(crate) const PAYLOAD_ALIGN: usize = mem::align_of::(); /// Set in `dwFlags` when control messages did not fit in the buffer. @@ -71,13 +71,11 @@ pub(crate) const SEND_LEN: usize = 3 * MESSAGE_LEN; pub(crate) const RECV_LEN: usize = 3 * MESSAGE_LEN; /// A control message buffer of `N` bytes. -/// -/// Aligned like a `usize`, which is what `WSA_CMSGDATA_ALIGN` rounds to, i.e. -/// [`PAYLOAD_ALIGN`]. The zero sized field is how a type borrows another's alignment, -/// `repr(align)` taking a literal rather than an expression. #[derive(Copy, Clone)] #[repr(C)] pub(crate) struct ControlBuf { + /// Aligns the buffer like the `usize` `WSA_CMSGDATA_ALIGN` rounds to. + /// Zero sized: `repr(align)` takes a literal, not an expression. _align: [usize; 0], bytes: [MaybeUninit; N], } @@ -109,8 +107,8 @@ impl ControlBuf { /// The control messages we send. /// -/// One method each rather than a generic `push`, so the set stays next to the [`SEND_LEN`] -/// that has to cover it. +/// One method each rather than a generic `push`, keeping the set next to the [`SEND_LEN`] +/// covering it. impl> Encoder<'_, M> { /// Sets the ECN codepoint of an IPv4 datagram. pub(crate) fn push_ecn_v4(&mut self, ecn: c_int) { @@ -215,8 +213,8 @@ mod tests { /// Every payload in a full buffer is aligned for the type read out of it. /// - /// Encoding the whole set the send path can produce also proves [`SEND_LEN`] covers it, - /// since [`Encoder::push`] panics rather than overrun the buffer. + /// Encoding the whole send set also proves [`SEND_LEN`] covers it, `push` panicking + /// rather than overrunning. #[test] fn payloads_are_aligned() { let mut buf = SendBuf::zeroed(); diff --git a/noq-udp/tests/tests.rs b/noq-udp/tests/tests.rs index d6b9b66c77..67e1097257 100644 --- a/noq-udp/tests/tests.rs +++ b/noq-udp/tests/tests.rs @@ -246,10 +246,9 @@ fn gso() { /// A datagram that is both segmented and ECN marked. /// -/// The traffic class arrives after the GRO segment size and the timestamp, so a receive -/// buffer that is too small drops it and ECN feedback silently disappears. `gso` sends no -/// ECN and the `ecn_*` tests send a single segment, so neither puts enough control -/// messages on one datagram to notice. +/// The traffic class arrives after the GRO segment size and the timestamp, so too small a +/// receive buffer drops it and ECN feedback disappears. `gso` sends no ECN and the `ecn_*` +/// tests send one segment, so neither puts enough control messages on a datagram to notice. #[test] #[cfg_attr(not(any(target_os = "linux", target_os = "android")), ignore)] fn gso_with_ecn() { From f317ebc7d6b54883c996c7f13585e31d2124303c Mon Sep 17 00:00:00 2001 From: dignifiedquire Date: Wed, 29 Jul 2026 16:46:46 +0200 Subject: [PATCH 10/11] docs(udp): quote the reference links and cite them upstream Bare URLs are not links; wrap them so rustdoc renders them, including the two Winsock ones that were already there. musl and glibc are cited from their own SCMs rather than the GitHub mirrors. --- noq-udp/src/cmsg/unix.rs | 8 +++++--- noq-udp/src/cmsg/windows.rs | 17 ++++++++++------- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/noq-udp/src/cmsg/unix.rs b/noq-udp/src/cmsg/unix.rs index 44cd938fe5..3864394112 100644 --- a/noq-udp/src/cmsg/unix.rs +++ b/noq-udp/src/cmsg/unix.rs @@ -36,7 +36,8 @@ pub(crate) union Payload { pub(crate) const MSG_CTRUNC: c_int = libc::MSG_CTRUNC; /// The buffer space one control message with a payload of this size takes up. -// https://man7.org/linux/man-pages/man3/cmsg.3.html +/// +/// const fn cmsg_space(payload_len: usize) -> usize { unsafe { libc::CMSG_SPACE(payload_len as _) as usize } } @@ -268,8 +269,9 @@ mod tests { /// /// What `cmsg::decode` relies on. musl aligns `cmsghdr` to 4 where glibc aligns it to /// 8, so aligning the buffer for it rather than for the macros breaks there. - // https://github.com/kraj/musl/blob/master/include/sys/socket.h#L44 - // https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/bits/socket.h#L283 + /// + /// + /// #[test] fn payloads_are_aligned() { let mut buf = RecvBuf::zeroed(); diff --git a/noq-udp/src/cmsg/windows.rs b/noq-udp/src/cmsg/windows.rs index 379b30ae57..5eac7e387e 100644 --- a/noq-udp/src/cmsg/windows.rs +++ b/noq-udp/src/cmsg/windows.rs @@ -11,8 +11,9 @@ use super::{CMsgHdr, Encoder, MsgHdr}; /// Every payload we put into, or read out of, a control message on this platform. /// /// A payload slot holds any one of these, so the largest of them sizes a message. -// https://learn.microsoft.com/en-us/windows/win32/api/ws2ipdef/ns-ws2ipdef-in_pktinfo -// https://learn.microsoft.com/en-us/windows/win32/api/ws2ipdef/ns-ws2ipdef-in6_pktinfo +/// +/// +/// #[derive(Copy, Clone)] #[repr(C)] #[allow(dead_code)] // the fields are here for their size, nothing reads them @@ -34,7 +35,7 @@ pub(crate) const PAYLOAD_ALIGN: usize = mem::align_of::(); pub(crate) const MSG_CTRUNC: c_int = WinSock::MSG_CTRUNC as c_int; // The four functions below follow the C macros in -// https://github.com/microsoft/win32metadata/blob/main/generation/WinSDK/RecompiledIdlHeaders/shared/ws2def.h#L741 +// /// `WSA_CMSG_ALIGN`, which control message headers are aligned to. const fn cmsghdr_align(len: usize) -> usize { @@ -143,8 +144,9 @@ impl> Encoder<'_, M> { } /// Helpers for [`WinSock::WSAMSG`] -// https://learn.microsoft.com/en-us/windows/win32/api/ws2def/ns-ws2def-wsamsg -// https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/Networking/WinSock/struct.WSAMSG.html +/// +/// +/// impl MsgHdr for WinSock::WSAMSG { type ControlMessage = WinSock::CMSGHDR; @@ -181,8 +183,9 @@ impl MsgHdr for WinSock::WSAMSG { } /// Helpers for [`WinSock::CMSGHDR`] -// https://learn.microsoft.com/en-us/windows/win32/api/ws2def/ns-ws2def-wsacmsghdr -// https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/Networking/WinSock/struct.CMSGHDR.html +/// +/// +/// impl CMsgHdr for WinSock::CMSGHDR { fn cmsg_len(length: usize) -> usize { cmsg_len(length) From ec969f124aa0dadbea22cefe06d7116a94c01562 Mon Sep 17 00:00:00 2001 From: dignifiedquire Date: Wed, 29 Jul 2026 16:54:22 +0200 Subject: [PATCH 11/11] refactor(udp): drop `cmsghdr` from the payload unions It was there when the union carried the buffer's alignment, since the buffer holds headers as well as payloads, the C `union { struct cmsghdr hdr; char buf[N]; }` idiom. The alignment now comes from a `usize`, which is at least as strict as any platform's `cmsghdr`, so all the member did was feed `size_of::()` a header that `CMSG_SPACE` already accounts for. No constant moves on any target we build: `in6_pktinfo` is the largest payload everywhere `cmsghdr` is not. A union named for payloads now holds only those. --- noq-udp/src/cmsg/unix.rs | 4 ++-- noq-udp/src/cmsg/windows.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/noq-udp/src/cmsg/unix.rs b/noq-udp/src/cmsg/unix.rs index 3864394112..26d28894e2 100644 --- a/noq-udp/src/cmsg/unix.rs +++ b/noq-udp/src/cmsg/unix.rs @@ -15,7 +15,6 @@ use crate::imp::IpTosTy; #[repr(C)] #[allow(dead_code)] // the fields are here for their size, nothing reads them pub(crate) union Payload { - hdr: libc::cmsghdr, #[cfg(not(target_os = "netbsd"))] ecn_v4: IpTosTy, ecn_v6: c_int, @@ -75,7 +74,8 @@ pub(crate) const RECV_LEN: usize = 4 * MESSAGE_LEN; #[derive(Copy, Clone)] #[repr(C)] pub(crate) struct ControlBuf { - /// Aligns the buffer like the `size_t` the `CMSG_*` macros round offsets to. + /// Aligns the buffer like the `size_t` the `CMSG_*` macros round offsets to, which + /// covers the headers too: no platform aligns `cmsghdr` more strictly than that. /// Zero sized: `repr(align)` takes a literal, not an expression. _align: [usize; 0], bytes: [MaybeUninit; N], diff --git a/noq-udp/src/cmsg/windows.rs b/noq-udp/src/cmsg/windows.rs index 5eac7e387e..3dea5cb07e 100644 --- a/noq-udp/src/cmsg/windows.rs +++ b/noq-udp/src/cmsg/windows.rs @@ -18,7 +18,6 @@ use super::{CMsgHdr, Encoder, MsgHdr}; #[repr(C)] #[allow(dead_code)] // the fields are here for their size, nothing reads them pub(crate) union Payload { - hdr: WinSock::CMSGHDR, ecn: c_int, segment_size: u32, pktinfo_v4: WinSock::IN_PKTINFO, @@ -75,7 +74,8 @@ pub(crate) const RECV_LEN: usize = 3 * MESSAGE_LEN; #[derive(Copy, Clone)] #[repr(C)] pub(crate) struct ControlBuf { - /// Aligns the buffer like the `usize` `WSA_CMSGDATA_ALIGN` rounds to. + /// Aligns the buffer like the `usize` `WSA_CMSGDATA_ALIGN` rounds to, which covers + /// the headers too: `CMSGHDR` is a `SIZE_T` and two `INT`s. /// Zero sized: `repr(align)` takes a literal, not an expression. _align: [usize; 0], bytes: [MaybeUninit; N],