Clarify safety invariants in SocketAddrArg. (#1325)

In making another safety review of the `SocketAddrArg` code, I found it
too subtle to have `MMsgHdr::new_with_addr` depending on knowing that
`with_msghdr` happens to avoid using a temporary in the case `SocketAddrAny`.
Change it to avoid doing this, add more comments, and change `with_msghdr`'s
argument to a reference, to prevent it from being accidentally used in
this way.

And change `with_noaddr_msghdr` to `noaddr_msghdr` and make it just return a
`msghdr`, because it never needs to construct any temporaries, so it
avoids this issue.

Also, make `SocketAddrArg::with_sockaddr` and `with_recv_msghdr` unsafe, as
the provided function will need to contain an `unsafe` block which will need
to be able to trust it's being called correctly.

And add safety comments for the callers and impls of these functions and
traits.

Author: sunfishcode

src/backend/libc/conv.rs

@@ -180,82 +180,3 @@ pub(super) fn ret_send_recv(len: isize) -> io::Result<usize> {
 pub(super) fn ret_send_recv(len: i32) -> io::Result<usize> {
     ret_usize(len as isize)
 }
-
-/// Convert the value to the `msg_iovlen` field of a `msghdr` struct.
-#[cfg(all(
-    not(any(windows, target_os = "espidf", target_os = "wasi")),
-    any(
-        target_os = "android",
-        all(
-            target_os = "linux",
-            not(target_env = "musl"),
-            not(all(target_env = "uclibc", any(target_arch = "arm", target_arch = "mips")))
-        )
-    )
-))]
-#[inline]
-pub(super) fn msg_iov_len(len: usize) -> c::size_t {
-    len
-}
-
-/// Convert the value to the `msg_iovlen` field of a `msghdr` struct.
-#[cfg(all(
-    not(any(
-        windows,
-        target_os = "espidf",
-        target_os = "redox",
-        target_os = "vita",
-        target_os = "wasi"
-    )),
-    not(any(
-        target_os = "android",
-        all(
-            target_os = "linux",
-            not(target_env = "musl"),
-            not(all(target_env = "uclibc", any(target_arch = "arm", target_arch = "mips")))
-        )
-    ))
-))]
-#[inline]
-pub(crate) fn msg_iov_len(len: usize) -> c::c_int {
-    len.try_into().unwrap_or(c::c_int::MAX)
-}
-
-/// Convert the value to a `socklen_t`.
-#[cfg(any(
-    bsd,
-    solarish,
-    target_env = "musl",
-    target_os = "aix",
-    target_os = "emscripten",
-    target_os = "fuchsia",
-    target_os = "haiku",
-    target_os = "hurd",
-    target_os = "nto",
-))]
-#[inline]
-pub(crate) fn msg_control_len(len: usize) -> c::socklen_t {
-    len.try_into().unwrap_or(c::socklen_t::MAX)
-}
-
-/// Convert the value to a `size_t`.
-#[cfg(not(any(
-    bsd,
-    solarish,
-    windows,
-    target_env = "musl",
-    target_os = "aix",
-    target_os = "emscripten",
-    target_os = "espidf",
-    target_os = "fuchsia",
-    target_os = "haiku",
-    target_os = "hurd",
-    target_os = "nto",
-    target_os = "redox",
-    target_os = "vita",
-    target_os = "wasi",
-)))]
-#[inline]
-pub(crate) fn msg_control_len(len: usize) -> c::size_t {
-    len
-}

src/backend/libc/net/addr.rs

