use thiserror's #[from] attr and remove Error::NixError

[MaxVerevkin]

No description provided.

[MaxVerevkin]

I tried to change MarshalError::DupUnixFd(nix::Error) to io::Error, but io::Error does not implement Eq.

[KillingSpark]

I tried to change MarshalError::DupUnixFd(nix::Error) to io::Error, but io::Error does not implement Eq.

You could try std::io::ErrorKind that's a simple enum and should be pretty close to nix::Error which is a wrapper around the unixy errno

[MaxVerevkin]

You could try std::io::ErrorKind

Good idea, that works.

By the way, what do you think about using std's Arc<OwnedFd> instead of custom UnixFd?

[KillingSpark]

Good idea, that works.

Great :)

By the way, what do you think about using std's Arc instead of custom UnixFd?

Replacing the InnerUnixFd with OwnedFd could be nice. But: I think the take API on the UnixFd is important. DBus sadly necessitates an API like this because theoretically the same FD could be referenced multiple times in one message, the user needs to decide how to handle this fact. I don't know if it's worth to replace that hidden implementation detail.

One thing that could definitely be improved though is to return OwnedFd from take_raw_fd and dup as well as a BorrowedFd from the get_raw_fd

[MaxVerevkin]

One thing that could definitely be improved though is to return OwnedFd from take_raw_fd and dup as well as a BorrowedFd from the get_raw_fd

That would definitely be an improvement. But UnixFd::new would need to be marked as unsafe, with the same safety requirements as OwnedFd::from_raw_fd, right?

the same FD could be referenced multiple times in one message

Didn't know that!

[KillingSpark]

That would definitely be an improvement. But UnixFd::new would need to be marked as unsafe, with the same safety requirements as OwnedFd::from_raw_fd, right?

Damn that's true and we can't guarantee those requirements. The party sending the message can attach just about anything to the message. The current implementation might even be unsound. Although I don't know how I would improve this. Are there resources that need more cleanup than a close() on the filedescriptor?

Didn't know that!

What DBus does is a bit sneaky. The filedescriptors get passed in a separate array and the actual values in the encoded message are just indices into this array. That's because the kernel assignes new filedescriptors in transit to the new process. (The whole shebang around cmessages is very weird and messy...). But this means that a filedescriptor can be "referenced" very often. And I don't think the spec forbids that.

[KillingSpark]

I think even without changing anything else the UnixFd::new needs to be marked unsafe tbh

[MaxVerevkin]

The current implementation might even be unsound

I don't think it is, since there are no OwnedFd involved, and closing incoming fds with close() is the only thing that can be done.

Are there resources that need more cleanup than a close() on the filedescriptor?

I can think of libseat_open_device from libseat, which gives an (fd, id) pair. libseat should be notified before the fd is closed. But not doing so is libseat user's bug, not rustbus'...

What DBus does is a bit sneaky. The filedescriptors get passed in a separate array and the actual values in the encoded message are just indices into this array. That's because the kernel assignes new filedescriptors in transit to the new process. (The whole shebang around cmessages is very weird and messy...). But this means that a filedescriptor can be "referenced" very often. And I don't think the spec forbids that.

I'm familiar with this "out-of-band-fds-thing" from wayland, but luckily there fds are associated with args in order, so each argument gets exactly one or zero fds. And messages may contain OwnedFds directly, like here.

[KillingSpark]

I can think of libseat_open_device from libseat, which gives an (fd, id) pair. libseat should be notified before the fd is closed. But not doing so is libseat user's bug, not rustbus'...

The issue I see here is this: Assume we receive a message with such an filedescriptor, the upper layer does not understand the message (for whatever reason, maybe the path the message was sent to isn't actually registered by the service, etc etc) then rustbus will eventually close the fd without correctly cleaning up. This could lead to ressources being accumulated at a service because of a buggy or malicious client. But I also don't see how we would do anything about this. Closing is a best-effort cleanup. But this seems to suggest to me that we shouldn't pass out OwnedFd and BorrowedFd because of their additional guarantees?

[KillingSpark]

I'm familiar with this "out-of-band-fds-thing" from wayland, but luckily there fds are associated with args in order, so each argument gets exactly one or zero fds. And messages may contain OwnedFds directly, like here.

That seems like a good lesson learned from dbus. Good job wayland devs :) I'm guessing that the OwnedFd is fine(-ish?) because it's at least supposed to be pointing to a keymap which supposedly only needs a close() for cleanup. The Filedescriptors we deal with here are a lot more general in nature.

[MaxVerevkin]

BorrowedFd does not have additional guaranties, so it should be fine.

I'm guessing that the OwnedFd is fine(-ish?) because it's at least supposed to be pointing to a keymap

In this case absolutely, but a protocol extension may use fds for whatever purpose. Maybe even to send libseat devices, which is the case with wp_drm_lease_v1!

Still, zvariant, wayland-client and wayrs-client use OwnedFd...

[MaxVerevkin]

Wait, BorrowedFd::borrow_raw's safety section does not mention anything about cleanup, but it allows to safely create OwnedFd with BorrowedFd::try_clone_to_owned.

[KillingSpark]

Still, zvariant, wayland-client and wayrs-client use OwnedFd...

Can't speak for the wayland crates but in case of zvariant I am pretty sure that it's a bug to do so.

Wait, BorrowedFd::borrow_raw's safety section does not mention anything about cleanup, but it allows to safely create OwnedFd with BorrowedFd::try_clone_to_owned.

Huh that is probably just an oversight in the docs. They don't expect you to create an BorrowedFd from anything else than an OwnedFd. Probably worth opening a PR/issue in the rust project though.

[MaxVerevkin]

Probably worth opening a PR/issue in the rust project though.

rust-lang/rust#121335

[KillingSpark]

So I think the discussion around the FD representation warrants a new issue/PR. Aside from that, do you want to change anything in this PR? Otherwise I'd go ahead and review/merge this in its current state.

[MaxVerevkin]

There are still places where nix is used in the public API, but I think I will finish this in a later PR.