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

Tracking Issue for feature(unix_socket_ancillary_data) #76915

Open
LinkTed opened this issue Sep 19, 2020 · 89 comments
Open

Tracking Issue for feature(unix_socket_ancillary_data) #76915

LinkTed opened this issue Sep 19, 2020 · 89 comments
Labels
A-io Area: std::io, std::fs, std::net and std::path C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. O-unix Operating system: Unix-like T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@LinkTed
Copy link
Contributor

LinkTed commented Sep 19, 2020

Tracking issue for feature(unix_socket_ancillary_data) to extend UnixStream and UnixDatagram to send and receive ancillary data. For example file descriptors.

Unresolved Questions

  • Review the SocketAncillary struct before stabilization to ensure it is compatible with all OSes.
  • fuchsia, haiku, illumos, ios, macos and solaris does not have MSG_CMSG_CLOEXEC constant in libc
  • fuchsia and uclibc(x86_64) does not have cmsghdr struct in libc
  • The current version of rust libc does not have ucred struct for the target OS emscripten, but emscripten has this struct in the standard C library

Implementation history

PR #69864, #82589, #83374

@LinkTed LinkTed added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Sep 19, 2020
@jonas-schievink jonas-schievink added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 19, 2020
@KodrAus KodrAus added the Libs-Tracked Libs issues that are tracked on the team's project board. label Nov 6, 2020
@KodrAus KodrAus added A-io Area: std::io, std::fs, std::net and std::path O-unix Operating system: Unix-like labels Jan 6, 2021
@tadeokondrak
Copy link
Contributor

tadeokondrak commented Feb 25, 2021

send_vectored_with_ancillary should switch to non-mutable references for its parameters, since sendmsg doesn't write through them.

#79753 changes just the bufs parameter, but it was closed for inactivity.

(Edit: Hmm, actually changing the ancillary data too would require both a SocketAncillary and SocketAncillaryMut, like IoSlice. Probably not worth it there)

@LinkTed
Copy link
Contributor Author

LinkTed commented Feb 27, 2021

@tadeokondrak I open a new PR #82589.

@LinkTed
Copy link
Contributor Author

LinkTed commented Mar 7, 2021

I create a new PR to send and receive TTL for UdpSocket from the ancillary interface. #82858

@reyk
Copy link
Contributor

reyk commented Mar 26, 2021

Hi @LinkTed, I posted a fix in #83374 for macOS, OpenBSD, and possibly other BSDs.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 29, 2021
unix: Fix feature(unix_socket_ancillary_data) on macos and other BSDs

This adds support for CMSG handling on macOS and fixes it on OpenBSD and possibly other BSDs.

When traversing the CMSG list, the previous code had an exception for Android where the next element after the last pointer could point to the first pointer instead of NULL.  This is actually not specific to Android: the `libc::CMSG_NXTHDR` implementation for Linux and emscripten have a special case to return NULL when the length of the previous element is zero; most other implementations simply return the previous element plus a zero offset in this case.

This MR makes the check non-optional which fixes CMSG handling and a possible endless loop on such systems; tested with file descriptor passing on OpenBSD, Linux, and macOS.

This MR additionally adds `SocketAncillary::is_empty` because clippy is right that it should be added.

This belongs to the `feature(unix_socket_ancillary_data)` tracking issue:  rust-lang#76915

r? `@joshtriplett`
@LinkTed
Copy link
Contributor Author

LinkTed commented Apr 4, 2021

What has to be done to stabilize this API?

@the8472
Copy link
Member

the8472 commented Apr 15, 2021

I think the API still needs polish. All those niche socket features are tricky and right now it's probably not extensible enough for 3rd party libraries to fill platform-specific gaps such as flags or cmsgs only supported on one particular OS. These basically are wrappers for the recvmsg and sndmsg syscalls, we should expose them in a way that's forwards-compatible so we don't have to add recv_vectored_with_ancillary_from_with_flags or something like that in the future.