@@ -253,6 +253,7 @@ impl SocketAddrStorage {
 
     /// Return the `sa_family` of this socket address.
     pub fn family(&self) -> AddressFamily {
+        // SAFETY: `self.0` is a `sockaddr_storage` so it has enough space.
         unsafe {
             AddressFamily::from_raw(crate::backend::net::read_sockaddr::read_sa_family(
                 crate::utils::as_ptr(&self.0).cast::<c::sockaddr>(),
@@ -263,6 +264,7 @@ impl SocketAddrStorage {
     /// Clear the `sa_family` of this socket address to
     /// `AddressFamily::UNSPEC`.
     pub fn clear_family(&mut self) {
+        // SAFETY: `self.0` is a `sockaddr_storage` so it has enough space.
         unsafe {
             crate::backend::net::read_sockaddr::initialize_family_to_unspec(
                 crate::utils::as_mut_ptr(&mut self.0).cast::<c::sockaddr>(),

src/backend/libc/net/msghdr.rs

@@ -4,41 +4,124 @@
 //! the message headers may reference stack-local data.
 
 use crate::backend::c;
-use crate::backend::conv::{msg_control_len, msg_iov_len};
 
 use crate::io::{self, IoSlice, IoSliceMut};
 use crate::net::SocketAddrBuf;
 use crate::net::{addr::SocketAddrArg, RecvAncillaryBuffer, SendAncillaryBuffer};
 
 use core::mem::zeroed;
 
+/// Convert the value to the `msg_iovlen` field of a `msghdr` struct.
+#[cfg(all(
+    not(any(windows, target_os = "espidf", target_os = "wasi")),
+    any(
+        target_os = "android",
+        all(
+            target_os = "linux",
+            not(target_env = "musl"),
+            not(all(target_env = "uclibc", any(target_arch = "arm", target_arch = "mips")))
+        )
+    )
+))]
+#[inline]
+fn msg_iov_len(len: usize) -> c::size_t {
+    len
+}
+
+/// Convert the value to the `msg_iovlen` field of a `msghdr` struct.
+#[cfg(all(
+    not(any(
+        windows,
+        target_os = "espidf",
+        target_os = "redox",
+        target_os = "vita",
+        target_os = "wasi"
+    )),
+    not(any(
+        target_os = "android",
+        all(
+            target_os = "linux",
+            not(target_env = "musl"),
+            not(all(target_env = "uclibc", any(target_arch = "arm", target_arch = "mips")))
+        )
+    ))
+))]
+#[inline]
+fn msg_iov_len(len: usize) -> c::c_int {
+    len.try_into().unwrap_or(c::c_int::MAX)
+}
+
+/// Convert the value to a `socklen_t`.
+#[cfg(any(
+    bsd,
+    solarish,
+    target_env = "musl",
+    target_os = "aix",
+    target_os = "emscripten",
+    target_os = "fuchsia",
+    target_os = "haiku",
+    target_os = "hurd",
+    target_os = "nto",
+))]
+#[inline]
+fn msg_control_len(len: usize) -> c::socklen_t {
+    len.try_into().unwrap_or(c::socklen_t::MAX)
+}
+
+/// Convert the value to a `size_t`.
+#[cfg(not(any(
+    bsd,
+    solarish,
+    windows,
+    target_env = "musl",
+    target_os = "aix",
+    target_os = "emscripten",
+    target_os = "espidf",
+    target_os = "fuchsia",
+    target_os = "haiku",
+    target_os = "hurd",
+    target_os = "nto",
+    target_os = "redox",
+    target_os = "vita",
+    target_os = "wasi",
+)))]
+#[inline]
+fn msg_control_len(len: usize) -> c::size_t {
+    len
+}
+
 /// Create a message header intended to receive a datagram.
