Crash on illumos due to different passwd struct definition

[sunshowers]

Describe the bug

Hi there, and thanks for maintaining this library!

I'm trying to use a project that depends on whoami, on illumos, and I'm seeing memory corruption occur due to this.

This appears to be because the passwd struct is different on illumos than on other Unixes -- so the getpwuid_r call corrupts memory.

On illumos, the passwd struct is listed here (search for "passwd Structure"):

struct passwd {
    char *pw_name;      /* user's login name */
    char *pw_passwd;    /* no longer used */
    uid_t pw_uid;       /* user's uid */
    gid_t pw_gid;       /* user's gid */
    char *pw_age;       /* not used */
    char *pw_comment;   /* not used */
    char *pw_gecos;     /* typically user's full name */
    char *pw_dir;       /* user's home dir */
    char *pw_shell;     /* user's login shell */
};

But what whoami uses on illumos is defined here:

#[repr(C)]
struct PassWd {
    pw_name: *const c_void,
    pw_passwd: *const c_void,
    pw_uid: u32,
    pw_gid: u32,
    pw_gecos: *const c_void,
    pw_dir: *const c_void,
    pw_shell: *const c_void,
}

To Reproduce

On an illumos system, run:

#[test]
fn test_names() {
    println!("{:?}", whoami::username());
}

Then, run cargo test --release -- --nocapture.

This fails with:

     Running tests/basic.rs (target/release/deps/basic-3cc3589bf34767da)

running 1 test
"unknown"
error: test failed, to rerun pass `--test basic`

Caused by:
  process didn't exit successfully: `/home/rain/dev/whoami/target/release/deps/basic-3cc3589bf34767da --nocapture` (signal: 6, SIGABRT: process abort signal)

Expected behavior

No crash occurs.

Additional context

It looks like the nix library implements this call safely, using the passwd definition in libc here.

Would it be okay to switch the Unix impl of this library to using nix rather than doing this call by hand? That would make it so that whoami would work on any platforms supported by nix and libc (which is roughly all of them).

I'm happy to do the work here if you're okay with it. Thanks!

[AldaronLau]

@sunshowers Thanks for opening this issue!

Unfortunately, nix has an MSRV of Rust 1.69, which would break whoami's MSRV of Rust 1.40, so it is not an option for this project.

I'd be happy to review and merge a PR if you wanted to do the work fix it.

[sunshowers]

Thanks for the response. Would you be open to changing the MSRV policy at all? The general ecosystem consensus is that changing the MSRV can be done with a minor version bump (so as part of 1.5.0 in whoami's case).

Thee advantage of that would be that this crate has less unsafe code, instead relying on the already-audited unsafe code in nix. To my mind, that advantage far outweighs MSRV considerations, especially against ancient versions of Rust.

This comment on Zulip may be helpful. Assuming that whoami's usage follows the general patterns of overall crate usage, an MSRV of 1.69 would result in well over 90% of overall fetches being satisfied. And it's quite likely that whoami's largest dependencies have a much more modern MSRV. I briefly looked through the top couple of reverse dependencies and found that they don't actually have an MSRV policy at all, meaning they're only guaranteed to build on the latest version of stable Rust.

I've written a series of patches at https://github.com/oxidecomputer/whoami which switches whoami to using nix. This works very well, and massively reduces the amount of unsafe in whoami.

---

Another option is to use the libc crate. Currently, libc 0.2 has a fixed MSRV of 1.13. The MSRV policy for the next version of libc, 1.0, is actively being discussed here.

However, while that would fix the immediate issue, it still results in this crate containing a great deal of unsafe code.

---

In any case, once this issue is addressed we will likely need to put out a RUSTSEC advisory, since this is a stack buffer overflow. I'm happy to work with you on that!

[AldaronLau]

Thanks for the response. Would you be open to changing the MSRV policy at all? The general ecosystem consensus is that changing the MSRV can be done with a minor version bump (so as part of 1.5.0 in whoami's case).

Considering that I have advertized whoami 1.0 to not break MSRV without a major version bump, any MSRV policy changes would have to be accompanied with the 2.0 release. When whoami 1.0 was released, there was not a general ecosystem consensus on when to change MSRV, as advertizing and supporting an MSRV was just becoming popular.

Thee advantage of that would be that this crate has less unsafe code, instead relying on the already-audited unsafe code in nix. To my mind, that advantage far outweighs MSRV considerations, especially against ancient versions of Rust.

This comment on Zulip may be helpful. Assuming that whoami's usage follows the general patterns of overall crate usage, an MSRV of 1.69 would result in well over 90% of overall fetches being satisfied. And it's quite likely that whoami's largest dependencies have a much more modern MSRV. I briefly looked through the top couple of reverse dependencies and found that they don't actually have an MSRV policy at all, meaning they're only guaranteed to build on the latest version of stable Rust.

I've written a series of patches at https://github.com/oxidecomputer/whoami which switches whoami to using nix. This works very well, and massively reduces the amount of unsafe in whoami.

Makes sense to me. I think it's time to create the v2 branch, and if you open a PR I'll review it for inclusion there.

Another option is to use the libc crate. Currently, libc 0.2 has a fixed MSRV of 1.13. The MSRV policy for the next version of libc, 1.0, is actively being discussed here.

However, while that would fix the immediate issue, it still results in this crate containing a great deal of unsafe code.

I think this could be an option for whoami 1.0 release track, depending on what the policy ends up being.

In any case, once this issue is addressed we will likely need to put out a RUSTSEC advisory, since this is a stack buffer overflow. I'm happy to work with you on that!

Help would be appreciated on that, thank you!

[sunshowers]

Considering that I have advertized whoami 1.0 to not break MSRV without a major version bump, any MSRV policy changes would have to be accompanied with the 2.0 release. When whoami 1.0 was released, there was not a general ecosystem consensus on when to change MSRV, as advertizing and supporting an MSRV was just becoming popular.

Sounds good. Happy to put up a PR once the v2 branch is out. (I'd also have a few other suggestions for that branch, such as making the fallible methods the default ones, and maybe removing the infallible ones while providing an easy way to get defaults like unknown).

It would also be great to figure out an MSRV policy as part of the v2 release. What do you think of something like:

• best effort, but will occasionally bump if dependencies change
• a minimum of 6 months of support -- this would easily cross the 90% threshold, based on the data in the Zulip comment. (Rust 1.69 was released on 2023-04-20, so if we bump to that we'll get 10+ months of support.)
• minor version number must be bumped

And once again, thank you for maintaining this crate. As I was telling my coworker, whoami does something nothing else does -- provide a cross-platform wrapper to data that's generally pretty useful!

[AldaronLau]

Created different issues for follow-up work based on the discussion here. Closing the issue, since it's fixed on the v1 branch. Release will be tracked at #87

[AldaronLau]

whoami 1.5.0 just released to crates.io with this fix.