Tweak SocketAncillary API #86432

de-vri-es · 2021-06-18T10:20:40Z

This PR tweaks the API of the unix-specific SocketAncillary struct.

It does four things:

Add data(), buffer(), buffer_mut() and set_len() to allow integration with manual syscalls, for example when implementing unsupported socket types outside of std.
Switch to &mut [MaybeUnit<u8>] internally and add new_uninit().
Have add_fds() and add_creds() return a Result to indicate failure.
Mark add_fds() unsafe.

The reason for making add_fds() unsafe is that no lifetimes are tracked, so the FD could be closed and re-assigned before the ancillary data is actually used. This would result in sending the wrong file descriptor which may end up messing with an otherwise safe memory mapping.

de-vri-es · 2021-06-18T10:23:00Z

library/std/src/os/unix/net/ancillary.rs

+    ///
+    /// Use [`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>] {


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.

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

Sounds good to me.

de-vri-es · 2021-06-18T10:24:46Z

library/std/src/os/unix/net/ancillary.rs

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


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.

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.

de-vri-es · 2021-06-18T10:32:44Z

library/std/src/os/unix/net/ancillary.rs

+    /// - 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) {


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.

library/std/src/os/unix/net/ancillary.rs

the8472 · 2021-06-18T10:45:02Z

library/std/src/os/unix/net/ancillary.rs

+    #[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
+    pub unsafe fn buffer_mut(&mut self) -> &mut [MaybeUninit<u8>] {
+        self.buffer
+    }


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.

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).

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?

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.

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.

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.

The `SocketAncillary` object has no lifetime that could guarantee that the file descriptor remains open long enough. That means it could end up duplicating arbitrary file descriptors and send them to a remote process. Additionally, even if the fd lifetime was tracked, duplicating an FD and sending it to a remote process could violate the safety requirements of the object managing the file descriptor. This matters, because `as_raw_fd()` is not unsafe.

de-vri-es · 2021-06-18T12:25:38Z

Pushed a commit to make add_fds() unsafe. I also updated the PR description with the reasoning why.

LinkTed · 2021-06-19T11:36:57Z

library/std/src/os/unix/net/ancillary.rs

+/// 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 {


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.

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.

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

LinkTed · 2021-06-19T12:11:04Z

@de-vri-es There is a PR #82858, to extend the SocketAncillary. If you have time, you could review it :).

de-vri-es · 2021-06-23T09:45:51Z

@de-vri-es There is a PR #82858, to extend the SocketAncillary. If you have time, you could review it :).

I've looked it over and left some comments :)

JohnCSimon · 2021-11-08T15:37:56Z

Triage:
@m-ou-se This has been in review state since July.

m-ou-se

Mark add_fds() unsafe.

Can the new BorrowedFd or OwnedFd types be of use here?

m-ou-se · 2021-12-04T16:38:29Z

library/std/src/os/unix/net/ancillary.rs

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


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.

m-ou-se · 2021-12-04T16:39:31Z

library/std/src/os/unix/net/ancillary.rs

+    ///
+    /// Use [`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>] {


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

Sounds good to me.

JohnCSimon · 2022-02-13T00:47:03Z

ping from triage:
@de-vri-es
Returning to author to address comments.

Can the new BorrowedFd or OwnedFd types be of use here?

When it's ready for review send a message containing
@rustbot ready to switch to S-waiting-on-review

@rustbot author

JohnCSimon · 2022-03-20T05:23:11Z

@de-vri-es
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this. Thanks for your contribution.

@rustbot label: +S-inactive

rust-highfive assigned m-ou-se Jun 18, 2021

rustbot added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` C-enhancement Category: An issue proposing an enhancement or a PR with one. O-unix Operating system: Unix-like T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 18, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 18, 2021

de-vri-es commented Jun 18, 2021

View reviewed changes

de-vri-es added 3 commits June 18, 2021 12:27

Support using SocketAncillary with raw syscalls with unsafe functions.

15b6441

Allow ancillary socket data to be read into an uninitialized buffer.

28e9eca

Make SocketAncillary::add_* return a Result to indicate failure.

327cb39

de-vri-es force-pushed the socket-ancillary-tweaks branch from fb99d92 to 327cb39 Compare June 18, 2021 10:27

de-vri-es commented Jun 18, 2021

View reviewed changes

de-vri-es changed the title ~~Tweaks SocketAncillary API~~ Tweak SocketAncillary API Jun 18, 2021

the8472 reviewed Jun 18, 2021

View reviewed changes

Add note on zero-initialization and CMSG_NXTHDR.

bf85a1a

de-vri-es force-pushed the socket-ancillary-tweaks branch from caab6f4 to bf85a1a Compare June 18, 2021 11:39

LinkTed reviewed Jun 19, 2021

View reviewed changes

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2021

LinkTed mentioned this pull request Jul 24, 2021

UnixStream / UnixDatagram requires API for sending packet with FDs tokio-rs/tokio#2975

Open

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2021

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2021

JohnCSimon removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 6, 2021

JohnCSimon added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 6, 2021

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 27, 2021

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2021

m-ou-se reviewed Dec 4, 2021

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 13, 2022

JohnCSimon closed this Mar 20, 2022

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Mar 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweak SocketAncillary API #86432

Tweak SocketAncillary API #86432

de-vri-es commented Jun 18, 2021 •

edited

Loading

de-vri-es Jun 18, 2021 •

edited

Loading

m-ou-se Dec 4, 2021

de-vri-es Jun 18, 2021 •

edited

Loading

m-ou-se Dec 4, 2021

de-vri-es Jun 18, 2021

the8472 Jun 18, 2021

de-vri-es Jun 18, 2021 •

edited

Loading

the8472 Jun 18, 2021

de-vri-es Jun 18, 2021

the8472 Jun 18, 2021

de-vri-es Jun 18, 2021 •

edited

Loading

de-vri-es commented Jun 18, 2021

LinkTed Jun 19, 2021

de-vri-es Jun 23, 2021 •

edited

Loading

LinkTed Jun 23, 2021

LinkTed commented Jun 19, 2021

de-vri-es commented Jun 23, 2021

JohnCSimon commented Nov 8, 2021 •

edited

Loading

m-ou-se left a comment

m-ou-se Dec 4, 2021

m-ou-se Dec 4, 2021

JohnCSimon commented Feb 13, 2022

JohnCSimon commented Mar 20, 2022

Tweak SocketAncillary API #86432

Tweak SocketAncillary API #86432

Conversation

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

de-vri-es commented Jun 18, 2021

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinkTed commented Jun 19, 2021

de-vri-es commented Jun 23, 2021

JohnCSimon commented Nov 8, 2021 • edited Loading

m-ou-se left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnCSimon commented Feb 13, 2022

JohnCSimon commented Mar 20, 2022

de-vri-es commented Jun 18, 2021 •

edited

Loading

de-vri-es Jun 18, 2021 •

edited

Loading

de-vri-es Jun 18, 2021 •

edited

Loading

de-vri-es Jun 18, 2021 •

edited

Loading

de-vri-es Jun 18, 2021 •

edited

Loading

de-vri-es Jun 23, 2021 •

edited

Loading

JohnCSimon commented Nov 8, 2021 •

edited

Loading