-pub(crate) fn with_recv_msghdr<R>(
+///
+/// # Safety
+///
+/// If `f` dereferences the pointers in the `msghdr`, it must do so only within
+/// the bounds indicated by the associated lengths in the `msghdr`.
+///
+/// And, if `f` returns `Ok`, it must have updated the `msg_controllen` field
+/// of the `msghdr` to indicate how many bytes it initialized.
+pub(crate) unsafe fn with_recv_msghdr<R>(
     name: &mut SocketAddrBuf,
     iov: &mut [IoSliceMut<'_>],
     control: &mut RecvAncillaryBuffer<'_>,
     f: impl FnOnce(&mut c::msghdr) -> io::Result<R>,
 ) -> io::Result<R> {
     control.clear();
 
-    let mut msghdr = {
-        let mut h = zero_msghdr();
-        h.msg_name = name.storage.as_mut_ptr().cast();
-        h.msg_namelen = name.len;
-        h.msg_iov = iov.as_mut_ptr().cast();
-        h.msg_iovlen = msg_iov_len(iov.len());
-        h.msg_control = control.as_control_ptr().cast();
-        h.msg_controllen = msg_control_len(control.control_len());
-        h
-    };
+    let mut msghdr = zero_msghdr();
+    msghdr.msg_name = name.storage.as_mut_ptr().cast();
+    msghdr.msg_namelen = name.len;
+    msghdr.msg_iov = iov.as_mut_ptr().cast();
+    msghdr.msg_iovlen = msg_iov_len(iov.len());
+    msghdr.msg_control = control.as_control_ptr().cast();
+    msghdr.msg_controllen = msg_control_len(control.control_len());
 
     let res = f(&mut msghdr);
 
     // Reset the control length.
     if res.is_ok() {
-        unsafe {
-            control.set_control_len(msghdr.msg_controllen.try_into().unwrap_or(usize::MAX));
-        }
+        // SAFETY: `f` returned `Ok`, so our safety condition requires `f` to
+        // have initialized `msg_controllen` bytes.
+        control.set_control_len(msghdr.msg_controllen as usize);
     }
 
     name.len = msghdr.msg_namelen;
@@ -47,39 +130,49 @@ pub(crate) fn with_recv_msghdr<R>(
 }
 
 /// Create a message header intended to send without an address.
-pub(crate) fn with_noaddr_msghdr<R>(
+///
+/// The returned `msghdr` will contain raw pointers to the memory
+/// referenced by `iov` and `control`.
+pub(crate) fn noaddr_msghdr(
     iov: &[IoSlice<'_>],
     control: &mut SendAncillaryBuffer<'_, '_, '_>,
-    f: impl FnOnce(c::msghdr) -> R,
-) -> R {
-    f({
-        let mut h = zero_msghdr();
-        h.msg_iov = iov.as_ptr() as _;
-        h.msg_iovlen = msg_iov_len(iov.len());
-        h.msg_control = control.as_control_ptr().cast();
-        h.msg_controllen = msg_control_len(control.control_len());
-        h
-    })
+) -> c::msghdr {
+    let mut h = zero_msghdr();
+    h.msg_iov = iov.as_ptr() as _;
+    h.msg_iovlen = msg_iov_len(iov.len());
+    h.msg_control = control.as_control_ptr().cast();
+    h.msg_controllen = msg_control_len(control.control_len());
+    h
 }
 
 /// Create a message header intended to send with the specified address.
-pub(crate) fn with_msghdr<R>(
+///
+/// This creates a `c::msghdr` and calls a function `f` on it. The `msghdr`'s
+/// raw pointers may point to temporaries, so this function should avoid
+/// storing the pointers anywhere that would outlive the function call.
+///
+/// # Safety
+///
+/// If `f` dereferences the pointers in the `msghdr`, it must do so only within
+/// the bounds indicated by the associated lengths in the `msghdr`.
+pub(crate) unsafe fn with_msghdr<R>(
     addr: &impl SocketAddrArg,
     iov: &[IoSlice<'_>],
     control: &mut SendAncillaryBuffer<'_, '_, '_>,
-    f: impl FnOnce(c::msghdr) -> R,
+    f: impl FnOnce(&c::msghdr) -> R,
 ) -> R {
     addr.with_sockaddr(|addr_ptr, addr_len| {
-        f({
-            let mut h = zero_msghdr();
-            h.msg_name = addr_ptr as *mut _;
-            h.msg_namelen = addr_len as _;
-            h.msg_iov = iov.as_ptr() as _;
-            h.msg_iovlen = msg_iov_len(iov.len());
-            h.msg_control = control.as_control_ptr().cast();
-            h.msg_controllen = msg_control_len(control.control_len());
-            h
-        })
+        let mut h = zero_msghdr();
+        h.msg_name = addr_ptr as *mut _;
+        h.msg_namelen = addr_len as _;
+        h.msg_iov = iov.as_ptr() as _;
+        h.msg_iovlen = msg_iov_len(iov.len());
+        h.msg_control = control.as_control_ptr().cast();
+        h.msg_controllen = msg_control_len(control.control_len());
+        // Pass a reference to the `c::msghdr` instead of passing it by value
+        // because it may contain pointers to temporary objects that won't
+        // live beyond the call to `with_sockaddr`.
+        f(&h)
     })
 }
 

src/backend/libc/net/read_sockaddr.rs

@@ -112,7 +112,7 @@ unsafe fn read_sun_path0(storage: *const c::sockaddr) -> u8 {
 ///
 /// # Safety
 ///
-/// `storage` must point to a valid socket address returned from the OS.
+/// `storage` must point to a least an initialized `sockaddr_header`.
 #[inline]
 pub(crate) unsafe fn sockaddr_nonempty(storage: *const c::sockaddr, len: SocketAddrLen) -> bool {
     if len == 0 {
@@ -137,6 +137,10 @@ pub(crate) unsafe fn sockaddr_nonempty(storage: *const c::sockaddr, len: SocketA
 
 /// Set the `sa_family` field of a socket address to `AF_UNSPEC`, so that we
 /// can test for `AF_UNSPEC` to test whether it was stored to.
+///
+/// # Safety
+///
+/// `storage` must point to a least an initialized `sockaddr_header`.
 pub(crate) unsafe fn initialize_family_to_unspec(storage: *mut c::sockaddr) {
     (*storage.cast::<sockaddr_header>()).sa_family = c::AF_UNSPEC as _;
 }