Skip to content

Tweak SocketAncillary API #86432

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
82 changes: 56 additions & 26 deletions library/std/src/os/unix/net/ancillary.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
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, MaybeUninit};
Expand Down Expand Up @@ -84,28 +85,22 @@ fn add_to_ancillary_data<T>(
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(MaybeUninit::new(0));
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 @@ -536,10 +531,9 @@ 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.
///
/// # Example
///
Expand All @@ -554,7 +548,7 @@ 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()][..]);
/// ancillary.add_fds(&[sock.as_raw_fd()][..])?;
///
/// let mut buf = [1; 8];
/// let mut bufs = &mut [IoSlice::new(&mut buf[..])][..];
Expand All @@ -563,7 +557,7 @@ impl<'a> SocketAncillary<'a> {
/// }
/// ```
#[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
pub fn add_fds(&mut self, fds: &[RawFd]) -> bool {
pub fn add_fds(&mut self, fds: &[RawFd]) -> Result<(), AddAncillaryError> {
self.truncated = false;
add_to_ancillary_data(
&mut self.buffer,
Expand All @@ -576,14 +570,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 @@ -642,3 +635,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

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

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)
}
}
4 changes: 2 additions & 2 deletions library/std/src/os/unix/net/datagram.rs
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,7 @@ 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[..]);
/// 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 +570,7 @@ 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[..]);
/// ancillary.add_fds(&fds[..])?;
/// sock.send_vectored_with_ancillary(bufs, &mut ancillary)
/// .expect("send_vectored_with_ancillary function failed");
/// Ok(())
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/os/unix/net/stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,7 @@ 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[..]);
/// ancillary.add_fds(&fds[..])?;
/// socket.send_vectored_with_ancillary(bufs, &mut ancillary)
/// .expect("send_vectored_with_ancillary function failed");
/// Ok(())
Expand Down
6 changes: 3 additions & 3 deletions library/std/src/os/unix/net/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ 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()][..]));
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 +551,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 +608,7 @@ 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()][..]));
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