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.

Sign up for GitHub

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

Jump to bottom

Fix Clippy warnings on FreeBSD with the latest nightly #1639

Merged
merged 2 commits into from
Jan 23, 2022
Merged

Fix Clippy warnings on FreeBSD with the latest nightly #1639

merged 2 commits into from
Jan 23, 2022

Conversation

asomers
Copy link
Member

@asomers asomers commented Jan 22, 2022

  • Better type safety for mqueue
  • Suppress clippy::not_unsafe_ptr_arg_deref warnings in ptrace on BSD

@asomers
Better type safety for mqueue
475da53 
On some platforms, mqd_t is a pointer.  That means code like the below
can trigger a segfault.  Fix it by defining a Newtype around mqd_t that
prevents use-after-free and dangling pointer scenarios.

```rust
fn invalid_mqd_t() {
    let mqd: libc::mqd_t = std::ptr::null_mut();
    mq_close(mqd).unwrap();
}
```

Also, get test coverage for mqueue in CI on FreeBSD.
@asomers
Suppress clippy::not_unsafe_ptr_arg_deref warnings in ptrace on BSD
91049bc 
Technically these functions don't violate Rust's safety rules, because
libc::ptrace doesn't dereference those pointer args.  Instead, it passes
them directly to the kernel.
@asomers
Copy link
Member Author

asomers commented Jan 22, 2022

Supersedes #1638

@rtzoeller
Copy link
Collaborator

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 23, 2022

Build succeeded:

@bors bors bot merged commit 0c02ee2 into nix-rust:master Jan 23, 2022
@lucab lucab mentioned this pull request Feb 23, 2022
Closed
@lucab
Copy link
Contributor

lucab commented Feb 28, 2022

POSIX does explicitly mention that A message queue descriptor may be implemented using a file descriptor, which I think is the case at least on Linux.
Before this, it used to be possible to check an existing open file-descriptor via getattr and then convert it to a message queue descriptor. Would it be possible to get that functionality back, on specific platforms? How should that look like?

@lucab lucab mentioned this pull request Feb 28, 2022
Merged
@asomers asomers deleted the mq-safety branch February 28, 2022 16:43
@asomers
Copy link
Member Author

asomers commented Feb 28, 2022

@lucab could you give an example in Rust, C, or pseudocode? How would that feature work?

@lucab
Copy link
Contributor

lucab commented Feb 28, 2022

Sure, and thanks for the prompt reply!
The initial context here is systemd, which is a Linux-only service manager.
It supports listening on a message queue (or socket/FIFO/etc.) on behalf of a service and activating it on demand, see https://www.freedesktop.org/software/systemd/man/systemd.socket.html#ListenMessageQueue=.
The activation mechanism works in a way such that systemd performs the mq_open(), then later passes the open file-descriptor to the spawned service/process.
The spawned process here would be a Rust binary, and we have a library method is_mq() to check the inherited FD. It is built on top of the previous design of nix::mqueue::mq_getattr(), see https://docs.rs/libsystemd/latest/libsystemd/activation/trait.IsType.html#method.is_mq.
Overall I'm not particularly attached to the existing code/design, but I feel that we are now missing primitives for fallible conversion between a file-descriptor and a mqueue-descriptor.

@rtzoeller
Copy link
Collaborator

I feel that we are now missing primitives for fallible conversion between a file-descriptor and a mqueue-descriptor.

I think it's worth noting that the conversion itself was infallible. You could end up with an invalid mqueue-descriptor, as libsystemd handled, but the conversion still succeeded. I don't think nix can make the conversion fallible without performing a similar syscall, and don't think it needs to.

std::os::unix::io::FromRawFd is surprising unhelpful for you, as it takes ownership and is_mq() takes an immutable reference. Consequently I think we'll need to implement at least std::convert::From.

Implementing std::os::unix::io::AsRawFd and std::os::unix::io::IntoRawFd gets you from an MqdT to a RawFd (which isn't what you need), but IMO is worth doing for symmetry.

We should probably implement all four traits on Linux and any other platforms that use FDs instead of pointers for mqd_t, unless @asomers sees a reason not to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rtzoeller rtzoeller Awaiting requested review from rtzoeller

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@asomers @rtzoeller @lucab