Merge #2041

2041: Set the length of a socket address when calling `recvmsg` on Linux r=asomers a=JarredAllen

# Background
I think I've found a bug with `recvmsg` on Linux where it doesn't set the length of a received socket address. I found this while working with unix datagram sockets with path `struct sockaddr_un` addresses, but I think this applies to any variable-length socket address type.

At present, the `sun_len` field of `UnixAddr` on Linux shows up as 0 whenever I call `recvmsg`. I think it's reading uninitialized memory (afaict the `recvmsg` function never initializes it before we call `MaybeUninit::assume_init`), though it's possible that it's being zero-initialized somewhere that I missed. Either way, this isn't the correct behavior, which should set it to the length of the `struct sockaddr_un` contents (or whatever other type of socket).

# Changes
I changed the `recvmsg` function to construct the returned socket address from the `struct sockaddr` and length returned by the libc `recvmsg` call (using the `from_raw` function the trait provides). Since the trait is sealed so downstream crates can't implement it, I believe this shouldn't be a breaking change.

# Validation
I've tested that my code (which failed due to this bug) now works. I also added a new test case which tests that we construct `UnixAddr`s correctly in the `recvmsg` function, which passes locally for me and fails if I cherry-pick it onto the current `master`.

I've also checked that `cargo test` and `cargo clippy` both still pass on `aarch64-apple-darwin` and on `aarch64-unknown-linux-musl` targets (the two targets I develop for/have access to). Hopefully your CI will confirm that everything else still works.

Co-authored-by: Jarred Allen <[email protected]>
Co-authored-by: Jarred Allen <[email protected]>

Author: bors[bot]

CHANGELOG.md