Before stabilizing we should consider whether recv_vectored_with_ancillary_from should gain another argument. I would recommend adding a ReceiveOptions argument that allows one to specify flags, similar to OpenOptions. The options could then be used to set linux' MSG_DONTWAIT, MSG_PEEK or MSG_ERRQUEUE for example, maybe via custom_flags() similar to OpenOptions. That way we don't have to support all OS-specific peculiarities.

Or we could put the flags into SocketAncillary or at least reserve the right to do that in the future, but in that case the struct would need to be renamed because it would serve several purposes at once.

Similar could be done for sending.

Additionally the AncillaryData enum should be marked as #[non_exhaustive] and maybe gain a Unknown variant for things that the standard library doesn't support. And a way to get the raw ancillary message. That way one could for example decode struct sock_extended_err in a 3rd party crate without waiting for std to implement it.

@LinkTed
Copy link
Contributor Author

LinkTed commented Apr 17, 2021

@the8472 Thank you for your feedback. If this PR request is #82858 merged I will start implementing your suggestions.

@tv42
Copy link
Contributor

tv42 commented Jun 6, 2021

What about CMSG_SPACE? Without it, we're forced to either guess or probe the appropriate buffer size.

https://man7.org/linux/man-pages/man3/cmsg.3.html SCM_RIGHTS example shows how it's used to compute the correct buffer size (with some tricky business wrt alignment).

@LinkTed
Copy link
Contributor Author

LinkTed commented Jun 6, 2021

Hi @tv42,
what are the difference between CMSG_SPACE and CMSG_LEN?

Is there a function, which calculate the alignment for a struct to a specific address in Rust?

@comex
Copy link
Contributor

comex commented Jun 6, 2021

what are the difference between CMSG_SPACE and CMSG_LEN?

CMSG_SPACE includes alignment bytes, CMSG_LEN doesn't.

Is there a function, which calculate the alignment for a struct to a specific address in Rust?

I don't know what this means.

@LinkTed
Copy link
Contributor Author

LinkTed commented Jun 6, 2021

what are the difference between CMSG_SPACE and CMSG_LEN?

CMSG_SPACE includes alignment bytes, CMSG_LEN doesn't.

Is there a function, which calculate the alignment for a struct to a specific address in Rust?

I don't know what this means.

I thought that the alignment depends on the address, where you want to store the struct.

@zopsicle
Copy link
Contributor

zopsicle commented Jun 6, 2021

What it means for CMGS_SPACE to include “alignment bytes” is that it rounds the size up to include a number of padding bytes to make sure the next header is properly aligned (to the alignment of cmsghdr).

@LinkTed
Copy link
Contributor Author

LinkTed commented Jun 6, 2021

So, the address have to be a multiple of the return value of CMSG_ALIGN, right?

@zopsicle
Copy link
Contributor

zopsicle commented Jun 6, 2021

The address of the cmsghdr object has to be a multiple of std::mem::align_of::<cmsghdr>().

@LinkTed
Copy link
Contributor Author

LinkTed commented Jun 6, 2021

Sorry, that I ask again. Does the macro CMSG_ALIGN the same as std::mem::align_of::<cmsghdr>()?

@zopsicle
Copy link
Contributor

zopsicle commented Jun 6, 2021

CMSG_ALIGN(len) rounds len up to a multiple of std::mem::align_of::<cmsghdr>(). For instance, if the required alignment is 8, and len is 11, CMSG_ALIGN(len) will return 16.

As an example, consider the following two headers:

let A = cmsghdr{ cmsg_len = 19, cmsg_level = X, cmsg_type = Y };
let B = cmsghdr{ cmsg_len = 22, cmsg_level = W, cmsg_type = Z };

If cmsg_len were not rounded up, this sequence of headers would look like this in memory:

          ~~~~~~~~~A~~~~~~~~~ ~~~~~~~~~B~~~~~~~~~
         +----+----+----+----+----+----+----+----+
Address: |  0 |  8 | 12 | 16 | 19 | 27 | 31 | 35 |
         +----+----+----+----+----+----+----+----+
Data:    | 19 |  X |  Y |  _ | 22 |  W |  Z |  _ |
         +----+----+----+----+----+----+----+----+

Notice how header B is at improperly aligned address 19, which is not a multiple of the alignment of cmsghdr.

