-
Notifications
You must be signed in to change notification settings - Fork 182
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
linux_android: use libc::getrandom
#508
Conversation
52d3e0d
to
c10e466
Compare
let getrandom_fn = unsafe { mem::transmute::<NonNull<c_void>, GetRandomFn>(fptr) }; | ||
let dangling_ptr = ptr::NonNull::dangling().as_ptr(); | ||
// Check that `getrandom` syscall is supported by kernel | ||
let res = unsafe { getrandom_fn(dangling_ptr, 0, 0) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can skip this check and assume that if libc
exposes the getrandom
function, then the syscall is supported by the kernel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, getrandom is documented as returning ENOSYS.
use_file::getrandom_inner(dest) | ||
type GetRandomFn = unsafe extern "C" fn(*mut c_void, libc::size_t, libc::c_uint) -> libc::ssize_t; | ||
|
||
/// Sentinel value which indicates that `libc::getrandom` either not available, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is complicated. Is there really such a big performance penalty for unconditionally calling getrandom each time and then falling back to /dev/urandom when getrandom returns ENOSYS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...or just using an AtomicBool like before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "complicated"? This value is guaranteed to not be a valid function pointer, so we use it as "uninitialized" marker.
dlsym
can be a relatively heavy operation, so I would prefer to cache its result.
With AtomicBool
we would need a separate atomic to store function pointer. Why introduce two atomic loads when one would suffice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I missed the dlsym bit and assumed you were calling libc::getrandom.
Use of
libc::getrandom
will automatically give us optimizations like vDSO (#503) and can help with testing of fallback logic (#289).It was also requested in #285. In that discussion we decided against using this approach, but in the light of the vDSO optimization it may be worth to reconsider it.
In
linux_android_with_fallback
use oflibc::syscall
is replaced bydlsym
-based code similar to what we use in thenetbsd
backend.