@@ -41,6 +41,8 @@ This project adheres to [Semantic Versioning](https://semver.org/).
 ### Fixed
 - Fix: send `ETH_P_ALL` in htons format 
   ([#1925](https://github.com/nix-rust/nix/pull/1925))
+- Fix: `recvmsg` now sets the length of the received `sockaddr_un` field
+  correctly on Linux platforms. ([#2041](https://github.com/nix-rust/nix/pull/2041))
 - Fix potentially invalid conversions in
   `SockaddrIn::from<std::net::SocketAddrV4>`,
   `SockaddrIn6::from<std::net::SockaddrV6>`, `IpMembershipRequest::new`, and

src/sys/socket/addr.rs

@@ -765,6 +765,22 @@ impl SockaddrLike for UnixAddr {
     {
         mem::size_of::<libc::sockaddr_un>() as libc::socklen_t
     }
+
+    unsafe fn set_length(&mut self, new_length: usize) -> std::result::Result<(), SocketAddressLengthNotDynamic> {
+        // `new_length` is only used on some platforms, so it must be provided even when not used
+        #![allow(unused_variables)]
+        cfg_if! {
+            if #[cfg(any(target_os = "android",
+                         target_os = "fuchsia",
+                         target_os = "illumos",
+                         target_os = "linux",
+                         target_os = "redox",
+                ))] {
+                self.sun_len = new_length as u8;
+            }
+        };
+        Ok(())
+    }
 }
 
 impl AsRef<libc::sockaddr_un> for UnixAddr {
@@ -914,7 +930,32 @@ pub trait SockaddrLike: private::SockaddrLikePriv {
     {
         mem::size_of::<Self>() as libc::socklen_t
     }
+
+    /// Set the length of this socket address
+    ///
+    /// This method may only be called on socket addresses whose lengths are dynamic, and it
+    /// returns an error if called on a type whose length is static.
+    ///
+    /// # Safety
+    ///
+    /// `new_length` must be a valid length for this type of address. Specifically, reads of that
+    /// length from `self` must be valid.
+    #[doc(hidden)]
+    unsafe fn set_length(&mut self, _new_length: usize) -> std::result::Result<(), SocketAddressLengthNotDynamic> {
+        Err(SocketAddressLengthNotDynamic)
+    }
+}
+
+/// The error returned by [`SockaddrLike::set_length`] on an address whose length is statically
+/// fixed.
+#[derive(Copy, Clone, Debug)]
+pub struct SocketAddressLengthNotDynamic;
+impl fmt::Display for SocketAddressLengthNotDynamic {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str("Attempted to set length on socket whose length is statically fixed")
+    }
 }
+impl std::error::Error for SocketAddressLengthNotDynamic {}
 
 impl private::SockaddrLikePriv for () {
     fn as_mut_ptr(&mut self) -> *mut libc::sockaddr {
@@ -1360,6 +1401,15 @@ impl SockaddrLike for SockaddrStorage {
             None => mem::size_of_val(self) as libc::socklen_t,
         }
     }
+
+    unsafe fn set_length(&mut self, new_length: usize) -> std::result::Result<(), SocketAddressLengthNotDynamic> {
+        match self.as_unix_addr_mut() {
+            Some(addr) => {
+                addr.set_length(new_length)
+            },
+            None => Err(SocketAddressLengthNotDynamic),
+        }
+    }
 }
 
 macro_rules! accessors {
@@ -1678,7 +1728,7 @@ impl PartialEq for SockaddrStorage {
     }
 }
 
-mod private {
+pub(super) mod private {
     pub trait SockaddrLikePriv {
         /// Returns a mutable raw pointer to the inner structure.
         ///
@@ -2215,7 +2265,6 @@ mod datalink {
             &self.0
         }
     }
-
     }
 }
 

src/sys/socket/mod.rs

@@ -1613,7 +1613,7 @@ impl<S> MultiHeaders<S> {
     {
         // we will be storing pointers to addresses inside mhdr - convert it into boxed
         // slice so it can'be changed later by pushing anything into self.addresses
-        let mut addresses = vec![std::mem::MaybeUninit::uninit(); num_slices].into_boxed_slice();
+        let mut addresses = vec![std::mem::MaybeUninit::<S>::uninit(); num_slices].into_boxed_slice();
 
         let msg_controllen = cmsg_buffer.as_ref().map_or(0, |v| v.capacity());
 
@@ -1918,7 +1918,7 @@ unsafe fn read_mhdr<'a, 'i, S>(
     mhdr: msghdr,
     r: isize,
     msg_controllen: usize,
-    address: S,
+    mut address: S,
 ) -> RecvMsg<'a, 'i, S>
     where S: SockaddrLike
 {
@@ -1934,6 +1934,11 @@ unsafe fn read_mhdr<'a, 'i, S>(
         }.as_ref()
     };
 
+    // Ignore errors if this socket address has statically-known length
+    //
+    // This is to ensure that unix socket addresses have their length set appropriately.
+    let _ = address.set_length(mhdr.msg_namelen as usize);
+
     RecvMsg {
         bytes: r as usize,
         cmsghdr,
@@ -1969,7 +1974,7 @@ unsafe fn pack_mhdr_to_receive<S>(
     // initialize it.
     let mut mhdr = mem::MaybeUninit::<msghdr>::zeroed();
     let p = mhdr.as_mut_ptr();
-    (*p).msg_name = (*address).as_mut_ptr() as *mut c_void;
+    (*p).msg_name = address as *mut c_void;
     (*p).msg_namelen = S::size();
     (*p).msg_iov = iov_buffer as *mut iovec;
     (*p).msg_iovlen = iov_buffer_len as _;

test/sys/test_socket.rs

@@ -202,6 +202,49 @@ pub fn test_socketpair() {
     assert_eq!(&buf[..], b"hello");
 }
 
+#[test]
+pub fn test_recvmsg_sockaddr_un() {
+    use nix::sys::socket::{
+        self, bind, socket, AddressFamily, MsgFlags, SockFlag, SockType,
+    };
+
+    let tempdir = tempfile::tempdir().unwrap();
+    let sockname = tempdir.path().join("sock");
+    let sock = socket(
+        AddressFamily::Unix,
+        SockType::Datagram,
+        SockFlag::empty(),
+        None,
+    )
+    .expect("socket failed");
+    let sockaddr = UnixAddr::new(&sockname).unwrap();
+    bind(sock, &sockaddr).expect("bind failed");
+
+    // Send a message
+    let send_buffer = "hello".as_bytes();
+    if let Err(e) = socket::sendmsg(
+        sock,
+        &[std::io::IoSlice::new(send_buffer)],
+        &[],
+        MsgFlags::empty(),
+        Some(&sockaddr),
+    ) {
+        crate::skip!("Couldn't send ({e:?}), so skipping test");
+    }
+
+    // Receive the message
+    let mut recv_buffer = [0u8; 32];
+    let received = socket::recvmsg(
+        sock,
+        &mut [std::io::IoSliceMut::new(&mut recv_buffer)],
+        None,
+        MsgFlags::empty(),
+    )
+    .unwrap();
+    // Check the address in the received message
+    assert_eq!(sockaddr, received.address.unwrap());
+}
+
 #[test]
 pub fn test_std_conversions() {
     use nix::sys::socket::*;