By rounding up the lengths (assuming an alignment of 8 for this example), we get this:

          ~~~~~~~~~A~~~~~~~~~      ~~~~~~~~~B~~~~~~~~~
         +----+----+----+----+----+----+----+----+----+----+
Address: |  0 |  8 | 12 | 16 | 19 | 24 | 32 | 36 | 40 | 46 |
         +----+----+----+----+----+----+----+----+----+----+
Data:    | 19 |  X |  Y |  _ |    | 22 |  W |  Z |  _ |    |
         +----+----+----+----+----+----+----+----+----+----+
                              ~~~~ padding bytes       ~~~~ padding bytes

Now, every header and each field in every header is at the aligned address it should be.

@LinkTed
Copy link
Contributor Author

LinkTed commented Jun 6, 2021

Thanks, @chloekek. Currently, the code is using CMSG_LEN, which includes the alignment bytes. So, technically we have to ensure that the first cmsg struct is properly aligned, right?

But, if cmsg_len is rounded up, this means that the receiver interpreted the alignment bytes as data. For example, if the sender set 3 bytes, which then is aligned to 8, the receiver will think that he got 8 bytes. How do we handle this?

@zopsicle
Copy link
Contributor

zopsicle commented Jun 6, 2021

Actually, the man page says that you should include the padding in cmsg_len:

CMSG_LEN() returns the value to store in the cmsg_len member of the cmsghdr structure

This means that the receiver will indeed interpret those bytes as part of the data, I don’t think there’s any way around that?

This would mean that, if alignof(cmsghdr) = 8, and sizeof(int) = 4, you could only send an even number of file descriptors using SCM_RIGHTS. 😅 I guess if you want to send an odd number of file descriptors you could program the sender to set the odd one to -1 and program the receiver to ignore any -1s.

@LinkTed
Copy link
Contributor Author

LinkTed commented Jun 6, 2021

Interesting, currently there is a test, which send one FD and I tested on a 64-bit system, where the alignment of cmsghdr should be 8. 😀

@zopsicle
Copy link
Contributor

zopsicle commented Jun 6, 2021

What is suspicious is that CMSG_NXTHDR adds the alignment if necessary.

This could mean that either:

  • CMSG_NXTHDR is just being cautious to avoid undefined behavior (dereferencing pointer to unaligned data is UB).
  • You don’t actually have to include the padding in cmsg_len, despite what the man page suggests.

Note that while you could test this, other Unix implementations (e.g. BSD) may behave differently from Linux in this regard. It would be best to check the Official Spec™ (POSIX?).

As for file descriptors, the kernel will indeed divide the data length by the size of a file descriptor in order to compute how many file descriptors to send. And it seems that you indeed do have to set all of them to valid file descriptors, not to -1.

@LinkTed
Copy link
Contributor Author

LinkTed commented Jun 6, 2021

I think the man page is wrong. CMSG_LEN does not include the alignment. Only CMSG_SPACE as @comex suggested.

@zopsicle
Copy link
Contributor

zopsicle commented Jun 6, 2021

So to summarize:

  • Set cmsg_len to the return value of CMSG_LEN. cmsg_len can be any value greater than sizeof(cmsghdr).
  • Make sure that the next header is at a properly aligned address. CMSG_NXTHDR will round the address up for you, or you can use CMSG_ALIGN if you do the pointer arithmetic yourself.

@joshtriplett
Copy link
Member

I do care about the state of this API, and I'm in particular quite interested in the fd-sending mechanism.

It's certainly a good candidate for using whatever new where Os: Unix mechanism or similar that we introduce in the future.

bors added a commit to rust-lang-ci/rust that referenced this issue May 26, 2022
@m-ou-se m-ou-se removed the I-libs-nominated Nominated for discussion during a libs team meeting. label Jun 1, 2022
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
@jmillikin
Copy link
Contributor

jmillikin commented Sep 19, 2022

I'm interested in this feature and have time to work on it. However, the current API is very different from what I would expect:

  • The current API for AncillaryData is closely tied with UNIX sockets and doesn't seem to have any support for ancillary data from (for example) TCP/IPv6 sockets.
  • Putting an enum for possible SCM_* values into std::os::unix prevents use of OS-specific values such as Linux's SCM_SECURITY.

