Repository: rust-lang/socket2
Pull request #175 (2020-12-27 13:11:52)

Add contributing guide
Updates #168.

/cc @KodrAus


Patch:
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
new file mode 100644
index 0000000..828f425
--- /dev/null
+++ b/CONTRIBUTING.md
@@ -0,0 +1,83 @@
+# Contributing to Socket2
+
+There are many ways to contribute to Socket2, including (but not limited to)
+answering questions, adding new features, fixing bugs or expanding the
+documentation. This document will focus on adding new features and fixing bugs.
+
+If you're adding a new feature please first [open an issue] laying out what
+would be added and a design you're proposing. Doing this before actually writing
+the code will save you time if others suggest improvements to the design. Once
+there is some consensus on 1) the feature is a good addition to Socket2 and 2)
+the proposed design is the right one, open a [pull request] with the changes.
+
+If you're working on fixing a bug please say so on the specific issue so that
+two people don't work on fixing the same bug. For more complex bugs or fixes
+please also share your proposed design on the issue tracker, same as for new
+features.
+
+Once you're working on the code it's useful to understand where the code is (or
+should) be located, the [code structure section] describes how the code of
+Socket2 is organised.
+
+To make sure we don't create the same bug again, or to ensure that new features
+keep working, please add (regression) test for the changes you've made to the
+code. The [testing section] below describes how to run the tests and where to
+add new tests.
+
+[open an issue]: https://github.com/rust-lang/socket2/issues/new
+[pull request]: https://github.com/rust-lang/socket2/compare
+[code structure section]: #code-structure
+[testing section]: #testing
+
+# Code structure
+
+All types and methods that are available on all tier 1 platforms are defined in
+the first level of the source, i.e. `src/*.rs` files. Additional API that is
+platform specific, e.g. `Domain::UNIX`, is defined in `src/sys/*.rs` and only
+for the platforms that support it. For API that is not available on all tier 1
+platforms the `all` feature is used, to indicate to the user that they're using
+API that might is not available on all platforms.
+
+The main `Socket` type is defined in `src/socket.rs` with additional methods
+defined on in the the `src/sys/*.rs` files, as per above. The methods on
+`Socket` are split into multiple `impl` blocks. The first `impl` block contains
+a collection of system calls for creating and using the socket, e.g.
+`socket(2)`, `bind(2)`, `listen(2)`, etc. The other implementation blocks are
+for getting and setting socket options on various levels, e.g. `SOL_SOCKET`,
+where each block contains a single level. The methods in these block are sorted
+based on the option name, e.g. `IP_ADD_MEMBERSHIP` rather than
+`join_multicast_v4`. Finally the last block contains platforms specific methods
+such as `Socket::freebind` which is (at the time of writing) only available on
+Android, Linux and Fuschia, which is defined in the `src/sys/*.rs` files.
+
+Other types are mostly defined in `src/lib.rs`, except for `SockAddr` and
+`SockRef` which have there own file. These types follow the same structure as
+`Socket`, where OS specific methods are defined in `src/sys/*.rs`, e.g.
+`Type::cloexec`.
+
+# Testing
+
+Testing Socket2 is as simple as running `cargo test --all-features`.
+
+However Socket2 supports a good number of OSs and features. If you want to
+test/check all those combinations it's easier to use the [Makefile]. Using `make
+test_all` it will check all supported OS targets and all combinations of
+supported features. Note that this requires [cargo-hack] and various rustup
+targets to be installed. Cargo-hack must be installed manually, the various
+targets can be installed automatically using `make install_targets` (which uses
+[rustup]).
+
+[Makefile]: ./Makefile
+[cargo-hack]: https://crates.io/crates/cargo-hack
+[rustup]: https://rustup.rs
+
+## Adding a test
+
+Tests should be added to `tests/socket.rs`, following (roughly) the same order
+in which the methods are defined on a type. At the bottom of this file it has a
+macro to create a simple get/set socket option test, more complex API however
+needs a manually written test.
+
+Tests that need to use internal API can be defined directly at the bottom of the
+source file. No need for a test module since we intend on keeping the number of
+internal tests low.
diff --git a/Cargo.toml b/Cargo.toml
index e7bab65..8c5c4d2 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -39,5 +39,5 @@ libc = "0.2.81"
 winapi = { version = "0.3.9", features = ["handleapi", "ws2ipdef", "ws2tcpip"] }
 
 [features]
-# Enable all API, even ones not available on all OSes.
+# Enable all API, even ones not available on all OSs.
 all = []
diff --git a/src/lib.rs b/src/lib.rs
index 0b79d7b..8482d5d 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -6,13 +6,6 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-// # Source code structure.
-//
-// All types and methods that are available on tier 1 all platforms are defined
-// in the first level of the source, i.e. `src/*.rs` files. Additional API that
-// is platform specific, e.g. `Domain::UNIX`, is defined in `src/sys/*.rs` and
-// only for the platforms that support it.
-
 //! Utilities for creating and using sockets.
 //!
 //! The goal of this crate is to create and use a socket using advanced
@@ -55,7 +48,7 @@
 //! ## Features
 //!
 //! This crate has a single feature `all`, which enables all functions even ones
-//! that are not available on all OSes.
+//! that are not available on all OSs.
 
 #![doc(html_root_url = "https://docs.rs/socket2/0.3")]
 #![deny(missing_docs, missing_debug_implementations, rust_2018_idioms)]
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/socket.rs b/src/socket.rs
index 2e054b9..a999993 100644
--- a/src/socket.rs
+++ b/src/socket.rs
@@ -714,7 +714,7 @@ impl Socket {
     /// immediately and the closing is done in the background. When the socket
     /// is closed as part of exit(2), it always lingers in the background.
     ///
-    /// Note that the duration only has a precision of seconds on most OSes.
+    /// Note that the duration only has a precision of seconds on most OSs.
     pub fn set_linger(&self, linger: Option<Duration>) -> io::Result<()> {
         let linger = into_linger(linger);
         unsafe { setsockopt(self.inner, sys::SOL_SOCKET, sys::SO_LINGER, linger) }
@@ -1011,7 +1011,7 @@ impl Socket {
 impl Socket {
     /// Join a multicast group using `IPV6_ADD_MEMBERSHIP` option on this socket.
     ///
-    /// Some OSes use `IPV6_JOIN_GROUP` for this option.
+    /// Some OSs use `IPV6_JOIN_GROUP` for this option.
     ///
     /// This function specifies a new multicast group for this socket to join.
     /// The address must be a valid multicast address, and `interface` is the
@@ -1019,7 +1019,7 @@ impl Socket {
     pub fn join_multicast_v6(&self, multiaddr: &Ipv6Addr, interface: u32) -> io::Result<()> {
         let mreq = sys::Ipv6Mreq {
             ipv6mr_multiaddr: sys::to_in6_addr(multiaddr),
-            // NOTE: some OSes use `c_int`, others use `c_uint`.
+            // NOTE: some OSs use `c_int`, others use `c_uint`.
             ipv6mr_interface: interface as _,
         };
         unsafe {
@@ -1034,7 +1034,7 @@ impl Socket {
 
     /// Leave a multicast group using `IPV6_DROP_MEMBERSHIP` option on this socket.
     ///
-    /// Some OSes use `IPV6_LEAVE_GROUP` for this option.
+    /// Some OSs use `IPV6_LEAVE_GROUP` for this option.
     ///
     /// For more information about this option, see [`join_multicast_v6`].
     ///
@@ -1042,7 +1042,7 @@ impl Socket {
     pub fn leave_multicast_v6(&self, multiaddr: &Ipv6Addr, interface: u32) -> io::Result<()> {
         let mreq = sys::Ipv6Mreq {
             ipv6mr_multiaddr: sys::to_in6_addr(multiaddr),
-            // NOTE: some OSes use `c_int`, others use `c_uint`.
+            // NOTE: some OSs use `c_int`, others use `c_uint`.
             ipv6mr_interface: interface as _,
         };
         unsafe {
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");
 }