Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions src/use_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use core::{
/// - On Redox, only /dev/urandom is provided.
/// - On AIX, /dev/urandom will "provide cryptographically secure output".
/// - On Haiku and QNX Neutrino they are identical.
const FILE_PATH: &str = "/dev/urandom\0";
const FILE_PATH: &[u8] = b"/dev/urandom\0";
const FD_UNINIT: usize = usize::max_value();

pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
Expand Down Expand Up @@ -57,7 +57,7 @@ fn get_rng_fd() -> Result<libc::c_int, Error> {
#[cfg(any(target_os = "android", target_os = "linux"))]
wait_until_rng_ready()?;

let fd = unsafe { open_readonly(FILE_PATH)? };
let fd = open_readonly(FILE_PATH)?;
// The fd always fits in a usize without conflicting with FD_UNINIT.
debug_assert!(fd >= 0 && (fd as usize) < FD_UNINIT);
FD.store(fd as usize, Relaxed);
Expand All @@ -69,7 +69,7 @@ fn get_rng_fd() -> Result<libc::c_int, Error> {
#[cfg(any(target_os = "android", target_os = "linux"))]
fn wait_until_rng_ready() -> Result<(), Error> {
// Poll /dev/random to make sure it is ok to read from /dev/urandom.
let fd = unsafe { open_readonly("/dev/random\0")? };
let fd = open_readonly(b"/dev/random\0")?;
let mut pfd = libc::pollfd {
fd,
events: libc::POLLIN,
Expand Down
22 changes: 15 additions & 7 deletions src/util_libc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,14 +70,22 @@ pub fn sys_fill_exact(
Ok(())
}

// SAFETY: path must be null terminated, FD must be manually closed.
pub unsafe fn open_readonly(path: &str) -> Result<libc::c_int, Error> {
debug_assert_eq!(path.as_bytes().last(), Some(&0));
/// Open a file in read-only mode.
///
/// # Panics
/// If `path` does not contain any zeros.
// TODO: Move `path` to `CStr` and use `CStr::from_bytes_until_nul` (MSRV 1.69)
// or C-string literals (MSRV 1.77) for statics
#[inline(always)]
pub fn open_readonly(path: &[u8]) -> Result<libc::c_int, Error> {
assert!(path.iter().any(|&b| b == 0));
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, but since this already returns a Result, we should avoid assert!. Either the compiler is going to optimize away the check completely anyway, or we'd be left with dead code that constructs an Err, or we'd be left with dead code that panics. The possibility of dead code that panics is problematic for some static analysis mechanisms that try to ensure there are no panics in a program.

Copy link
Member

Choose a reason for hiding this comment

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

I think having an assert! here is fine. If this ever triggers, it's an error in our code, not an error in the user's calling environment or input.

For opt-level=1 (https://rust.godbolt.org/z/K3cjrsxqa) or opt-level=s (https://rust.godbolt.org/z/MT9jozE1e) , the assert is optimized away, and on opt-level=0 there's a bunch of dead code that panics, even if the assert is replaced with a branch(https://rust.godbolt.org/z/939exhnKn). Leaving it as-is also makes things more readable/debuggable.

A better approach for this is documenting and testing that our implementation does not panic, I opened #435.

loop {
let fd = libc::open(
path.as_ptr().cast::<libc::c_char>(),
libc::O_RDONLY | libc::O_CLOEXEC,
);
let fd = unsafe {
libc::open(
path.as_ptr().cast::<libc::c_char>(),
libc::O_RDONLY | libc::O_CLOEXEC,
)
};
if fd >= 0 {
return Ok(fd);
}
Expand Down