How much flexibility is available for making changes to the API at this point? If I were to write an RFC with an API approximating the following, would that be a plausible next step toward stabilization?

// ===== API generic to any UNIX platform
trait unix::net::SocketSend {
  // equivalent to sendmsg()
  fn send_message(..., Option<&mut AncillaryData>) -> io::Result<usize>;
}
trait unix::net::SocketRecv {
  // equivalent to recvmsg()
  fn receive_message(..., Option<&mut AncillaryData>) -> io::Result<usize>;
}

struct unix::net::AncillaryData;
  fn control_messages<'a>(&'a self) -> impl Iter<ControlMessage<'a>>;

struct unix::net::ControlMessage<'a>;
  fn level() -> c_int;
  fn type() -> c_int;
  fn data() -> &'a [u8]

// ===== Platform-specific API (Linux)
#[non_exhaustive]
enum linux::net::ControlMessage<'a> {
  ScmRights(ScmRights<'a>),
  ScmCredentials(ScmCredentials<'a>),
}
trait linux::net::ControlMessageExt: Sealed {
  fn try_get(&self) -> Option<linux::net::ControlMessage<'a>>,
}

struct linux::net::ScmCredentials<'a>;
  fn new(&unix::net::ControlMessage) -> Option<Self>;

struct linux::net::ScmRights<'a>;
  fn new(&unix::net::ControlMessage) -> Option<Self>;

// ===== Platform-specific API (etc)
trait freebsd::net::ControlMessageExt: Sealed { ... }
trait macos::net::ControlMessageExt: Sealed { ... }
trait netbsd::net::ControlMessageExt: Sealed { ... }

Advantages:

  • API layout not affected by #[cfg(target_os)]
  • New platform-specific functionality can be added freely via non_exhaustive enums.
  • Exposing the ControlMessage fields directly allows users to self-service control message types not yet supported by the Rust standard library.

@kotauskas
Copy link

I found a potential critical concern with how ScmRights deals with its file descriptors upon being dropped – or, more specifically, how it does not.

rlimit DoS via SCM_RIGHTS is a pretty insidious issue that can easily go unnoticed and needs to be manually mitigated by the user of the standard library to make sure that file descriptor table pollution is avoided and no resources are leaked.

My own implementation of Unix domain sockets and ancillary data in the interprocess crate (actual current ancillary data code not pushed to main yet) does this differently and opts for a wrapper struct for SCM_RIGHTS (not too unlike ScmRights) that holds conditional ownership over its file descriptors, depending on whether it was constructed with &[BorrowedFd] or received from the kernel. If it's logically &[OwnedFd] with logical quasi-ownership through an immutable reference (as it would be if the FDs came from the kernel), it closes the descriptors upon being dropped, averting descriptor table pollution. It's not the cleanest implementation and I'm still working on it, but it's better than relegating prevention of rlimit DoS onto the downstream user and it takes care not to double-free.

@jmillikin
Copy link
Contributor

I wrote a draft RFC for a v2 of Unix socket ancillary data support: rust-lang/rfcs#3430

In my testing, that proposal solves all the known issues with the current implementation (platform-specific control messages, third-party library extensibility, and file descriptor ownership). I would be happy if folks interested in stabilized ancillary data support could take a look at that RFC.

@the8472
Copy link
Member

the8472 commented May 10, 2023

Are you planning to implement it too? Asking since I intended to work on this eventually.

@jmillikin
Copy link
Contributor

Yes, if that RFC is accepted then I plan to implement it. I already have it working in a local branch, so it'd just be a matter of adding docs and tests.

@jmillikin
Copy link
Contributor

The RFC looks like it's moving now (tagged libs-api-nominated) so I've started work on the implementation of non-contentious parts.

@purplesyringa
Copy link

As far as I can see, the current implementation is unsound: it takes a user-provided buffer and performs unaligned reads and writes. Is this a known issue?

purplesyringa added a commit to purplesyringa/crossmist that referenced this issue Jan 4, 2024
253 seems to be a hardcoded value without any particular reasoning. Move it to a constant and use a
rounder value of 128.

There were also troubles regarding alignment: the API asks us to pass an arbitrary byte buffer and
then performs unaligned reads/writes. Workaround that by aligning the buffer manually.

For more information, see rust-lang/rust#76915 (comment)
@GrahamBarnett
Copy link

GrahamBarnett commented Apr 26, 2024

  1. Would if be possible to make the constructor public?
  pub fn new(buffer: &'a mut [u8]), current_length: usize, truncated: bool -> Self {
    SocketAncillary { buffer, length: current_length, truncated }
  }

this allows us to work on an existing ancillary buffer or re-use a SocketAncillary on different read buffers if we want, and as a by-product makes testing a lot easier

  1. Please add #[derive(Debug)] to
  SocketCred, 
  ScmRights, 
  AncillaryDataIter<'a, T>
  AncillaryData<'a>

and:

  impl<'a> std::fmt::Debug for ScmRights<'a> {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.debug_list()
            .entries(Self(unsafe { AncillaryDataIter::<'a, RawFd>::new(self.0.data) }).collect::<Vec<RawFd>>())
            .finish()
    }
  }
  
  impl<'a> std::fmt::Debug for ScmCredentials<'a> {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.debug_list()
            .entries(Self(unsafe { AncillaryDataIter::<'a, libc::ucred>::new(self.0.data) }).collect::<Vec<SocketCred>>())
            .finish()
    }
  }
  
  impl<'a> std::fmt::Debug for Messages<'a> {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        let messages = Messages{buffer: self.buffer, current: self.current};
        f.debug_list().entries(messages.collect::<Vec<Result<AncillaryData<'a>, AncillaryError>>>()).finish()
    }
  }`

this allows us to println!("{:?}", ancillary.messages()) which makes logging/ debugging a lot easier.
It also means that you can debug the SocketAncillary struct itself (and get buffer status), or can show messages themselves as needed.

  1. I had these changes suggested by the linter on 1.75:
    let data_len = (*cmsg).cmsg_len as usize - cmsg_len_zero; -->linter says should be--> let data_len = cmsg.cmsg_len - cmsg_len_zero;
    match (*cmsg).cmsg_level { -->linter says should be--> match cmsg.cmsg_level {
    cmsg_type: (*cmsg).cmsg_type, -->linter says should be--> cmsg_type: cmsg.cmsg_type,

Many thanks

@andrewbaxter
Copy link

andrewbaxter commented May 4, 2024

  1. Would if be possible to make the constructor public?

My use case is I don't know exactly where in a stream ancillary data will appear - I'd like to use several reads to parse a message and aggregate ancillary data across them (in a single buffer) to be used at the end. It doesn't appear trivially possible with the current design.

@GrahamBarnett
Copy link

GrahamBarnett commented May 4, 2024 via email

@MaxVerevkin
Copy link

Wouldn't it be better for SocketAncillary's add_creds and add_fds to return a Result<(), NotEnoughSpaceErrror> instead of a bool?

@jonleivent
Copy link

Would it be better for SocketAncillary::new to take a MaybeUninit buffer? I assume that the original buffer should not be read from after calling SocketAncillary::new. Also, this saves the initialization expense.

@the8472
Copy link
Member

the8472 commented Sep 17, 2024

When initializing a buffer that will contain a series of
              cmsghdr structures (e.g., to be sent with [sendmsg(2)](https://man7.org/linux/man-pages/man2/sendmsg.2.html)),
              that buffer should first be zero-initialized to ensure the
              correct operation of CMSG_NXTHDR().

at least sending it requires zero-initialization anyway.

@jonleivent
Copy link

that buffer should first be zero-initialized to ensure the
correct operation of CMSG_NXTHDR().

Is that the responsibility of the caller of, or of SocketAncillary::new itself? Because if this is a safety concern, how do you enforce it on the caller? Wouldn't a common mistake be to re-use the buffer for subsequent SocketAncillary::new calls without resetting its contents?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: std::io, std::fs, std::net and std::path C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. O-unix Operating system: Unix-like T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests