Skip to content

Don't assume memory layout of std::net::SocketAddr (master) #122

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 9, 2020
Merged

Don't assume memory layout of std::net::SocketAddr (master) #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4c91cf6
Don't assume memory layout of std::net::SocketAddr
faern Nov 8, 2020
86fd2dd
Add tests for SockAddr conversion
faern Nov 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 119 additions & 26 deletions src/sockaddr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ use std::net::{SocketAddr, SocketAddrV4, SocketAddrV6};
use std::{fmt, ptr};

use crate::sys::{
c_int, sa_family_t, sockaddr, sockaddr_in, sockaddr_in6, sockaddr_storage, socklen_t, AF_INET,
sa_family_t, sockaddr, sockaddr_in, sockaddr_in6, sockaddr_storage, socklen_t, AF_INET,
AF_INET6,
};
#[cfg(windows)]
use winapi::shared::ws2ipdef::SOCKADDR_IN6_LH_u;

/// The address of a socket.
///
Expand All @@ -24,7 +26,7 @@ impl SockAddr {
/// 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>::uninit();
let mut storage = MaybeUninit::<sockaddr_storage>::zeroed();
ptr::copy_nonoverlapping(
addr as *const _ as *const u8,
storage.as_mut_ptr() as *mut u8,
Expand All @@ -33,7 +35,7 @@ impl SockAddr {
SockAddr {
// This is safe as we written the address to `storage` above.
storage: storage.assume_init(),
len: len,
len,
}
}

Expand All @@ -55,30 +57,50 @@ impl SockAddr {
/// 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> {
self.as_inet()
.map(|a| a.into())
.or_else(|| self.as_inet6().map(|a| a.into()))
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) };

let ip = crate::sys::from_in_addr(addr.sin_addr);
let port = u16::from_be(addr.sin_port);
Some(SocketAddr::V4(SocketAddrV4::new(ip, port)))
} else if self.storage.ss_family == AF_INET6 as sa_family_t {
// Safety: if the ss_family field is AF_INET6 then storage must be a sockaddr_in6.
let addr = unsafe { &*(&self.storage as *const _ as *const sockaddr_in6) };

let ip = crate::sys::from_in6_addr(addr.sin6_addr);
let port = u16::from_be(addr.sin6_port);
Some(SocketAddr::V6(SocketAddrV6::new(
ip,
port,
addr.sin6_flowinfo,
#[cfg(unix)]
addr.sin6_scope_id,
#[cfg(windows)]
unsafe {
*addr.u.sin6_scope_id()
},
)))
} else {
None
}
}

/// Returns this address as a `SocketAddrV4` if it is in the `AF_INET`
/// family.
pub fn as_inet(&self) -> Option<SocketAddrV4> {
if self.storage.ss_family as c_int == AF_INET {
let storage: *const sockaddr_storage = (&self.storage) as *const _;
Some(unsafe { *(storage as *const sockaddr_in as *const _) })
} else {
None
match self.as_std() {
Some(SocketAddr::V4(addr)) => Some(addr),
_ => None,
}
}

/// Returns this address as a `SocketAddrV6` if it is in the `AF_INET6`
/// family.
pub fn as_inet6(&self) -> Option<SocketAddrV6> {
if self.storage.ss_family as c_int == AF_INET6 {
let storage: *const sockaddr_storage = (&self.storage) as *const _;
Some(unsafe { *(storage as *const sockaddr_in6 as *const _) })
} else {
None
match self.as_std() {
Some(SocketAddr::V6(addr)) => Some(addr),
_ => None,
}
}
}
Expand All @@ -94,22 +116,67 @@ impl From<SocketAddr> for SockAddr {

impl From<SocketAddrV4> for SockAddr {
fn from(addr: SocketAddrV4) -> SockAddr {
unsafe {
SockAddr::from_raw_parts(
&addr as *const _ as *const _,
mem::size_of::<SocketAddrV4>() as socklen_t,
)
let sockaddr_in = sockaddr_in {
sin_family: AF_INET as sa_family_t,
sin_port: addr.port().to_be(),
sin_addr: crate::sys::to_in_addr(&addr.ip()),
sin_zero: [0; 8],
#[cfg(any(
target_os = "dragonfly",
target_os = "freebsd",
target_os = "ios",
target_os = "macos",
target_os = "netbsd",
target_os = "openbsd"
))]
sin_len: 0,
};
let mut storage = MaybeUninit::<sockaddr_storage>::zeroed();
// Safety: A `sockaddr_in` is memory compatible with a `sockaddr_storage`
unsafe { (storage.as_mut_ptr() as *mut sockaddr_in).write(sockaddr_in) };
SockAddr {
storage: unsafe { storage.assume_init() },
len: mem::size_of::<sockaddr_in>() as socklen_t,
}
}
}

impl From<SocketAddrV6> for SockAddr {
fn from(addr: SocketAddrV6) -> SockAddr {
unsafe {
SockAddr::from_raw_parts(
&addr as *const _ as *const _,
mem::size_of::<SocketAddrV6>() as socklen_t,
)
#[cfg(windows)]
let u = unsafe {
let mut u = mem::zeroed::<SOCKADDR_IN6_LH_u>();
*u.sin6_scope_id_mut() = addr.scope_id();
u
};

let sockaddr_in6 = sockaddr_in6 {
sin6_family: AF_INET6 as sa_family_t,
sin6_port: addr.port().to_be(),
sin6_addr: crate::sys::to_in6_addr(addr.ip()),
sin6_flowinfo: addr.flowinfo(),
#[cfg(unix)]
sin6_scope_id: addr.scope_id(),
#[cfg(windows)]
u,
#[cfg(any(
target_os = "dragonfly",
target_os = "freebsd",
target_os = "ios",
target_os = "macos",
target_os = "netbsd",
target_os = "openbsd"
))]
sin6_len: 0,
#[cfg(any(target_os = "solaris", target_os = "illumos"))]
__sin6_src_id: 0,
};
let mut storage = MaybeUninit::<sockaddr_storage>::zeroed();
// Safety: A `sockaddr_in6` is memory compatible with a `sockaddr_storage`
unsafe { (storage.as_mut_ptr() as *mut sockaddr_in6).write(sockaddr_in6) };
SockAddr {
storage: unsafe { storage.assume_init() },
len: mem::size_of::<sockaddr_in6>() as socklen_t,
}
}
}
Expand All @@ -134,3 +201,29 @@ impl fmt::Debug for SockAddr {
.finish()
}
}

#[cfg(test)]
mod tests {
use super::*;
use std::net::{Ipv4Addr, Ipv6Addr};

#[test]
fn conversion_v4() {
let addr = SocketAddrV4::new(Ipv4Addr::new(1, 2, 3, 4), 9876);
let sockaddr = SockAddr::from(addr);
assert_eq!(sockaddr.family(), AF_INET as sa_family_t);
assert!(sockaddr.as_inet6().is_none());
assert_eq!(sockaddr.as_inet(), Some(addr));
assert_eq!(sockaddr.as_std(), Some(SocketAddr::V4(addr)));
}

#[test]
fn conversion_v6() {
let addr = SocketAddrV6::new(Ipv6Addr::new(1, 2, 3, 4, 5, 6, 7, 8), 9876, 11, 12);
let sockaddr = SockAddr::from(addr);
assert_eq!(sockaddr.family(), AF_INET6 as sa_family_t);
assert!(sockaddr.as_inet().is_none());
assert_eq!(sockaddr.as_inet6(), Some(addr));
assert_eq!(sockaddr.as_std(), Some(SocketAddr::V6(addr)));
}
}
44 changes: 23 additions & 21 deletions src/sys/unix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use std::sync::atomic::{AtomicBool, Ordering};
use std::time::Duration;
use std::{cmp, fmt, io, mem};

use libc::{self, c_void, ssize_t};
use libc::{self, c_void, in6_addr, in_addr, ssize_t};

#[cfg(not(target_os = "redox"))]
use crate::RecvFlags;
Expand Down Expand Up @@ -787,13 +787,12 @@ impl Socket {
unsafe {
let imr_interface: libc::in_addr =
self.getsockopt(libc::IPPROTO_IP, libc::IP_MULTICAST_IF)?;
Ok(from_s_addr(imr_interface.s_addr))
Ok(from_in_addr(imr_interface))
}
}

pub fn set_multicast_if_v4(&self, interface: &Ipv4Addr) -> io::Result<()> {
let interface = to_s_addr(interface);
let imr_interface = libc::in_addr { s_addr: interface };
let imr_interface = to_in_addr(interface);

unsafe { self.setsockopt(libc::IPPROTO_IP, libc::IP_MULTICAST_IF, imr_interface) }
}
Expand Down Expand Up @@ -833,11 +832,9 @@ impl Socket {
}

pub fn join_multicast_v4(&self, multiaddr: &Ipv4Addr, interface: &Ipv4Addr) -> io::Result<()> {
let multiaddr = to_s_addr(multiaddr);
let interface = to_s_addr(interface);
let mreq = libc::ip_mreq {
imr_multiaddr: libc::in_addr { s_addr: multiaddr },
imr_interface: libc::in_addr { s_addr: interface },
imr_multiaddr: to_in_addr(multiaddr),
imr_interface: to_in_addr(interface),
};
unsafe { self.setsockopt(libc::IPPROTO_IP, libc::IP_ADD_MEMBERSHIP, mreq) }
}
Expand All @@ -852,11 +849,9 @@ impl Socket {
}

pub fn leave_multicast_v4(&self, multiaddr: &Ipv4Addr, interface: &Ipv4Addr) -> io::Result<()> {
let multiaddr = to_s_addr(multiaddr);
let interface = to_s_addr(interface);
let mreq = libc::ip_mreq {
imr_multiaddr: libc::in_addr { s_addr: multiaddr },
imr_interface: libc::in_addr { s_addr: interface },
imr_multiaddr: to_in_addr(multiaddr),
imr_interface: to_in_addr(interface),
};
unsafe { self.setsockopt(libc::IPPROTO_IP, libc::IP_DROP_MEMBERSHIP, mreq) }
}
Expand Down Expand Up @@ -1248,21 +1243,28 @@ fn timeval2dur(raw: libc::timeval) -> Option<Duration> {
}
}

fn to_s_addr(addr: &Ipv4Addr) -> libc::in_addr_t {
let octets = addr.octets();
u32::from_ne_bytes(octets)
pub(crate) fn to_in_addr(addr: &Ipv4Addr) -> in_addr {
// `s_addr` is stored as BE on all machines, and the array is in BE order.
// So the native endian conversion method is used so that it's never swapped.
in_addr {
s_addr: u32::from_ne_bytes(addr.octets()),
}
}

fn from_s_addr(in_addr: libc::in_addr_t) -> Ipv4Addr {
in_addr.to_be().into()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.to_be() here was not correct really... Well it yielded the same result, but it was not optimally from an understanding point of view IMO. in_addr is stored in BE. So u32::from_be would be more correct. But both of them just swap the bytes on a LE machine and does nothing on a BE machine, so it does not matter in practice.

Anyway. My solution uses to_ne_bytes to avoid a double swap (since the From<u32> impl for Ipv4Addr swaps the bytes back again).

pub(crate) fn from_in_addr(in_addr: in_addr) -> Ipv4Addr {
Ipv4Addr::from(in_addr.s_addr.to_ne_bytes())
Copy link
Contributor Author

@faern faern Nov 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These helper methods were really handy. Removed a lot of code from the sockaddr.rs file and got rid of a number of #[cfg(...)]s. So I made them pub(crate) so I could use them in the other module.

I also changed the abstraction one level. So instead of converting to/from the only field in the in_addr/in6_addr structs I made these helpers convert directly into that struct. This in turn helped remove some repeated code here in the two sys files as well.

}

fn to_in6_addr(addr: &Ipv6Addr) -> libc::in6_addr {
pub(crate) fn to_in6_addr(addr: &Ipv6Addr) -> libc::in6_addr {
let mut ret: libc::in6_addr = unsafe { mem::zeroed() };
ret.s6_addr = addr.octets();
return ret;
}

pub(crate) fn from_in6_addr(in6_addr: in6_addr) -> Ipv6Addr {
Ipv6Addr::from(in6_addr.s6_addr)
}

#[cfg(target_os = "android")]
fn to_ipv6mr_interface(value: u32) -> c_int {
value as c_int
Expand Down Expand Up @@ -1297,12 +1299,12 @@ fn dur2linger(dur: Option<Duration>) -> libc::linger {
#[test]
fn test_ip() {
let ip = Ipv4Addr::new(127, 0, 0, 1);
assert_eq!(ip, from_s_addr(to_s_addr(&ip)));
assert_eq!(ip, from_in_addr(to_in_addr(&ip)));

let ip = Ipv4Addr::new(127, 34, 4, 12);
let want = 127 << 0 | 34 << 8 | 4 << 16 | 12 << 24;
assert_eq!(to_s_addr(&ip), want);
assert_eq!(from_s_addr(want), ip);
assert_eq!(to_in_addr(&ip).s_addr, want);
assert_eq!(from_in_addr(in_addr { s_addr: want }), ip);
}

#[test]
Expand Down
Loading