rust-lang / rust-lang/libc

Unnecessary `unsafe` for `libc::major` and `libc::minor`? Or is `libc::makedev` missing unsafe?

Open
#3,759 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

C-API-request
Dominant language
Rust
Stars
2.6k
Forks
1.3k
Avg merge
1d 22h
Merged PRs (30d)
69

Description

  • The libc::major and libc::minor functions are unsafe. The reverse operation libc::makedev is not.
  • There is no safety invariants on docs.rs for these, this seems to be the norm for libc, and I can see why (you should refer to the man page for the C safety invariants probably).
  • However, there is nothing in my (Linux Glibc) man page that describes why they would have safety invariants. Nor do the FreeBSD, NetBSD, OpenBSD or Illumos man pages have anything along those lines.
  • Most implementations are in pure rust and use no unsafe operations. The sole exception seems to be Illumos where C functions are being called. Here makedev is also unsafe, unlike other platforms.
  • Illumos can apparently return NODEV, but that is a safe failure (with a corresponding errno being set).

I have not been able to check the man page of every single Unix system, so maybe there is one where this makes sense. Otherwise I think this is an oversight.

I think one or both of the following should be done:

  • If it is not an oversight, since it is safe on the majority of Unices, it would be good to add a documentation string that explains why it is unsafe on some.
  • Also, I would expect some consistency between makedev and major/minor in how unsafe vs not unsafe is handled across platforms.
    • Is the idea that functions should only be unsafe on platforms where a C function is called? Then major/minor are wrong.
    • Is the idea that functions should be unsafe if it they call C functions on any platforms? Then makedev is wrong. I can see the argument to not get lints about unnecessary unsafe blocks, but that only works if it is done consistently.

I also very much doubt there is any Unix where any of these functions could reasonably be unsafe. After all, even the C function will only do some arithmetic.

Minimum working example (not that I think it makes sense here):

/// To be called with value from std::os::unix::fs::MetadataExt::rdev
fn split_to_major_minor(rdev: u64) -> (u64, u64) {
    // SAFETY: I don't think there *actually* are any safety invariants here
    (unsafe { libc::major(rdev) } as u64, unsafe { libc::minor(rdev) } as u64)
}

Target triplet: x86_64-unknown-linux-gnu (and several other ones I checked)
Libc version: 0.2.155

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by comparing the platform implementations and signatures of libc::major, libc::minor, and libc::makedev, including the Illumos variants. Check the relevant Unix man pages and existing safety documentation. Done means determining whether the unsafe annotations are correct and documenting or consistently changing them across supported targets.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
operating-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.