Skip to content

linux_android_with_fallback: do not use dlsym on MUSL targets #602

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

Merged
merged 3 commits into from
Feb 12, 2025
Merged

linux_android_with_fallback: do not use dlsym on MUSL targets #602

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c46936
linux_android_with_fallback: do not use dlsym on MUSL targets
newpavlov Feb 5, 2025
60f2393
Add `#[cold]` to `use_file_fallback`
newpavlov Feb 5, 2025
3f2d0e5
Revert addition of `#[cold]`
newpavlov Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions src/backends/linux_android_with_fallback.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ use super::use_file;
use crate::Error;
use core::{
ffi::c_void,
mem::{self, MaybeUninit},
ptr::{self, NonNull},
mem::{transmute, MaybeUninit},
ptr::NonNull,
sync::atomic::{AtomicPtr, Ordering},
};
use use_file::util_libc;
Expand All @@ -17,18 +17,28 @@ type GetRandomFn = unsafe extern "C" fn(*mut c_void, libc::size_t, libc::c_uint)
/// or not supported by kernel.
const NOT_AVAILABLE: NonNull<c_void> = unsafe { NonNull::new_unchecked(usize::MAX as *mut c_void) };

static GETRANDOM_FN: AtomicPtr<c_void> = AtomicPtr::new(ptr::null_mut());
static GETRANDOM_FN: AtomicPtr<c_void> = AtomicPtr::new(core::ptr::null_mut());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we avoid this noise in the diff? doesn't seem related to the fix being made.


#[cold]
#[inline(never)]
fn init() -> NonNull<c_void> {
static NAME: &[u8] = b"getrandom\0";
let name_ptr = NAME.as_ptr().cast::<libc::c_char>();
let raw_ptr = unsafe { libc::dlsym(libc::RTLD_DEFAULT, name_ptr) };
// Use static linking to `libc::getrandom` on MUSL targets and `dlsym` everywhere else
#[cfg(not(target_env = "musl"))]
let raw_ptr = {
static NAME: &[u8] = b"getrandom\0";
let name_ptr = NAME.as_ptr().cast::<libc::c_char>();
unsafe { libc::dlsym(libc::RTLD_DEFAULT, name_ptr) }
};
#[cfg(target_env = "musl")]
let raw_ptr = {
let fptr: GetRandomFn = libc::getrandom;
unsafe { transmute::<GetRandomFn, *mut c_void>(fptr) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cast to a pointer and back smells like an indirect call.

Copy link
Member Author

@newpavlov newpavlov Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is effectively no overhead to this approach. "Direct" call to libc::getradom compiles down to:

xor     edx, edx
jmp     qword ptr [rip + getrandom@GOTPCREL]

While the "indirect" call compiles to:

mov     rax, qword ptr [rip + GETRANDOM_FN]
xor     edx, edx
jmp     rax

Yes, the former is a bit friendlier to CPUs, but compared to the syscall cost the difference is negligible. If you care about this difference, then you should use the linux_getrandom opt-in backend.

};

let res_ptr = match NonNull::new(raw_ptr) {
Some(fptr) => {
let getrandom_fn = unsafe { mem::transmute::<NonNull<c_void>, GetRandomFn>(fptr) };
let dangling_ptr = ptr::NonNull::dangling().as_ptr();
let getrandom_fn = unsafe { transmute::<NonNull<c_void>, GetRandomFn>(fptr) };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the diff on these 2 lines is also not necessary.

let dangling_ptr = NonNull::dangling().as_ptr();
// Check that `getrandom` syscall is supported by kernel
let res = unsafe { getrandom_fn(dangling_ptr, 0, 0) };
if cfg!(getrandom_test_linux_fallback) {
Expand All @@ -54,7 +64,7 @@ fn init() -> NonNull<c_void> {
res_ptr
}

// prevent inlining of the fallback implementation
// Prevent inlining of the fallback implementation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessary

#[inline(never)]
fn use_file_fallback(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
use_file::fill_inner(dest)
Expand All @@ -78,7 +88,7 @@ pub fn fill_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
use_file_fallback(dest)
} else {
// note: `transmute` is currently the only way to convert a pointer into a function reference
let getrandom_fn = unsafe { mem::transmute::<NonNull<c_void>, GetRandomFn>(fptr) };
let getrandom_fn = unsafe { transmute::<NonNull<c_void>, GetRandomFn>(fptr) };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessary

util_libc::sys_fill_exact(dest, |buf| unsafe {
getrandom_fn(buf.as_mut_ptr().cast(), buf.len(), 0)
})
Expand Down