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.

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

check libc before using SYS_getrandom on Linux #285

Open
ahgamut opened this issue Oct 4, 2022 · 5 comments
Open

check libc before using SYS_getrandom on Linux #285

ahgamut opened this issue Oct 4, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@ahgamut
Copy link

ahgamut commented Oct 4, 2022

Hi,

I was porting HVM to run on Cosmopolitan Libc, and I noticed a small difference between how getrandom is handled by this crate when compared to the Rust standard library.

The Rust standard library has a syscall macro which creates a weak symbol for getrandom, so if the underlying libc provides getrandom, that function is called instead of making a syscall. The standard library uses getrandom for HashMap keys here.

The getrandom crate makes the Linux syscall directly without first checking for getrandom in the libc.

The pattern used in the Rust standard library is flexible to which libc is used, be it glibc, musl, or Cosmopolitan.

Could you change https://github.com/rust-random/getrandom/blob/master/src/linux_android.rs to check if getrandom is provided by the libc first, before using the syscall?

@josephlr
Copy link
Member

josephlr commented Oct 4, 2022

Good catch, this library aims to have an identical implementation to the standard library (see rust-lang/rust#62079), so we shouldn't deviate unless it's necessary. I think adding this sort of detection would be fine. Our logic would then become:

  • Use libc::getrandom if it exists (requires glibc 2.25 or musl 1.1.20)
  • Use raw syscall if it exists (requires Linux 3.17 or a backported kernel like RHEL 7)
  • Use the /dev/urandom file as the final fallback.

@newpavlov
Copy link
Member

newpavlov commented Oct 4, 2022

Personally, on Linux I prefer using syscalls directly, to the point of thinking that ideally we should not rely on libc::syscall and instead use something like the syscall crate (it's a real shame in my opinion that we do not have a libc-free std/target).

Regardless of the approach used by std, what are the practical motivations for prioritizing libc::getrandom over libc::syscall + libc::SYS_getrandom? On top of my head I can think of the following:

  • Support of libc::getrandom on pre-3.17 kernels. IIUC it would simply mean that libc has a similar code to ours for working with /dev/urandom, meaning all the issues with using FD and polling /dev/random. I would prefer to have such code on the Rust side, not in libc.
  • Allow interception of getrandom calls. If you are certain that you absolutely need it for some reason, it still should be possible with the libc::syscall-based code. We generally discourage replacing of OS entropy source with something custom.

Assuming we will add the libc::getrandom backend, do we want to keep the libc::syscall-based one?

@briansmith
Copy link
Contributor

The Rust standard library has a syscall macro which creates a weak symbol for getrandom, so if the underlying libc provides getrandom, that function is called instead of making a syscall.

How does the Rust stdlib ensure that the getrandom it finds is actually provided by libc and not by any other library? The getrandom crate intentionally tries to prevent other libraries from overriding the operating system implementation with their own custom implementation, when there is an operating system implementation.

Good catch, this library aims to have an identical implementation to the standard library (see rust-lang/rust#62079), so we shouldn't deviate unless it's necessary.

Reasonably policy. But, let's make sure that the standard library is doing the right thing.

@briansmith
Copy link
Contributor

How does the Rust stdlib ensure that the getrandom it finds is actually provided by libc and not by any other library?

The getrandom crate presently already does this weak symbol lookup for macOS, FreeBSD, Dragonfly, Solaris, and BSDs. It seems to be a bit at odds with the hoops that we go through to make custom implementations only work on platforms that getrandom doesn't already explicitly support.

I am not sure how we test that both code paths (weak symbol found, weak symbol not found and fallback to syscall) are correct in CI/CD.

@josephlr josephlr added the enhancement New feature or request label Oct 23, 2022
@josephlr
Copy link
Member

josephlr commented May 7, 2024

@ahgamut after looking at this again, I think the correct approach is to not compile for a Linux target unless you are actually targeting Linux. Given Cosmopolitan's goals of being a compile-once/run-everywhere libc, it shouldn't be using the x86_64-unknown-linux-gnu target (which is specifically Linux-only). If rust supported a x86_64-unknown-unknown-cosmo unix-like target, that would probably be the best bet. Then, we could just add target_env = "cosmo" to our list of targets which just call libc::getrandom.

Given that the Linux fallback logic is already fairly complex, I don't think it makes sense to make it more complex.

An alternative would be to only use the syscall wrapper on Linux targets which lack libc::getrandom, but again the splitting of the implementation is probably not worth it as most Linux targets have a minimum glibc of 2.17 (libc::getrandom was introduced in glibc 2.25).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants