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.

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

Tweak SocketAncillary API #86432

Closed
wants to merge 5 commits into from
Closed

Tweak SocketAncillary API #86432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
15b6441
Support using SocketAncillary with raw syscalls with unsafe functions.
de-vri-es Jun 18, 2021
28e9eca
Allow ancillary socket data to be read into an uninitialized buffer.
de-vri-es Jun 18, 2021
327cb39
Make SocketAncillary::add_* return a Result to indicate failure.
de-vri-es Jun 18, 2021
bf85a1a
Add note on zero-initialization and CMSG_NXTHDR.
de-vri-es Jun 18, 2021
99a938f
Mark `SocketAncillary::add_fds` unsafe.
de-vri-es Jun 18, 2021
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
167 changes: 134 additions & 33 deletions library/std/src/os/unix/net/ancillary.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
use super::{sockaddr_un, SocketAddr};
use crate::convert::TryFrom;
use crate::fmt;
use crate::io::{self, IoSlice, IoSliceMut};
use crate::marker::PhantomData;
use crate::mem::{size_of, zeroed};
use crate::mem::{size_of, zeroed, MaybeUninit};
use crate::os::unix::io::RawFd;
use crate::path::Path;
use crate::ptr::{eq, read_unaligned};
use crate::slice::from_raw_parts;
use crate::slice;
use crate::sys::net::Socket;

// FIXME(#43348): Make libc adapt #[doc(cfg(...))] so we don't need these fake definitions here?
Expand Down Expand Up @@ -79,36 +80,30 @@ pub(super) fn send_vectored_with_ancillary_to(
}

