search

ardaku / whoami

Rust crate to get the current user and environment.
Apache License 2.0
195 stars 31 forks source link

Fix Instances of Memory Corruption on Illumos #93

Closed AldaronLau closed 8 months ago

AldaronLau commented 8 months ago

In the future it would be nice to use libc (once an MSRV policy is decided on that's compatible with whoami 1.0's MSRV guarantees) or nix (in whoami 2.1), but for now doing a minimal patch.

Also, supporting Illumos as a tested target.

AldaronLau commented 8 months ago

Should fix https://github.com/ardaku/whoami/issues/91

sunshowers commented 8 months ago

Thanks!

Just a couple of comments:

  1. I don't think this catch-all should exist: https://github.com/ardaku/whoami/blob/d6ee13ed9e818aa51b8d86d95e8009a376289a40/src/os/unix.rs#L26-L34. Instead, if a platform isn't known then it should fail to compile.
  2. I think you can use libc 0.2 -- the MSRV for that won't change. It's just the MSRV for libc 1.0 that will change. If you do that, 1 is moot -- since you'll be pulling in all the definitions from libc.

Also, could you rename Illumos to illumos? It's always spelled with a lowercase "i" to avoid confusion between uppercase I and lowercase l.

AldaronLau commented 8 months ago
  1. I don't think this catch-all should exist: https://github.com/ardaku/whoami/blob/d6ee13ed9e818aa51b8d86d95e8009a376289a40/src/os/unix.rs#L26-L34 . Instead, if a platform isn't known then it should fail to compile.

Yeah, I agree this should be reworked. I'll work on switching it for whoami 1.5.0, aiming to get that out today.

2. I think you can use libc 0.2 -- the MSRV for that won't change. It's just the MSRV for libc 1.0 that will change. If you do that, 1 is moot -- since you'll be pulling in all the definitions from libc.

This should be fine, will aim to get this change in for 1.6.0 since there's already a lot going into 1.5.0.

Also, could you rename Illumos to illumos? It's always spelled with a lowercase "i" to avoid confusion between uppercase I and lowercase l.

Where exactly should it be renamed? The platform enum should stay Illumos to match Rust casing guidelines. Just the README and Display implementation on Platform?

sunshowers commented 8 months ago

Yeah, readme, doc comments and Display impl. Thank you!