Skip to content
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.

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

Fix using SockaddrStorage to store Unix domain addresses on Linux #1871

Merged
merged 1 commit into from Nov 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion CHANGELOG.md
Expand Up @@ -6,7 +6,9 @@ This project adheres to [Semantic Versioning](https://semver.org/).
## [Unreleased] - ReleaseDate
### Added

- Add `MntFlags` and `unmount` on all of the BSDs.
- Added `SockaddrStorage::{as_unix_addr, as_unix_addr_mut}`
([#1871](https://github.com/nix-rust/nix/pull/1871))
- Added `MntFlags` and `unmount` on all of the BSDs.
([#1849](https://github.com/nix-rust/nix/pull/1849))
- Added a 'Statfs::flags' method.
([#1849](https://github.com/nix-rust/nix/pull/1849))
Expand Down Expand Up @@ -38,6 +40,8 @@ This project adheres to [Semantic Versioning](https://semver.org/).

### Fixed

- Fixed using `SockaddrStorage` to store a Unix-domain socket address on Linux.
([#1871](https://github.com/nix-rust/nix/pull/1871))
- Fix microsecond calculation for `TimeSpec`.
([#1801](https://github.com/nix-rust/nix/pull/1801))
- Fix `User::from_name` and `Group::from_name` panicking
Expand Down
122 changes: 122 additions & 0 deletions src/sys/socket/addr.rs
Expand Up @@ -1537,6 +1537,17 @@ impl SockaddrLike for SockaddrStorage {
let mut ss: libc::sockaddr_storage = mem::zeroed();
let ssp = &mut ss as *mut libc::sockaddr_storage as *mut u8;
ptr::copy(addr as *const u8, ssp, len as usize);
#[cfg(any(
target_os = "android",
target_os = "fuchsia",
target_os = "illumos",
target_os = "linux"
))]
if i32::from(ss.ss_family) == libc::AF_UNIX {
// Safe because we UnixAddr is strictly smaller than
// SockaddrStorage, and we just initialized the structure.
(*(&mut ss as *mut libc::sockaddr_storage as *mut UnixAddr)).sun_len = len as u8;
}
Comment on lines +1546 to +1550
Copy link
Contributor

Choose a reason for hiding this comment

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

A libc::sockaddr_storage was initialized above, but I don't think that means that those bytes represent a valid UnixAddr? For example libc::sockaddr_storage has 6 padding bytes on Linux x86-64 that may not be zeroed, and reading those uninitialized bytes as a UnixAddr would be UB. I think this would only occur if len is less than 8 (sizeof(u16) + 6). Edit: And only if the UnixAddr were to read bytes past len. But as long as internally UnixAddr makes sure to never read past len - offsetof(sun_path) bytes of sun_path, then I think it would be fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

The docs for mem::zeroed say that padding bytes may not be initialized. But if you look at the source, they are, at least in the current implementation. So I don't think there's a problem to merge this PR. But we should still fix the issue you raised with libc.

Some(Self { ss })
}
} else {
Expand Down Expand Up @@ -1598,6 +1609,21 @@ impl SockaddrLike for SockaddrStorage {
}
}
}

#[cfg(any(
target_os = "android",
target_os = "fuchsia",
target_os = "illumos",
target_os = "linux"
))]
fn len(&self) -> libc::socklen_t {
match self.as_unix_addr() {
// The UnixAddr type knows its own length
Some(ua) => ua.len(),
// For all else, we're just a boring SockaddrStorage
None => mem::size_of_val(self) as libc::socklen_t
}
}
}

macro_rules! accessors {
Expand Down Expand Up @@ -1635,6 +1661,64 @@ macro_rules! accessors {
}

impl SockaddrStorage {
/// Downcast to an immutable `[UnixAddr]` reference.
pub fn as_unix_addr(&self) -> Option<&UnixAddr> {
cfg_if! {
if #[cfg(any(target_os = "android",
target_os = "fuchsia",
target_os = "illumos",
target_os = "linux"
))]
{
let p = unsafe{ &self.ss as *const libc::sockaddr_storage };
// Safe because UnixAddr is strictly smaller than
// sockaddr_storage, and we're fully initialized
let len = unsafe {
(*(p as *const UnixAddr )).sun_len as usize
};
} else {
let len = self.len() as usize;
}
}
// Sanity checks
if self.family() != Some(AddressFamily::Unix) ||
len < offset_of!(libc::sockaddr_un, sun_path) ||
len > mem::size_of::<libc::sockaddr_un>() {
None
} else {
Some(unsafe{&self.su})
}
}

/// Downcast to a mutable `[UnixAddr]` reference.
pub fn as_unix_addr_mut(&mut self) -> Option<&mut UnixAddr> {
cfg_if! {
if #[cfg(any(target_os = "android",
target_os = "fuchsia",
target_os = "illumos",
target_os = "linux"
))]
{
let p = unsafe{ &self.ss as *const libc::sockaddr_storage };
// Safe because UnixAddr is strictly smaller than
// sockaddr_storage, and we're fully initialized
let len = unsafe {
(*(p as *const UnixAddr )).sun_len as usize
};
} else {
let len = self.len() as usize;
}
}
// Sanity checks
if self.family() != Some(AddressFamily::Unix) ||
len < offset_of!(libc::sockaddr_un, sun_path) ||
len > mem::size_of::<libc::sockaddr_un>() {
None
} else {
Some(unsafe{&mut self.su})
}
}

#[cfg(any(target_os = "android", target_os = "linux"))]
accessors! {as_alg_addr, as_alg_addr_mut, AlgAddr,
AddressFamily::Alg, libc::sockaddr_alg, alg}
Expand Down Expand Up @@ -3064,6 +3148,44 @@ mod tests {
}
}

mod sockaddr_storage {
use super::*;

#[test]
fn from_sockaddr_un_named() {
let ua = UnixAddr::new("/var/run/mysock").unwrap();
let ptr = ua.as_ptr() as *const libc::sockaddr;
let ss = unsafe {
SockaddrStorage::from_raw(ptr, Some(ua.len()))
}.unwrap();
assert_eq!(ss.len(), ua.len());
}

#[cfg(any(target_os = "android", target_os = "linux"))]
#[test]
fn from_sockaddr_un_abstract_named() {
let name = String::from("nix\0abstract\0test");
let ua = UnixAddr::new_abstract(name.as_bytes()).unwrap();
let ptr = ua.as_ptr() as *const libc::sockaddr;
let ss = unsafe {
SockaddrStorage::from_raw(ptr, Some(ua.len()))
}.unwrap();
assert_eq!(ss.len(), ua.len());
}

#[cfg(any(target_os = "android", target_os = "linux"))]
#[test]
fn from_sockaddr_un_abstract_unnamed() {
let empty = String::new();
let ua = UnixAddr::new_abstract(empty.as_bytes()).unwrap();
let ptr = ua.as_ptr() as *const libc::sockaddr;
let ss = unsafe {
SockaddrStorage::from_raw(ptr, Some(ua.len()))
}.unwrap();
assert_eq!(ss.len(), ua.len());
}
}

mod unixaddr {
use super::*;

Expand Down