RFC: Unix socket ancillary data v2 #3430

jmillikin · 2023-05-10T08:25:04Z

NobodyXu · 2023-05-10T08:51:57Z

text/3430-unix-socket-ancillary-data-v2.md

+information about received file descriptors).
+
+```rust
+struct ControlMessagesBuf;


Would it make sense to have a SmallVec like buffer here:

Suggested change

struct ControlMessagesBuf;

struct ControlMessagesBuf<const N: usize>;

where N decides how many messages it can store inline?

The ControlMessagesBuf capacity is in bytes, rather than messages -- and some message types are dynamically sized, for example SCM_RIGHTS is semantically a (usize, [i32]) tuple.

I think it would be possible to have an AncillaryData backed by a &mut [u8], but when writing the draft implementation I found it difficult to use. If the number of heap allocations is a concern, reusing an AncillaryData between socket calls is probably more practical than stack allocation.

text/3430-unix-socket-ancillary-data-v2.md

the8472 · 2023-05-10T18:36:40Z

text/3430-unix-socket-ancillary-data-v2.md

+    fn start_recvmsg(&mut self) -> Option<&mut [u8]>;
+    unsafe fn finish_recvmsg(&mut self, control_messages_len: usize);


This might be necessary for use with sendmmsg, recvmmsg, io_uring. But for standard sendmsg/recvmsg it would be better to have some generic implementation that takes any AsFd.

I'm not sure I understand the meaning of this comment. Was it meant to be placed elsewhere?

If it helps, here's how these functions are used:

impl RecvMessage for UnixStream { fn recv_message( &self, bufs: &mut [IoSliceMut<'_>], ancillary_data: &mut AncillaryData<'_>, options: RecvOptions, ) -> io::Result<(usize, RecvResult)> { let mut msg: libc::msghdr = unsafe { zeroed() }; msg.msg_iov = bufs.as_mut_ptr().cast(); msg.msg_iovlen = bufs.len() as _; if let Some(ancillary_buf) = ancillary_data.start_recvmsg() { msg.msg_control = ancillary_buf.as_mut_ptr().cast(); msg.msg_controllen = ancillary_buf.len(); } let size = self.0.recv_msg(&mut msg, options.as_recv_flags())?; unsafe { ancillary_data.finish_recvmsg(msg.msg_controllen) }; Ok((size, RecvResult::new(msg.msg_flags))) } }

the8472 · 2023-05-10T18:51:48Z

text/3430-unix-socket-ancillary-data-v2.md

+```rust
+struct MessageSender<S, 'a>;
+
+impl<S, 'a> MessageSender<S, 'a> {


I'm not sure if this carries its weight. It only borrows its member so it can never live long. At that point one might as well call the SendMessage trait directly. Ditto for MessageReceiver.

Calling the trait directly is fairly verbose for simple cases, and I found MessageSender useful when writing small utilities to experiment with the shape of the API. It's not strictly necessary, but is nice to have from an ergonomics perspective.

Without it we'd probably want the traits to have send_message, send_message_vectored, send_message_vectored_with_flags, and so on. I'm not 100% opposed to this if it's what the libs-api team prefers, but as a future user of this API I'd prefer having a helper struct available.

text/3430-unix-socket-ancillary-data-v2.md

riking · 2023-05-15T22:51:15Z

text/3430-unix-socket-ancillary-data-v2.md

+impl RecvResult {
+    fn new(flags: c_int) -> RecvResult;
+
+    fn custom_flags(&self, flags: c_int) -> bool;


This should be fn custom_flags(&self, flag_mask: c_int) -> c_int; in order to accommodate flags that take multiple bits.

Hmm, I had expected the implementation would be something like this, which would (I think?) work for multi-bit flags:

fn custom_flags(&self, flags: c_int) -> bool { self.bits & flags == flags }

Is there a case that would need the user to inspect the masked bits directly? I'm fine with using that API, but would like to make sure I understand it so I can write good examples in the docs.

what happens when you have a 2-bit field and you want to see if it's 2? self.bits & flags == flags can't do that because the mask would need to be e.g. 0x30 but would need to be compared against 0x20

I'm not sure what that means, sorry. If the platform has msg_flags flags like this:

// set on `(*struct msghdr)->msg_flags const MSG_DID_MAGIC: c_int = 0x30;

Then testing whether that flag was set would need to flags & MSG_DID_MAGIC == MSG_DID_MAGIC.

Did you have a particular platform / flag in mind? Looking up its definition in the OS headers or libc might be helpful.

if you had:

const MSG_FOO_MODE_MASK: c_int = 0x30; const MSG_FOO_MODE_A: c_int = 0; const MSG_FOO_MODE_B: c_int = 0x10; const MSG_FOO_MODE_C: c_int = 0x20; const MSG_FOO_MODE_D: c_int = 0x30; fn foo_mode_is(msg_flags: c_int, mode: c_int) -> bool { msg_flags & MSG_FOO_MODE_MASK == mode }

then we should design it so you can have an equivalent for foo_mode_is(msg_flags, MSG_FOO_MODE_C) where it checks that both bit 1 << 4 is zero and bit 1 << 5 is one, since both bits need to be checked to match and not all bits are ones. flags & mask == mask is only capable of checking for bits that are ones, it is incapable of checking for zeros.

That's the first time I've seen that sort of structure in a msghdr flag. Which OS / architecture has recvmsg message flags like that?

Also, if there are any OSes with flags like that, then I'd probably try to have an API like this (names illustrative):

impl RecvResult { fn custom_flags(&self, flags: c_int) -> bool; fn as_raw_flags(&self) -> c_int; // ... + methods for POSIX-defined flags }

So that most platform-specific flags can be checked easily, and any that use multi-bit values with masking can do that part in their own code.

That's the first time I've seen that sort of structure in a msghdr flag. Which OS / architecture has recvmsg message flags like that?

nothing I'm aware of, I was just attempting to elaborate on @riking's comment. That said, multi-bit flags like that could totally exist or be added sometime in the future.

text/3430-unix-socket-ancillary-data-v2.md

sunfishcode · 2023-10-20T23:56:28Z

I'm curious why Rust's standard library should have ancillary data support, as opposed to letting this functionality be provided by third-party libraries. The RFC mentions nix; another is rustix. Those specific libraries don't satisfy all the RFC's motivations, however, a new library API could be developed that does.

With the Standard library developer's guide guidance in mind: ancillary data doesn't require special language or compiler support, and doesn't need to extend standard library types—it's possible to build the functionality on top of the existing AsFd/AsSocket impls. And, it is a somewhat complex API which very plausibly may want to evolve over time, in a number of different dimensions.

jmillikin · 2023-10-21T01:52:38Z

I'm curious why Rust's standard library should have ancillary data support, as opposed to letting this functionality be provided by third-party libraries. The RFC mentions nix; another is rustix. Those specific libraries don't satisfy all the RFC's motivations, however, a new library API could be developed that does.

As a general matter I think it makes sense for Rust to have a large and capable standard library.

I'm aware this is not a universally-held opinion. Some people think Rust should delegate OS-specific functionality to third-party libraries, as is done in the JavaScript world. In this model there is a large set of implementations for everything from filesystem interaction to networking, and users are expected to mix-n-match to fit their needs.

Using your library rustix as an example, it defines an enormous API surface -- writing files, opening network sockets, launching subprocesses, and so on. It's basically its own standard library specialized for Unix-ish platforms.

However, from my perspective, a library like rustix is unacceptable as a dependency. It's unstable (pre-v1.0) and has had ~70 releases in the past year, which is a release cadence I'm completely unequipped to keep up with. And this is before getting into its hefty dependency tree -- picking one at random, rustix depends on https://github.com/lambda-fairy/rust-errno, which is also pre-v1.0 and has 24 versions.

Finally, yes, I could take this RFC's code and release it as a standalone library. But then I'm just contributing to the problem.

With the Standard library developer's guide guidance in mind: ancillary data doesn't require special language or compiler support, and doesn't need to extend standard library types—it's possible to build the functionality on top of the existing AsFd/AsSocket impls. And, it is a somewhat complex API which very plausibly may want to evolve over time, in a number of different dimensions.

The same could be said for pretty much everything under std::os::unix, and (per the first half of this comment) I am unconvinced by that argument. Supporting widely-implemented standard POSIX functionality in the standard library means that simple uses don't need to pull in anything else, and complex use cases (rare platforms, newly-developed control messages) can use the provided standard traits/structs as a Schelling point.

the8472 · 2023-10-21T09:43:42Z

Imo the value of adding cmsg support is that it's both tricky to implement (with all those gnarly C macros and unaligned data and so on) and very useful for common unix patterns such as fd-passing and checking peer credentials.
By adding it we make it easier to write more unixy code without reaching for gobs of unsafe.

FD-passing via unix socket is also the sanest way among alternatives. Using procfs or inheritance has issues.

There even is a use of fd-passing in std itself and it would be great if we could replace that with a safe abstraction: https://github.com/rust-lang/rust/blob/45a45c6e60835e15c92374be1f832bc756fc8b1a/library/std/src/sys/unix/process/process_unix.rs#L670-L777
Though ControlMessagesBuf would be inappropriate here. Allocations are not permitted in the pre_exec context.

And, it is a somewhat complex API which very plausibly may want to evolve over time, in a number of different dimensions.

What do you have in mind? With RecvOptions and RecvResult it seems like we can already fully cover the recvmsg API, even if new flags get added in the future.

Noah-Kennedy · 2023-10-21T17:05:23Z

IMO one of the key advantages here of having this in std is that libraries such as tokio generally try and match APIs in std, and will sometimes try and hold off on developing our own APIs for something so that we don't fall out of sync with std when we don't need to.

I don't actually see this being used too much by users of std, however I would like to see std adopt an implementation of ancillary data so that others who use std as a reference can adopt this without worrying about falling out of sync with std.

programmerjake · 2023-10-24T01:06:09Z

rust-lang/libs-team#284 (comment)

ControlMessagesBuf owning an allocation seems quite problematic since network code often wants to work with stack allocations, recycle allocations in weird ways or interpret buffers coming from somewhere else

maybe add ControlMessagesBuf::clear()? that's equivalent to dropping (which presumably cleans up any owned FDs, which is the reason for not having any methods to remove some items) and re-creating it except without needing to reallocate.

jmillikin · 2023-10-24T02:16:02Z

maybe add ControlMessagesBuf::clear()?

The clear method is AncillaryData::clear() because ControlMessagesBuf doesn't know which parts of its buffer are FDs.

jmillikin · 2023-10-28T01:57:25Z

I've reworked the middle layer of the design with the goal of supporting stack allocation and clarifying the ownership of received FDs.

@the8472 could you please let me know if the changes resolve your concerns in rust-lang/libs-team#284 (comment) ? The details are:

ControlMessagesBuf no longer exists.
AncillaryData is now constructed with a user-provided &[MaybeUninit<u8>] for its control messages capacity.
New type AncillaryDataBuf provides heap allocation and can be used to construct an AncillaryData of dynamic size.
I replaced start_recvmsg / finish_recvmsg with three new methods on AncillaryData (fn control_messages_buf(), fn set_control_messages_len(), and unsafe fn take_ownership_of_scm_rights()).

de-vri-es · 2023-11-06T11:58:26Z

text/3430-unix-socket-ancillary-data-v2.md

+    // copy a control message into the ancillary data; error on out-of-capacity.
+    fn add_control_message(
+        &mut self,
+        control_message: impl Into<ControlMessage<'_>>,


Wouldn't it make more sense to demand impl Into<ControlMessage<'fd>>?

Then you could implement From<BorrowedFd<'fd>> and From<&'fd [BorrowedFd<'fd>]> for ControlMessage<'fd> and get rid of the special add_file_descriptors().

It could also be useful for future additions of specific control messages that borrow data.

I would even suggest that the name 'fd is a bit specific. In essence the two lifetimes represent the byte buffer and the encoded resources. So 'buf and 'res could be used as more generic names.

In this design, control messages are for structured viewing and appending to the msg_control buffer. Think of the ControlMessage struct as a Rustic equivalent to the CMSG_*() macros. They don't own file descriptors -- the ownership of FDs is managed by AncillaryData.

The purpose of the separate layers is to try to find the (rather small!) intersection between Rust's ownership semantics, the API idioms of early-1980s Unix, and a desire to avoid heap-allocating everything.

Thinking purely in terms of ownership, the logical signature of sendmsg / recvmsg might be notated something like this:

trait ControlMessage { ... } sendmsg( &self, buf: &[u8], cmsg: Vec<Box<dyn ControlMessage>>, output_cmsg_capacity: usize, rights: Vec<BorrowedFd<'_>>, ) -> (io::Result<usize>, Vec<Box<dyn ControlMessage>>); recvmsg( &self, buf: &mut [MaybeUninit<u8>], cmsg: Vec<Box<dyn ControlMessage>>, output_cmsg_capacity: usize, ) -> (io::Result<&mut [u8]>, Vec<Box<dyn ControlMessage>>, Vec<OwnedFd>);

In these signatures it's clear how the control messages are consumed and FDs are borrowed/produced. However, there's just no way such definitions could be used in Rust std -- besides the awful call sites, you'd have to heap-allocate everything and you'd need to bake in knowledge of all the various SCM_* types.

Supporting third-party libraries that want to add support for OS-specific SCM_* types means that we need to expose the actual raw byes of control messages in some reasonably accessible place -- hence struct ControlMessage<'a> providing a borrowed view of a &'a ScmTimestampPktinfo or whatever.

But if you're exposing raw cmsg bytes, they can't also be used for ownership of FDs without adding even more complexity to the API surface. I tried -- the original version of this had ControlMessage<'fd> and a pub enum ScmRights { Borrowed(...), Owned(..) } -- but it just didn't work well.

Hmm, but nothing prevents a user from adding SCM_RIGHTS manually using this function, and then the lifetime of the FDs isn't checked. So I still think this should have a lifetime for the resources the control message represents.

Even then, I think this function should be unsafe, as the control message contains instructions for the kernel. So who knows what it will do (the lifetime will still help prevent wrong usage though).

Hmm, but nothing prevents a user from adding SCM_RIGHTS manually using this function, and then the lifetime of the FDs isn't checked. So I still think this should have a lifetime for the resources the control message represents.

If we want to allow third-party code to implement arbitrary SCM_* control message types, then there's no way to prevent the user from manually encoding an SCM_RIGHTS.

Even then, I think this function should be unsafe, as the control message contains instructions for the kernel. So who knows what it will do (the lifetime will still help prevent wrong usage though).

That's not what the unsafe keyword is for.

Even then, I think this function should be unsafe, as the control message contains instructions for the kernel. So who knows what it will do (the lifetime will still help prevent wrong usage though).

That's not what the unsafe keyword is for.

It is, because we don't know what the kernel will do to our memory and open file descriptors because of those instructions.

If we want to allow third-party code to implement arbitrary SCM_* control message types, then there's no way to prevent the user from manually encoding an SCM_RIGHTS.

Yeah, but we can help them not make mistakes by asking for the correct lifetime. This is also an example why I think the function should be unsafe. Otherwise you can use safe code to let the kernel dup arbitrary numbers as a FD.

Are you sure that you can receive control messages with sendmsg and send them with recvmsg? The C API does technically allow it according to the pointer types, but I can't find it anywhere in documentation.

nix and rustix also don't support it, although of course that doesn't mean it's impossible:
https://docs.rs/nix/latest/nix/sys/socket/fn.sendmsg.html
https://docs.rs/nix/latest/nix/sys/socket/fn.recvmsg.html
https://docs.rs/rustix/latest/rustix/net/fn.sendmsg.html
https://docs.rs/rustix/latest/rustix/net/fn.recvmsg.html

An example of control messages received from a sendmsg() is Linux's socket timestamps: https://www.kernel.org/doc/html/v6.6/networking/timestamping.html

Control messages passed into recvmsg() are sometimes used to configure socket options for that specific call only, though I forget the exact SCM_* type at the moment.

That page says that timestamps are received with a recvmsg() call:

2 Data Interfaces

Timestamps are read using the ancillary data feature of recvmsg(). See man 3 cmsg for details of this interface. The socket manual page (man 7 socket) describes how timestamps generated with SO_TIMESTAMP and SO_TIMESTAMPNS records can be retrieved.

The SO_TIMESTAMP and SO_TIMESTAMPNS options only apply to incoming packets:

O_TIMESTAMP

Generates a timestamp for each incoming packet in (not necessarily monotonic) system time.

Same timestamping mechanism as SO_TIMESTAMP, but reports the timestamp as struct timespec in nsec resolution.

You can enable timestamps through a control message, but only when sending packets:

In addition to socket options, timestamp generation can be requested per write via cmsg, only for SOF_TIMESTAMPING_TX_* (see Section 1.3.1). Using this feature, applications can sample timestamps per sendmsg() without paying the overhead of enabling and disabling timestamps via setsockopt.

This all indicates that you can only send control messages with sendmsg() and you can only receive them with recvmsg().

And if that's true, we can reflect that in the API, which can be simplified a bit.

If you can send ancillary data with recvmsg() and you can receive it with sendmsg(), then I agree one struct for both is much easier. But it seems that you can't, and then splitting the API for the two separate use cases makes things a lot simpler.

Hmm, interesting. I've written software in the past for a platform that modified the sendmsg() control messages buffer to report metadata about outgoing packet processing, but apparently it shouldn't have been doing that.

I'll want to do some more testing just to double-check, but if the read/write stages are strictly separated on all reasonable target platforms then that should let the new API be cleaner.

Thank you for pointing me in the right direction!

de-vri-es · 2023-11-06T12:05:00Z

text/3430-unix-socket-ancillary-data-v2.md

+    ) -> AncillaryData<'a, 'fd>;
+
+    // returns initialized portion of `control_messages_buf`.
+    fn control_messages(&self) -> &ControlMessages;


This function seems a little underpowered. As a low level API I understand something like this is needed, but it does leave the interpretation totally to the user. Even in the case of recognized messages like SCM_RIGHTS.

In tokio_seqpacket I went for an enum holding known types and Other variant for unrecognized messages: https://docs.rs/tokio-seqpacket/latest/tokio_seqpacket/ancillary/enum.AncillaryMessage.html

The downside of that is that it's specific to one type of socket and/or address family.

Maybe there should be an associated type for the socket to advertise what type of control messages are supported. Then the iterator could use that to interpret messages into something more useful.

The current implementation of unix_socket_ancillary_data (in nightly) does use an enum, but as noted in the RFC's first section that enum has turned out to be one of the biggest problems.

The Rust standard library can't have enums with cases that only exist on some platforms, and there's only one control message type specified by POSIX -- SCM_RIGHTS. So you end up with an enum that looks something like this:

pub enum ScmRights<'a> { Borrowed(&'a [BorrowedFd<'a>]), Owned(Vec<OwnedFd>), // heap allocation! } pub struct GenericControlMessage<'a> { ... } pub enum ControlMessage<'a> { ScmRights(ScmRights<'a>), Other(GenericControlMessage<'a>), }

Given that the SCM_RIGHTS is also the only control message type that represents unmanaged resources transferred across the socket boundary, it makes sense to treat it specially inside the library. That leaves enum ControlMessage with only a single case, at which point it can be collapsed.

Maybe there should be an associated type for the socket to advertise what type of control messages are supported. Then the iterator could use that to interpret messages into something more useful.

The maintenance load of cataloging SCM_* types across OSes would be very high, and documentation is thin regarding which control messages are supported by any given socket type. I don't think there's any way we can expect the Rust standard library to provide types for more than a small handful of the most popular ones.

Hmm, I get your point. If the enum is only used for reporting control messages from the kernel, then it wouldn't be too bad if it contains variants that don't exist on all platforms. They would simply never be constructed.

But I do agree that cataloguing all messages is too much, considering all the platform specific differences.

So it probably does make sense to implement a higher level API for end-users on top of a pretty low level API. That could certainly be left for crates.io libraries though.

de-vri-es · 2023-11-06T12:15:35Z

text/3430-unix-socket-ancillary-data-v2.md

+    ) -> Result<(), AncillaryDataNoCapacity>;
+
+    // Transfers ownership of received FDs to the iterator.
+    fn received_fds(&mut self) -> AncillaryDataReceivedFds<'_>;


If I understand correctly you can use either control_messages() or received_fds() to iterate over the received messages (the latter iterating only over FDs and taking ownership of them).

Maybe this should be named take_received_fds() to reflect that it takes ownership?

On a different topic, with this design, this function modifies the received messages but leaves the message in the buffer. So if user code later uses control_messages(), it will contain SCM_RIGHTS message with invalid file descriptors. While -1 is a commonly used sentinel value, it is not one that the kernel will put in an SCM_RIGHTS message.

In toki_seqpacket, I opted to either borrow or consume the control messages with AncillaryMessageReader::messages() or AncillaryMessageReader::into_messages().

That way, you don't need to worry about leaving the SCM_RIGHTS (or others) in an invalid state, because it will not be accessible anymore (except through the original &[u8], but nobody is promising it is a control message at that point).

As a downside, these are two totally separate iterator implementations. Even the yielded items are different (AncillaryMessage<'a> versus OwnedAncillaryMessage<'a>)

Users iterating over the control messages need to be prepared to skip messages they weren't expecting:

for cmsg in ancillary.control_messages() { if let Some(msg) = ScmSomeThing::from_cmsg(cmsg) { ... } if let Some(msg) = ScmOtherThing::from_cmsg(cmsg) { ... } }

So as long as they aren't specifically looking for SCM_RIGHTS, the presence of tombstoned messages in the buffer won't bother them. And since the API for accessing file descriptors is right there in AncillaryData, users have no need to construct their own SCM_RIGHTS handling unless they want something really specific.

de-vri-es · 2023-11-06T12:21:36Z

text/3430-unix-socket-ancillary-data-v2.md

+
+    // Scan the control messages buffer for `SCM_RIGHTS` and take ownership of
+    // any file descriptors found within.
+    unsafe fn take_ownership_of_scm_rights(&mut self);


What exactly will this do?

Set some flag internally to not close file descriptors in drop()? Or replace them all internally with -1?

Since users can still observe the control messages though control_messages(), I think the distinction should be very clear.

Also, if it replaces fds with -1, it is hard to use correctly. You would need to call take_ownership_of_scm_rights() after wrapping the fds in OwnedFd. But if the code panics in between or someone returns an Err, the file descriptor will be closed twice.

If it's a flag it's also difficult to use. You could set it before wrapping the fds in your own OwnedFd, but then you might leak some file descriptors when you panic or exit early with an Err (better than closing FDs twice, but still not great).

It would be much more convenient to take ownership as you're iterating. Then you can panic or return an error without worrying about IO safety.

As described in the comment and elsewhere in the RFC, it takes ownership of the file descriptors in the ancillary data's control messages buffer.

Implementation code: https://github.com/rust-lang/rust/compare/master...jmillikin:upstream__rust:unix-ancillary-data-v2?expand=1#diff-d64b3603bfa3a85d538397a6c7fd860fa3d325c23a6dab5e8e9e818e4c75f466R433-R435

It would be much more convenient to take ownership as you're iterating. Then you can panic or return an error without worrying about IO safety.

Ownership needs to be taken by the AncillaryData before the iteration can start, otherwise iteration would require unsafe fn next(). The place where the encoded RawFds are (logically) transmuted to OwnedFd is here.

de-vri-es · 2023-11-06T12:40:43Z

text/3430-unix-socket-ancillary-data-v2.md

+
+    // Update the control messages buffer length according to the result of
+    // calling `sendmsg()` or `recvmsg()`.
+    fn set_control_messages_len(&mut self, len: usize);


This feels like it should be unsafe. Setting this wrong could cause other code to interpret parts of the buffer as data from the kernel.

There's no way for code to know whether an AncillaryData was populated with data from the kernel, or even whether a given ControlMessage was created via AncillaryData. Code that needs to trust its inputs must verify its call path into sendmsg() / recvmsg().

This particular function is not unsafe because using it incorrectly will not violate any of Rust's memory-safety guarantees.

Hmm, I see, because taking ownership of FDs is delayed until take_ownership_of_scm_rights() is called, and that one is unsafe. I do think it would be nice if take_ownership_of_scm_rights() is not a separate step, but we can discuss that in a different thread.

de-vri-es · 2023-11-06T12:43:28Z

text/3430-unix-socket-ancillary-data-v2.md

+#### `fn AncillaryData::set_control_messages_len`
+
+Does not take ownership of FDs in any `SCM_RIGHTS` control messages that
+might exist within the new buffer length.
+
+**Panics**:
+ * if `len > control_messages_buf.len()`
+ * if `control_messages_buf()` hasn't been called to clear the length.
+
+The second panic condition means that creating an `AncillaryData` and then
+immediately calling `set_control_messages_len` will panic to avoid potentially
+reading uninitialized data.
+
+Also, calling `set_control_messages_len()` twice without an intervening
+`control_messages_buf()` will panic to avoid leaking received FDs.


I don't think panicking prevents leaking file descriptors. A panic could be caught with catch_unwind(). In most async runtimes this is already done for all spawned tasks, so when a background tasks panics, the process keeps running.

I also don't really understand why this doesn't take ownership of the file descriptors. I assume it will set the valid range of the control message, and the drop implementation will close them based on this. Won't it?

I don't think panicking prevents leaking file descriptors. A panic could be caught with catch_unwind(). In most async runtimes this is already done for all spawned tasks, so when a background tasks panics, the process keeps running.

The panic happens before the buffer length is reset. If the panic (assertion) did not happen, then an implementation of SendMessage or RecvMessage that called set_control_messages_len() in the wrong sequence would implicitly leak all owned FDs in the ancillary data, which may be difficult to detect (vs a panic, which is quite easily detectable).

I also don't really understand why this doesn't take ownership of the file descriptors. I assume it will set the valid range of the control message, and the drop implementation will close them based on this. Won't it?

This function is called by both the sendmsg() and recvmsg() code paths. The sendmsg() code path does not, and must not, take ownership of SCM_RIGHTS in its buffer.

de-vri-es · 2023-11-06T12:45:55Z

text/3430-unix-socket-ancillary-data-v2.md

+### `struct AncillaryDataBuf`
+
+An `AncillaryDataBuf` is an owned variant of `AncillaryData`, using heap
+allocation (an internal `Vec<u8>`). It exposes a subset of the `Vec` capacity
+management methods.


Personally I don't really see the need for this type. Ancillary data is in my experience fairly short. Using a fixed size buffer will almost always be good enough.

That said, I don't really mind this type existing. I do wonder if it's worth extending the API surface for this.

It's useful when using file-descriptor passing as part of a larger / more complex API, such as Wayland or D-Bus. Dynamic allocation means the library doesn't need to hardcode the maximum number of FDs it can send/receive (other than the OS limit).

de-vri-es · 2023-11-06T12:47:47Z

text/3430-unix-socket-ancillary-data-v2.md

+    fn new() -> AncillaryDataBuf<'static>;
+    fn with_capacity(capacity: usize) -> AncillaryDataBuf<'static>;


Why does the type have a lifetime if all constructors return a AncillaryDataBuf<'static>?

The lifetime parameter of AncillaryDataBuf is the lifetime of the BorrowedFds that have been passed to it.

But you can't construct one with a non-'static lifetime with the documented interface.

The 'static lifetime is a subtype of all other lifetimes.

fn new_static() -> &'static () { &() } fn use_static() { let t = (); let ts = [&t, new_static()]; }

Maybe it's just a typo? Should new() and with_capacity() return Self instead of AncillaryDataBuf<'static>?

Ah, right. My bad, just looks confusing.

de-vri-es · 2023-11-06T12:52:29Z

text/3430-unix-socket-ancillary-data-v2.md

+long enough to be sent, and (2) received FDs aren't leaked.
+
+```rust
+struct AncillaryData<'a, 'fd>;


Why do these need to be two separate lifetimes? Both the byte buffer and the file descriptors need to be valid while the AncillaryData struct exists. What's the benefit of splitting these?

That was the original design, but the borrow checker complains -- it thinks the ancillary data's buffer remains borrowed for as long as any borrowed files. Giving them separate lifetimes gives the borrow better better visibility into what's actually going on.

If you want, you can clone my branch and change this to AncillaryData<'a> -- the included test should then fail to compile.

de-vri-es · 2023-11-06T12:56:30Z

text/3430-unix-socket-ancillary-data-v2.md

+By default an `AncillaryData` has no received FDs, and this method will
+ignore FDs added via `add_file_descriptors`. The iterator holds a mutable
+borrow on the `AncillaryData`, and will close any unclaimed FDs when
+it's dropped.


Suggested change

By default an `AncillaryData` has no received FDs, and this method will

ignore FDs added via `add_file_descriptors`. The iterator holds a mutable

borrow on the `AncillaryData`, and will close any unclaimed FDs when

it's dropped.

By default an `AncillaryData` has no received FDs, and this method will

ignore FDs added via `add_file_descriptors`. The iterator holds a mutable

borrow on the `AncillaryData`, and will close any unclaimed FDs when

it's dropped.

I understand that the file descriptors added via add_file_descriptors() need to be ignored. They're not owned by the AncillaryData struct. But how does this work internally? Won't both recvmgs() and add_file_descriptors() put a SCM_RIGHTS message in the underlying buffer? How will the implementation distinguish between these SCM_RIGHTS messages?

In tokio_seqpacket, this kind of issue is part of why I went for two separate types: AncillaryMessageReader for receiving, and AncillaryMessageWriter for sending message.

Internally there's a high-watermark that gets set, so the AncillaryData knows which portion of its buffer came from recvmsg() and which portion was added later.

jmillikin · 2023-11-08T13:14:06Z

@de-vri-es Thank you for the detailed review! Many of the questions seem to be about what the implementation might look like, so I put together a quick sketch implementation at https://github.com/rust-lang/rust/compare/master...jmillikin:upstream__rust:unix-ancillary-data-v2?expand=1 -- it's not beautiful code, but it should be enough to get an idea of how things fit together.

I'll respond to the review comments in their threads.

de-vri-es · 2023-11-08T14:23:40Z

It feels like a lot of complexity comes from supporting recvmsg and sendmsg in the same type. What do you think of the approach in tokio_seqpacket with separate reader and writer for ancillary data messages?

jmillikin · 2023-11-08T14:30:34Z

It feels like a lot of complexity comes from supporting recvmsg and sendmsg in the same type. What do you think of the approach in tokio_seqpacket with separate reader and writer for ancillary data messages?

I think I would prefer to avoid making the new API surface even larger with separate send/recv cursors unless every other alternative has been exhausted.

de-vri-es · 2023-11-08T14:40:14Z

I dont disagree with that, but the implementation will be a lot simpler.

The reader can assume all data it is passed is from the kernel, and it can instantly take ownership when it is created (or when you call set_control_message_len() if you want to do that after construction). It's much harder to use wrong.

And the writer doesn't have to worry about owning file descriptors, because it never owns anything.

I also dont think the combined API would necessarily be larger: there's two types instead of one, but the writer doesn't need to allow iterating over added messages and the reader doesn't allow adding messages.

/edit: I'm not the only one who though it was easier to have two types. rustix also has different types for building ancillary messages and for parsing them:
https://docs.rs/rustix/latest/rustix/net/struct.SendAncillaryBuffer.html
https://docs.rs/rustix/latest/rustix/net/struct.RecvAncillaryBuffer.html

joshtriplett · 2023-11-14T16:22:13Z

Thanks for the extensive review, @de-vri-es! Based on consensus in today's @rust-lang/libs-api meeting, I'm un-nominating this for now. Please re-nominate when all the suggestions have either been resolved or have reached a point of dispute that the team needs to look at. You can use rustbot to add the I-libs-api-nominated label.

unix_socket_ancillary_data_v2

fda7369

jmillikin force-pushed the unix-socket-ancillary-data branch from 7885e8b to fda7369 Compare May 10, 2023 08:25

This was referenced May 10, 2023

Tracking Issue for feature(unix_socket_ancillary_data) rust-lang/rust#76915

Open

Tracking Issue for feature(unix_socket_peek) rust-lang/rust#76923

Open

NobodyXu suggested changes May 10, 2023

View reviewed changes

ehuss added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label May 10, 2023

the8472 reviewed May 10, 2023

View reviewed changes

riking reviewed May 15, 2023

View reviewed changes

MaxVerevkin mentioned this pull request Sep 21, 2023

Similar project: railway MaxVerevkin/wayrs#7

Closed

jmillikin mentioned this pull request Oct 20, 2023

ACP: Unix socket ancillary data v2 rust-lang/libs-team#284

Closed

programmerjake reviewed Oct 20, 2023

View reviewed changes

review feedback: fn iter() + impl Extend, other fixes.

5cb2f2e

joshtriplett added the I-libs-api-nominated Indicates that an issue has been nominated for prioritizing at the next libs-api team meeting. label Oct 24, 2023

This was referenced Oct 25, 2023

Convert Unix{Datagram,Stream}::{set_}passcred() to per-OS traits rust-lang/rust#117156

Merged

Add types for inspecting control messages in Unix socket ancillary data rust-lang/rust#117196

Closed

jmillikin force-pushed the unix-socket-ancillary-data branch from b59e6c1 to 713b756 Compare October 26, 2023 02:33

Update ControlMessages* based on implementation PR

d57ff25

jmillikin force-pushed the unix-socket-ancillary-data branch 2 times, most recently from c2bb793 to d57ff25 Compare October 28, 2023 00:24

jmillikin force-pushed the unix-socket-ancillary-data branch 2 times, most recently from 50493d2 to 339b3c7 Compare October 28, 2023 01:59

Rework to AncillaryData<'a> / AncillaryDataBuf for stack alloc.

beffd22

jmillikin force-pushed the unix-socket-ancillary-data branch from 339b3c7 to beffd22 Compare October 28, 2023 02:58

de-vri-es reviewed Nov 6, 2023

View reviewed changes

joshtriplett removed the I-libs-api-nominated Indicates that an issue has been nominated for prioritizing at the next libs-api team meeting. label Nov 14, 2023

jmillikin closed this Jun 9, 2024

jmillikin deleted the unix-socket-ancillary-data branch June 9, 2024 07:11

	struct ControlMessagesBuf;
	struct ControlMessagesBuf<const N: usize>;

		fn start_recvmsg(&mut self) -> Option<&mut [u8]>;
		unsafe fn finish_recvmsg(&mut self, control_messages_len: usize);

		fn new() -> AncillaryDataBuf<'static>;
		fn with_capacity(capacity: usize) -> AncillaryDataBuf<'static>;

RFC: Unix socket ancillary data v2 #3430

RFC: Unix socket ancillary data v2 #3430

Conversation

jmillikin commented May 10, 2023 • edited by kennytm Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmillikin May 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmillikin May 11, 2023 • edited Loading

Choose a reason for hiding this comment

riking May 15, 2023 • 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

Choose a reason for hiding this comment

jmillikin May 16, 2023 • edited Loading

Choose a reason for hiding this comment

jmillikin May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunfishcode commented Oct 20, 2023

jmillikin commented Oct 21, 2023

the8472 commented Oct 21, 2023 • edited Loading

Noah-Kennedy commented Oct 21, 2023

programmerjake commented Oct 24, 2023 • edited Loading

jmillikin commented Oct 24, 2023

jmillikin commented Oct 28, 2023

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 Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

de-vri-es Nov 9, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmillikin Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

de-vri-es Nov 6, 2023 • 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmillikin commented Nov 8, 2023

de-vri-es commented Nov 8, 2023

jmillikin commented Nov 8, 2023

de-vri-es commented Nov 8, 2023 • edited Loading

joshtriplett commented Nov 14, 2023

jmillikin commented May 10, 2023 •

edited by kennytm

Loading

jmillikin May 11, 2023 •

edited

Loading

jmillikin May 11, 2023 •

edited

Loading

riking May 15, 2023 •

edited

Loading

jmillikin May 16, 2023 •

edited

Loading

jmillikin May 16, 2023 •

edited

Loading

the8472 commented Oct 21, 2023 •

edited

Loading

programmerjake commented Oct 24, 2023 •

edited

Loading

de-vri-es Nov 8, 2023 •

edited

Loading

de-vri-es Nov 9, 2023 •

edited

Loading

jmillikin Nov 8, 2023 •

edited

Loading

de-vri-es Nov 6, 2023 •

edited

Loading

de-vri-es commented Nov 8, 2023 •

edited

Loading