From 9f4e87764fecb4e6a97f5cc2d9a40cd3a9a54aa9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Ad=C3=A4?= <35699107+Jan561@users.noreply.github.com> Date: Sun, 1 Oct 2023 20:59:36 +0200 Subject: [PATCH] fixes UB for recvmmsg, simplifies function signature of recvmmsg and sendmmsg (#2120) --- CHANGELOG.md | 5 +++++ src/sys/socket/mod.rs | 44 ++++++++++++++++++++++++++--------------- test/sys/test_socket.rs | 19 +++++++++++------- 3 files changed, 45 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 69f628d8..d876b760 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,9 @@ This project adheres to [Semantic Versioning](https://semver.org/). - Fix `SigSet` incorrect implementation of `Eq`, `PartialEq` and `Hash` ([#1946](https://github.com/nix-rust/nix/pull/1946)) +- Fixed the function signature of `recvmmsg`, potentially causing UB + ([#2119](https://github.com/nix-rust/nix/issues/2119)) + ### Changed - The following APIs now take an implementation of `AsFd` rather than a @@ -33,6 +36,8 @@ This project adheres to [Semantic Versioning](https://semver.org/). relaxed lifetime requirements relative to 0.27.1. ([#2136](https://github.com/nix-rust/nix/pull/2136)) +- Simplified the function signatures of `recvmmsg` and `sendmmsg` + ## [0.27.1] - 2023-08-28 ### Fixed diff --git a/src/sys/socket/mod.rs b/src/sys/socket/mod.rs index e03b1d69..69933bd1 100644 --- a/src/sys/socket/mod.rs +++ b/src/sys/socket/mod.rs @@ -1552,11 +1552,11 @@ pub fn sendmmsg<'a, XS, AS, C, I, S>( flags: MsgFlags ) -> crate::Result> where - XS: IntoIterator, + XS: IntoIterator, AS: AsRef<[Option]>, - I: AsRef<[IoSlice<'a>]> + 'a, - C: AsRef<[ControlMessage<'a>]> + 'a, - S: SockaddrLike + 'a + I: AsRef<[IoSlice<'a>]>, + C: AsRef<[ControlMessage<'a>]>, + S: SockaddrLike, { let mut count = 0; @@ -1564,11 +1564,11 @@ pub fn sendmmsg<'a, XS, AS, C, I, S>( for (i, ((slice, addr), mmsghdr)) in slices.into_iter().zip(addrs.as_ref()).zip(data.items.iter_mut() ).enumerate() { let p = &mut mmsghdr.msg_hdr; - p.msg_iov = slice.as_ref().as_ptr() as *mut libc::iovec; + p.msg_iov = slice.as_ref().as_ptr().cast_mut().cast(); p.msg_iovlen = slice.as_ref().len() as _; p.msg_namelen = addr.as_ref().map_or(0, S::len); - p.msg_name = addr.as_ref().map_or(ptr::null(), S::as_ptr) as _; + p.msg_name = addr.as_ref().map_or(ptr::null(), S::as_ptr).cast_mut().cast(); // Encode each cmsg. This must happen after initializing the header because // CMSG_NEXT_HDR and friends read the msg_control and msg_controllen fields. @@ -1583,9 +1583,16 @@ pub fn sendmmsg<'a, XS, AS, C, I, S>( pmhdr = unsafe { CMSG_NXTHDR(p, pmhdr) }; } - count = i+1; + // Doing an unchecked addition is alright here, as the only way to obtain an instance of `MultiHeaders` + // is through the `preallocate` function, which takes an `usize` as an argument to define its size, + // which also provides an upper bound for the size of this zipped iterator. Thus, `i < usize::MAX` or in + // other words: `count` doesn't overflow + count = i + 1; } + // SAFETY: all pointers are guaranteed to be valid for the scope of this function. `count` does represent the + // maximum number of messages that can be sent safely (i.e. `count` is the minimum of the sizes of `slices`, + // `data.items` and `addrs`) let sent = Errno::result(unsafe { libc::sendmmsg( fd, @@ -1711,14 +1718,19 @@ pub fn recvmmsg<'a, XS, S, I>( mut timeout: Option, ) -> crate::Result> where - XS: IntoIterator, - I: AsRef<[IoSliceMut<'a>]> + 'a, + XS: IntoIterator, + I: AsMut<[IoSliceMut<'a>]>, { let mut count = 0; - for (i, (slice, mmsghdr)) in slices.into_iter().zip(data.items.iter_mut()).enumerate() { + for (i, (mut slice, mmsghdr)) in slices.into_iter().zip(data.items.iter_mut()).enumerate() { let p = &mut mmsghdr.msg_hdr; - p.msg_iov = slice.as_ref().as_ptr() as *mut libc::iovec; - p.msg_iovlen = slice.as_ref().len() as _; + p.msg_iov = slice.as_mut().as_mut_ptr().cast(); + p.msg_iovlen = slice.as_mut().len() as _; + + // Doing an unchecked addition is alright here, as the only way to obtain an instance of `MultiHeaders` + // is through the `preallocate` function, which takes an `usize` as an argument to define its size, + // which also provides an upper bound for the size of this zipped iterator. Thus, `i < usize::MAX` or in + // other words: `count` doesn't overflow count = i + 1; } @@ -1726,6 +1738,8 @@ where .as_mut() .map_or_else(std::ptr::null_mut, |t| t as *mut _ as *mut libc::timespec); + // SAFETY: all pointers are guaranteed to be valid for the scope of this function. `count` does represent the + // maximum number of messages that can be received safely (i.e. `count` is the minimum of the sizes of `slices` and `data.items`) let received = Errno::result(unsafe { libc::recvmmsg( fd, @@ -1743,6 +1757,7 @@ where }) } +/// Iterator over results of [`recvmmsg`]/[`sendmmsg`] #[cfg(any( target_os = "linux", target_os = "android", @@ -1750,9 +1765,6 @@ where target_os = "netbsd", ))] #[derive(Debug)] -/// Iterator over results of [`recvmmsg`]/[`sendmmsg`] -/// -/// pub struct MultiResults<'a, S> { // preallocated structures rmm: &'a MultiHeaders, @@ -1903,7 +1915,7 @@ mod test { let t = sys::time::TimeSpec::from_duration(std::time::Duration::from_secs(10)); - let recv = super::recvmmsg(rsock.as_raw_fd(), &mut data, recv_iovs.iter(), flags, Some(t))?; + let recv = super::recvmmsg(rsock.as_raw_fd(), &mut data, recv_iovs.iter_mut(), flags, Some(t))?; for rmsg in recv { #[cfg(not(any(qemu, target_arch = "aarch64")))] diff --git a/test/sys/test_socket.rs b/test/sys/test_socket.rs index df831899..4b55a712 100644 --- a/test/sys/test_socket.rs +++ b/test/sys/test_socket.rs @@ -564,7 +564,7 @@ mod recvfrom { let res: Vec> = recvmmsg( rsock.as_raw_fd(), &mut data, - msgs.iter(), + msgs.iter_mut(), MsgFlags::empty(), None, ) @@ -652,7 +652,7 @@ mod recvfrom { let res: Vec> = recvmmsg( rsock.as_raw_fd(), &mut data, - msgs.iter(), + msgs.iter_mut(), MsgFlags::MSG_DONTWAIT, None, ) @@ -2324,12 +2324,17 @@ fn test_recvmmsg_timestampns() { // Receive the message let mut buffer = vec![0u8; message.len()]; let cmsgspace = nix::cmsg_space!(TimeSpec); - let iov = vec![[IoSliceMut::new(&mut buffer)]]; + let mut iov = vec![[IoSliceMut::new(&mut buffer)]]; let mut data = MultiHeaders::preallocate(1, Some(cmsgspace)); - let r: Vec> = - recvmmsg(in_socket.as_raw_fd(), &mut data, iov.iter(), flags, None) - .unwrap() - .collect(); + let r: Vec> = recvmmsg( + in_socket.as_raw_fd(), + &mut data, + iov.iter_mut(), + flags, + None, + ) + .unwrap() + .collect(); let rtime = match r[0].cmsgs().next() { Some(ControlMessageOwned::ScmTimestampns(rtime)) => rtime, Some(_) => panic!("Unexpected control message"),