fn add_to_ancillary_data<T>(
buffer: &mut [u8],
buffer: &mut [MaybeUninit<u8>],
length: &mut usize,
source: &[T],
cmsg_level: libc::c_int,
cmsg_type: libc::c_int,
) -> bool {
let source_len = if let Some(source_len) = source.len().checked_mul(size_of::<T>()) {
if let Ok(source_len) = u32::try_from(source_len) {
source_len
} else {
return false;
}
} else {
return false;
};
) -> Result<(), AddAncillaryError> {
let source_len =
source.len().checked_mul(size_of::<T>()).ok_or_else(|| AddAncillaryError::new())?;
let source_len = u32::try_from(source_len).map_err(|_| AddAncillaryError::new())?;

unsafe {
let additional_space = libc::CMSG_SPACE(source_len) as usize;

let new_length = if let Some(new_length) = additional_space.checked_add(*length) {
new_length
} else {
return false;
return Err(AddAncillaryError::new());
};

if new_length > buffer.len() {
return false;
return Err(AddAncillaryError::new());
}

buffer[*length..new_length].fill(0);
buffer[*length..new_length].fill(MaybeUninit::new(0));

*length = new_length;

Expand All @@ -131,7 +126,7 @@ fn add_to_ancillary_data<T>(
}

if previous_cmsg.is_null() {
return false;
return Err(AddAncillaryError::new());
}

(*previous_cmsg).cmsg_level = cmsg_level;
Expand All @@ -142,7 +137,7 @@ fn add_to_ancillary_data<T>(

libc::memcpy(data, source.as_ptr().cast(), source_len as usize);
}
true
Ok(())
}

struct AncillaryDataIter<'a, T> {
Expand Down Expand Up @@ -309,7 +304,7 @@ impl<'a> AncillaryData<'a> {
let cmsg_len_zero = libc::CMSG_LEN(0) as usize;
let data_len = (*cmsg).cmsg_len as usize - cmsg_len_zero;
let data = libc::CMSG_DATA(cmsg).cast();
let data = from_raw_parts(data, data_len);
let data = slice::from_raw_parts(data, data_len);

match (*cmsg).cmsg_level {
libc::SOL_SOCKET => match (*cmsg).cmsg_type {
Expand Down Expand Up @@ -401,7 +396,7 @@ impl<'a> Iterator for Messages<'a> {
#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
#[derive(Debug)]
pub struct SocketAncillary<'a> {
buffer: &'a mut [u8],
buffer: &'a mut [MaybeUninit<u8>],
length: usize,
truncated: bool,
}
Expand All @@ -420,6 +415,23 @@ impl<'a> SocketAncillary<'a> {
/// ```
#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
pub fn new(buffer: &'a mut [u8]) -> Self {
let buffer = unsafe { slice::from_raw_parts_mut(buffer.as_mut_ptr().cast(), buffer.len()) };
Self::new_uninit(buffer)
}

/// Create an ancillary data with an uninitialized buffer.
///
/// # Example
///
/// ```no_run
/// # #![allow(unused_mut)]
/// #![feature(unix_socket_ancillary_data, new_uninit)]
/// use std::os::unix::net::SocketAncillary;
/// let mut ancillary_buffer = Box::new_uninit_slice(128);
/// let mut ancillary = SocketAncillary::new_uninit(&mut ancillary_buffer[..]);
/// ```
#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
pub fn new_uninit(buffer: &'a mut [MaybeUninit<u8>]) -> Self {
SocketAncillary { buffer, length: 0, truncated: false }
}

Expand All @@ -429,6 +441,37 @@ impl<'a> SocketAncillary<'a> {
self.buffer.len()
}

/// Returns the raw ancillary data as byte slice.
#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
pub fn data(&self) -> &[u8] {
unsafe { MaybeUninit::slice_assume_init_ref(&self.buffer[..self.length]) }
}

/// Returns the entire buffer, including unused capacity.
///
/// Use [`data()`](Self::data) if you are only interested in the used portion of the buffer.
#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
pub fn buffer(&self) -> &[MaybeUninit<u8>] {
Copy link
Contributor Author

@de-vri-es de-vri-es Jun 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Note that we don't really need buffer() for integrating with manual syscalls. data(), buffer_mut() and set_len() are enough. However, it seemed weird to have buffer_mut() and not buffer().

We could also rename buffer_mut() to just buffer(). I don't have any strong feelings either way.

Copy link
Member

Choose a reason for hiding this comment

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

We could also rename buffer_mut() to just buffer().

Sounds good to me.

self.buffer
}

/// Returns the entire buffer as mutable slice, including unused capacity.
///
/// You should normally call [`set_len()`](Self::set_len)
/// after changing the contents of the buffer.
///
/// # Safety
/// All data written to the buffer must be valid ancillary data for the target platform,
/// and you must call [`set_len()`](Self::set_len) after changing
/// the buffer contents to update the internal bookkeeping.
///
/// Make sure to zero-initialize the buffer if you are going to use it with [`libc::CMSG_NXTHDR()`].
/// That function requires the buffer to be zero-initialized in order to work correctly
#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
pub unsafe fn buffer_mut(&mut self) -> &mut [MaybeUninit<u8>] {
self.buffer
}
Copy link
Member

Choose a reason for hiding this comment

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

Is the use-case appending additional messages to the buffer? In that case it might be more ergonomic to only return the tail beyond the current length.

Copy link
Contributor Author

@de-vri-es de-vri-es Jun 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The use-case I have in mind is to pass the entire buffer to a syscall to let the kernel write into it. I wasn't really thinking of manually extending it with hand-crafted messages.

If we would want to support that, I would lean towards making a public version of add_to_ancillary_data (also unsafe of course).

Copy link
Member

Choose a reason for hiding this comment

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

I see, I thought this was about handling message types not supported by std rather than handling ancilliary data in conjunction with other syscalls. Are there syscalls other than recvmsg that would write ancillary data? io_uring I guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly, although I'm not familiar with io_uring myself. It also helps when implementing unsupported socket types on crates.io. I'd love to use this in tokio-seqpacket for example.

Copy link
Member

Choose a reason for hiding this comment

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

I'd love to use this in tokio-seqpacket for example.

At least for that particular usecase you probably wouldn't need it. You could temporarily create a ManuallyDrop<UnixDatagram> socket and then call send_with_ancillary since that boils down to the same syscall. At least once a way to pass flags is provided. That way a lot of code duplication can be avoided.

Copy link
Contributor Author

@de-vri-es de-vri-es Jun 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think that would be a good solution. That would mean using knowledge about implementation details of std. The UnixDatagram API does not guarantee any mapping to specific syscalls or that from_raw_fd is pretty much a simple cast.


/// Returns `true` if the ancillary data is empty.
#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
pub fn is_empty(&self) -> bool {
Expand All @@ -441,10 +484,24 @@ impl<'a> SocketAncillary<'a> {
self.length
}

/// Set the number of valid ancillary data bytes and the truncated flag.
///
/// This can be used with [`buffer_mut()`](Self::buffer_mut)
/// to manually write ancillary data into the buffer.
///
/// # Safety
/// - The length may not exceed [`capacity()`](Self::capacity).
/// - The data in the buffer at `0..length` must be valid ancillary data.
#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
pub unsafe fn set_len(&mut self, length: usize, truncated: bool) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not think of situations where you want to change length and the truncated flag independently of each-other, so I figured it makes sense to ask for both at the same time. That way you can't accidentally forget to update the truncated flag.

self.length = length;
self.truncated = truncated;
}

/// Returns the iterator of the control messages.
#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
pub fn messages(&self) -> Messages<'_> {
Messages { buffer: &self.buffer[..self.length], current: None }
Messages { buffer: self.data(), current: None }
}

/// Is `true` if during a recv operation the ancillary was truncated.
Expand Down Expand Up @@ -477,10 +534,16 @@ impl<'a> SocketAncillary<'a> {

/// Add file descriptors to the ancillary data.
///
/// The function returns `true` if there was enough space in the buffer.
/// If there was not enough space then no file descriptors was appended.
/// Technically, that means this operation adds a control message with the level `SOL_SOCKET`
/// and type `SCM_RIGHTS`.
/// This operation adds a control message with the level `SOL_SOCKET` and type `SCM_RIGHTS`.
/// If there is not enough space in the buffer for all file descriptors,
/// an error is returned and no file descriptors are added.
///
/// # Safety
/// This function copies raw file descriptors into the ancillary data buffer without a lifetime.
/// You must guarantee that the file descriptor remains open until the ancillary data is no longer used.
///
/// Additionally, you must ensure that duplicating the file descriptor and sending it to a remote process
/// does not violate the safety requirements of the object managing the file descriptor.
///
/// # Example
///
Expand All @@ -495,7 +558,9 @@ impl<'a> SocketAncillary<'a> {
///
/// let mut ancillary_buffer = [0; 128];
/// let mut ancillary = SocketAncillary::new(&mut ancillary_buffer[..]);
/// ancillary.add_fds(&[sock.as_raw_fd()][..]);
/// unsafe {
/// ancillary.add_fds(&[sock.as_raw_fd()][..])?;
/// }
///
/// let mut buf = [1; 8];
/// let mut bufs = &mut [IoSlice::new(&mut buf[..])][..];
Expand All @@ -504,7 +569,7 @@ impl<'a> SocketAncillary<'a> {
/// }
/// ```
#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
pub fn add_fds(&mut self, fds: &[RawFd]) -> bool {
pub unsafe fn add_fds(&mut self, fds: &[RawFd]) -> Result<(), AddAncillaryError> {
self.truncated = false;
add_to_ancillary_data(
&mut self.buffer,
Expand All @@ -517,14 +582,13 @@ impl<'a> SocketAncillary<'a> {

/// Add credentials to the ancillary data.
///
/// The function returns `true` if there was enough space in the buffer.
/// If there was not enough space then no credentials was appended.
/// Technically, that means this operation adds a control message with the level `SOL_SOCKET`
/// and type `SCM_CREDENTIALS` or `SCM_CREDS`.
///
/// This function adds a control message with the level `SOL_SOCKET`
/// and type `SCM_CREDENTIALS` or `SCM_CREDS` (depending on the platform).
/// If there is not enough space in the buffer for all credentials,
/// an error is returned and no credentials are added.
#[cfg(any(doc, target_os = "android", target_os = "linux",))]
#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
pub fn add_creds(&mut self, creds: &[SocketCred]) -> bool {
pub fn add_creds(&mut self, creds: &[SocketCred]) -> Result<(), AddAncillaryError> {
self.truncated = false;
add_to_ancillary_data(
&mut self.buffer,
Expand Down Expand Up @@ -583,3 +647,40 @@ impl<'a> SocketAncillary<'a> {
self.truncated = false;
}
}

/// An error returned when trying to add anciallary data that exceeds the buffer capacity.
#[cfg(any(doc, target_os = "android", target_os = "linux",))]
#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
pub struct AddAncillaryError {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we really create an Error for on special failure? Maybe, create an enum AncillaryError for all errors, which can occur with Ancillary, or use io:Error:other.

Copy link
Contributor Author

@de-vri-es de-vri-es Jun 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We could add an inner enum to change the error message, but I would be hestitent to expose the enum directly to the user. That would limit what we can do with it in the future, because we would have to stay backwards compatible.

I wouldn't directly return an io::Error, since it's a bit overkill to have a Box<dyn Error>. I kinda like a opaque cheap error type that can be converted into io::Error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, io::Error would be okay, or we can use #[non_exhaustive] for the enum.

_priv: (),
}

impl AddAncillaryError {
fn new() -> Self {
Self { _priv: () }
}
}

#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
impl fmt::Debug for AddAncillaryError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("AddAncillaryError").finish()
}
}

#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
impl fmt::Display for AddAncillaryError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "could not add data to anciallary buffer")
}
}

#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
impl crate::error::Error for AddAncillaryError {}

#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
impl From<AddAncillaryError> for io::Error {
Copy link
Contributor Author

@de-vri-es de-vri-es Jun 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I added this conversion because it is likely that add_fds() and add_creds() are going to be used from functions that already return an io::Result. So this allows you to just use the ? operator.

Copy link
Member

Choose a reason for hiding this comment

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

  • io::Error::new does a lot of allocations. This should probably use io::Error::new_const.
  • This should now be ErrorKind::Uncategorized instead of Other.

fn from(other: AddAncillaryError) -> Self {
Self::new(io::ErrorKind::Other, other)
}
}
8 changes: 6 additions & 2 deletions library/std/src/os/unix/net/datagram.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,9 @@ impl UnixDatagram {
/// let fds = [0, 1, 2];
/// let mut ancillary_buffer = [0; 128];
/// let mut ancillary = SocketAncillary::new(&mut ancillary_buffer[..]);
/// ancillary.add_fds(&fds[..]);
/// unsafe {
/// ancillary.add_fds(&fds[..])?;
/// }
/// sock.send_vectored_with_ancillary_to(bufs, &mut ancillary, "/some/sock")
/// .expect("send_vectored_with_ancillary_to function failed");
/// Ok(())
Expand Down Expand Up @@ -570,7 +572,9 @@ impl UnixDatagram {
/// let fds = [0, 1, 2];
/// let mut ancillary_buffer = [0; 128];
/// let mut ancillary = SocketAncillary::new(&mut ancillary_buffer[..]);
/// ancillary.add_fds(&fds[..]);
/// unsafe {
/// ancillary.add_fds(&fds[..])?;
/// }
/// sock.send_vectored_with_ancillary(bufs, &mut ancillary)
/// .expect("send_vectored_with_ancillary function failed");
/// Ok(())
Expand Down
4 changes: 3 additions & 1 deletion library/std/src/os/unix/net/stream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,9 @@ impl UnixStream {
/// let fds = [0, 1, 2];
/// let mut ancillary_buffer = [0; 128];
/// let mut ancillary = SocketAncillary::new(&mut ancillary_buffer[..]);
/// ancillary.add_fds(&fds[..]);
/// unsafe {
/// ancillary.add_fds(&fds[..])?;
/// }
/// socket.send_vectored_with_ancillary(bufs, &mut ancillary)
/// .expect("send_vectored_with_ancillary function failed");
/// Ok(())
Expand Down
10 changes: 7 additions & 3 deletions library/std/src/os/unix/net/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,9 @@ fn test_send_vectored_fds_unix_stream() {

let mut ancillary1_buffer = [0; 128];
let mut ancillary1 = SocketAncillary::new(&mut ancillary1_buffer[..]);
assert!(ancillary1.add_fds(&[s1.as_raw_fd()][..]));
unsafe {
ancillary1.add_fds(&[s1.as_raw_fd()][..]).unwrap();
}

let usize = or_panic!(s1.send_vectored_with_ancillary(&bufs_send, &mut ancillary1));
assert_eq!(usize, 8);
Expand Down Expand Up @@ -551,7 +553,7 @@ fn test_send_vectored_with_ancillary_to_unix_datagram() {
cred1.set_pid(getpid());
cred1.set_uid(getuid());
cred1.set_gid(getgid());
assert!(ancillary1.add_creds(&[cred1.clone()][..]));
ancillary1.add_creds(&[cred1.clone()][..]).unwrap();

let usize =
or_panic!(bsock1.send_vectored_with_ancillary_to(&bufs_send, &mut ancillary1, &path2));
Expand Down Expand Up @@ -608,7 +610,9 @@ fn test_send_vectored_with_ancillary_unix_datagram() {

let mut ancillary1_buffer = [0; 128];
let mut ancillary1 = SocketAncillary::new(&mut ancillary1_buffer[..]);
assert!(ancillary1.add_fds(&[bsock1.as_raw_fd()][..]));
unsafe {
ancillary1.add_fds(&[bsock1.as_raw_fd()][..]).unwrap();
}

or_panic!(bsock1.connect(&path2));
let usize = or_panic!(bsock1.send_vectored_with_ancillary(&bufs_send, &mut ancillary1));
Expand Down