Repository: rust-lang/socket2
Pull request #176 (2020-12-27 14:20:41)

Refactor Sockaddr


Patch:
diff --git a/src/sockaddr.rs b/src/sockaddr.rs
index 239a81c..4af775f 100644
--- a/src/sockaddr.rs
+++ b/src/sockaddr.rs
@@ -1,6 +1,6 @@
 use std::mem::{self, size_of, MaybeUninit};
 use std::net::{SocketAddr, SocketAddrV4, SocketAddrV6};
-use std::{fmt, io, ptr};
+use std::{fmt, io};
 
 use crate::sys::{
     sa_family_t, sockaddr, sockaddr_in, sockaddr_in6, sockaddr_storage, socklen_t, AF_INET,
@@ -19,39 +19,57 @@ pub struct SockAddr {
 }
 
 impl SockAddr {
-    /// Constructs a `SockAddr` from its raw components.
+    /// Initialise a `SockAddr` by calling the function `init`.
     ///
-    /// # Safety
-    ///
-    /// It is up to the user to ensure the `addr` pointer and `len` length are
-    /// correct.
-    pub unsafe fn from_raw_parts(addr: *const sockaddr, len: socklen_t) -> SockAddr {
-        let mut storage = MaybeUninit::<sockaddr_storage>::zeroed();
-        ptr::copy_nonoverlapping(
-            addr as *const _ as *const u8,
-            storage.as_mut_ptr() as *mut u8,
-            len as usize,
-        );
-        SockAddr {
-            // This is safe as we written the address to `storage` above.
-            storage: storage.assume_init(),
-            len,
-        }
-    }
-
-    /// Initilaise a `SockAddr` by calling the function `f`.
+    /// The type of the address storage and length passed to the function `init`
+    /// is OS/architecture specific.
     ///
     /// # Safety
     ///
     /// Caller must initialise the provided address storage and set the length
-    /// properly.
-    pub(crate) unsafe fn init<F, T>(f: F) -> io::Result<(T, SockAddr)>
+    /// properly. The address is zeroed before `init` is called and is thus
+    /// valid to dereference and read from. The length initialised to the
+    /// maximum length of the storage.
+    ///
+    /// # Examples
+    ///
+    #[cfg_attr(unix, doc = "```")]
+    #[cfg_attr(not(unix), doc = "```ignore")]
+    /// use std::io;
+    /// use std::os::unix::io::AsRawFd;
+    ///
+    /// use socket2::{SockAddr, Socket, Domain, Type};
+    ///
+    /// # fn main() -> io::Result<()> {
+    /// let socket = Socket::new(Domain::IPV4, Type::STREAM, None)?;
+    ///
+    /// // Initialise a `SocketAddr` byte calling `getsockname(2)`.
+    /// let (_, address) = unsafe {
+    ///     SockAddr::init(|addr_storage, len| {
+    ///         // The `getsockname(2)` system call will intiliase `storage` for
+    ///         // us, setting `len` to the correct length.
+    ///         if libc::getsockname(socket.as_raw_fd(), addr_storage.cast(), len) == -1 {
+    ///             Err(io::Error::last_os_error())
+    ///         } else {
+    ///             Ok(())
+    ///         }
+    ///     })
+    /// }?;
+    /// # drop(address);
+    /// # Ok(())
+    /// # }
+    /// ```
+    pub unsafe fn init<F, T>(init: F) -> io::Result<(T, SockAddr)>
     where
         F: FnOnce(*mut sockaddr_storage, *mut socklen_t) -> io::Result<T>,
     {
+        const STORAGE_SIZE: socklen_t = size_of::<sockaddr_storage>() as socklen_t;
+        // NOTE: `SockAddr::unix` depends on the storage being zeroed before
+        // calling `init`.
         let mut storage = MaybeUninit::<sockaddr_storage>::zeroed();
-        let mut len = size_of::<sockaddr_storage>() as socklen_t;
-        f(storage.as_mut_ptr(), &mut len).map(|res| {
+        let mut len = STORAGE_SIZE;
+        init(storage.as_mut_ptr(), &mut len).map(|res| {
+            debug_assert!(len <= STORAGE_SIZE, "overflown address storage");
             let addr = SockAddr {
                 // Safety: zeroed-out `sockaddr_storage` is valid, caller must
                 // ensure at least `len` bytes are valid.
@@ -83,9 +101,9 @@ impl SockAddr {
         &self.storage
     }
 
-    /// Returns this address as a `SocketAddr` if it is in the `AF_INET` (IP v4)
-    /// or `AF_INET6` (IP v6) family, otherwise returns `None`.
-    pub fn as_std(&self) -> Option<SocketAddr> {
+    /// Returns this address as a `SocketAddr` if it is in the `AF_INET` (IPv4)
+    /// or `AF_INET6` (IPv6) family, otherwise returns `None`.
+    pub fn as_socket(&self) -> Option<SocketAddr> {
         if self.storage.ss_family == AF_INET as sa_family_t {
             // Safety: if the ss_family field is AF_INET then storage must be a sockaddr_in.
             let addr = unsafe { &*(&self.storage as *const _ as *const sockaddr_in) };
@@ -117,8 +135,8 @@ impl SockAddr {
 
     /// Returns this address as a `SocketAddrV4` if it is in the `AF_INET`
     /// family.
-    pub fn as_inet(&self) -> Option<SocketAddrV4> {
-        match self.as_std() {
+    pub fn as_socket_ipv4(&self) -> Option<SocketAddrV4> {
+        match self.as_socket() {
             Some(SocketAddr::V4(addr)) => Some(addr),
             _ => None,
         }
@@ -126,8 +144,8 @@ impl SockAddr {
 
     /// Returns this address as a `SocketAddrV6` if it is in the `AF_INET6`
     /// family.
-    pub fn as_inet6(&self) -> Option<SocketAddrV6> {
-        match self.as_std() {
+    pub fn as_socket_ipv6(&self) -> Option<SocketAddrV6> {
+        match self.as_socket() {
             Some(SocketAddr::V6(addr)) => Some(addr),
             _ => None,
         }
@@ -240,16 +258,16 @@ fn ipv4() {
     let addr = SockAddr::from(std);
     assert_eq!(addr.family(), AF_INET as sa_family_t);
     assert_eq!(addr.len(), size_of::<sockaddr_in>() as socklen_t);
-    assert_eq!(addr.as_std(), Some(SocketAddr::V4(std)));
-    assert_eq!(addr.as_inet(), Some(std));
-    assert!(addr.as_inet6().is_none());
+    assert_eq!(addr.as_socket(), Some(SocketAddr::V4(std)));
+    assert_eq!(addr.as_socket_ipv4(), Some(std));
+    assert!(addr.as_socket_ipv6().is_none());
 
     let addr = SockAddr::from(SocketAddr::from(std));
     assert_eq!(addr.family(), AF_INET as sa_family_t);
     assert_eq!(addr.len(), size_of::<sockaddr_in>() as socklen_t);
-    assert_eq!(addr.as_std(), Some(SocketAddr::V4(std)));
-    assert_eq!(addr.as_inet(), Some(std));
-    assert!(addr.as_inet6().is_none());
+    assert_eq!(addr.as_socket(), Some(SocketAddr::V4(std)));
+    assert_eq!(addr.as_socket_ipv4(), Some(std));
+    assert!(addr.as_socket_ipv6().is_none());
 }
 
 #[test]
@@ -259,14 +277,14 @@ fn ipv6() {
     let addr = SockAddr::from(std);
     assert_eq!(addr.family(), AF_INET6 as sa_family_t);
     assert_eq!(addr.len(), size_of::<sockaddr_in6>() as socklen_t);
-    assert_eq!(addr.as_std(), Some(SocketAddr::V6(std)));
-    assert!(addr.as_inet().is_none());
-    assert_eq!(addr.as_inet6(), Some(std));
+    assert_eq!(addr.as_socket(), Some(SocketAddr::V6(std)));
+    assert!(addr.as_socket_ipv4().is_none());
+    assert_eq!(addr.as_socket_ipv6(), Some(std));
 
     let addr = SockAddr::from(SocketAddr::from(std));
     assert_eq!(addr.family(), AF_INET6 as sa_family_t);
     assert_eq!(addr.len(), size_of::<sockaddr_in6>() as socklen_t);
-    assert_eq!(addr.as_std(), Some(SocketAddr::V6(std)));
-    assert!(addr.as_inet().is_none());
-    assert_eq!(addr.as_inet6(), Some(std));
+    assert_eq!(addr.as_socket(), Some(SocketAddr::V6(std)));
+    assert!(addr.as_socket_ipv4().is_none());
+    assert_eq!(addr.as_socket_ipv6(), Some(std));
 }
diff --git a/src/sys/unix.rs b/src/sys/unix.rs
index d3d3a67..8274e60 100644
--- a/src/sys/unix.rs
+++ b/src/sys/unix.rs
@@ -360,42 +360,54 @@ impl SockAddr {
     ///
     /// Returns an error if the path is longer than `SUN_LEN`.
     #[cfg(feature = "all")]
+    #[allow(unused_unsafe)] // TODO: replace with `unsafe_op_in_unsafe_fn` once stable.
     pub fn unix<P>(path: P) -> io::Result<SockAddr>
     where
         P: AsRef<Path>,
     {
-        // Safety: zeroed `sockaddr_un` is valid.
-        let mut addr: libc::sockaddr_un = unsafe { mem::zeroed() };
-
-        let bytes = path.as_ref().as_os_str().as_bytes();
-        if bytes.len() >= addr.sun_path.len() {
-            return Err(io::Error::new(
-                io::ErrorKind::InvalidInput,
-                "path must be shorter than SUN_LEN",
-            ));
-        }
-
-        addr.sun_family = libc::AF_UNIX as sa_family_t;
-        // Safety: `bytes` and `addr.sun_path` are not overlapping and `bytes`
-        // points to valid memory.
         unsafe {
-            ptr::copy_nonoverlapping(
-                bytes.as_ptr(),
-                addr.sun_path.as_mut_ptr() as *mut u8,
-                bytes.len(),
-            )
-        };
-        // Zeroed memory above, so the path is already null terminated.
-
-        let base = &addr as *const _ as usize;
-        let path = &addr.sun_path as *const _ as usize;
-        let sun_path_offset = path - base;
-        let mut len = sun_path_offset + bytes.len();
-        match bytes.first() {
-            Some(&0) | None => {}
-            Some(_) => len += 1,
-        };
-        Ok(unsafe { SockAddr::from_raw_parts(&addr as *const _ as *const _, len as socklen_t) })
+            SockAddr::init(|storage, len| {
+                // Safety: `SockAddr::init` zeros the address, which is a valid
+                // representation.
+                let storage: &mut libc::sockaddr_un = unsafe { &mut *storage.cast() };
+                let len: &mut socklen_t = unsafe { &mut *len };
+
+                let bytes = path.as_ref().as_os_str().as_bytes();
+                if bytes.len() >= storage.sun_path.len() {
+                    return Err(io::Error::new(
+                        io::ErrorKind::InvalidInput,
+                        "path must be shorter than SUN_LEN",
+                    ));
+                }
+
+                storage.sun_family = libc::AF_UNIX as sa_family_t;
+                // Safety: `bytes` and `addr.sun_path` are not overlapping and
+                // both point to valid memory.
+                // `SockAddr::init` zeroes the memory, so the path is already
+                // null terminated.
+                unsafe {
+                    ptr::copy_nonoverlapping(
+                        bytes.as_ptr(),
+                        storage.sun_path.as_mut_ptr() as *mut u8,
+                        bytes.len(),
+                    )
+                };
+
+                let base = storage as *const _ as usize;
+                let path = &storage.sun_path as *const _ as usize;
+                let sun_path_offset = path - base;
+                let length = sun_path_offset
+                    + bytes.len()
+                    + match bytes.first() {
+                        Some(&0) | None => 0,
+                        Some(_) => 1,
+                    };
+                *len = length as socklen_t;
+
+                Ok(())
+            })
+        }
+        .map(|(_, addr)| addr)
     }
 }
 
@@ -532,7 +544,7 @@ fn recvmsg(
     let msg_namelen = if msg_name.is_null() {
         0
     } else {
-        size_of::<libc::sockaddr_storage>() as libc::socklen_t
+        size_of::<sockaddr_storage>() as libc::socklen_t
     };
     // libc::msghdr contains unexported padding fields on Fuchsia.
     let mut msg: libc::msghdr = unsafe { mem::zeroed() };


Test patch:
diff --git a/tests/socket.rs b/tests/socket.rs
index 049cbac..7dad160 100644
--- a/tests/socket.rs
+++ b/tests/socket.rs
@@ -110,8 +110,8 @@ fn protocol_fmt_debug() {
 fn socket_address_unix() {
     let string = "/tmp/socket";
     let addr = SockAddr::unix(string).unwrap();
-    assert!(addr.as_inet().is_none());
-    assert!(addr.as_inet6().is_none());
+    assert!(addr.as_socket_ipv4().is_none());
+    assert!(addr.as_socket_ipv6().is_none());
 }
 
 #[test]
@@ -461,7 +461,10 @@ fn send_from_recv_to_vectored() {
     #[cfg(all(unix, not(target_os = "redox")))]
     assert_eq!(flags.is_out_of_band(), false);
     assert_eq!(flags.is_truncated(), false);
-    assert_eq!(addr.as_inet6().unwrap(), addr_a.as_inet6().unwrap());
+    assert_eq!(
+        addr.as_socket_ipv6().unwrap(),
+        addr_a.as_socket_ipv6().unwrap()
+    );
 
     assert_eq!(unsafe { assume_init(&surgeon) }, b"surgeon");
     assert_eq!(unsafe { assume_init(&has) }, b"has");
@@ -508,7 +511,10 @@ fn recv_from_vectored_truncated() {
         .unwrap();
     assert_eq!(received, 24);
     assert_eq!(flags.is_truncated(), true);
-    assert_eq!(addr.as_inet6().unwrap(), addr_a.as_inet6().unwrap());
+    assert_eq!(
+        addr.as_socket_ipv6().unwrap(),
+        addr_a.as_socket_ipv6().unwrap()
+    );
     assert_eq!(unsafe { assume_init(&buffer) }, b"do not feed the gremlins");
 }