Improve the memory safety guarantees around open_readonly.

Do more complete NUL termination checking, at compile-time. Remove the
`unsafe` from the function as it is now memory-safe.

Author: briansmith

src/use_file.rs

@@ -1,6 +1,6 @@
 //! Implementations that just need to read from a file
 use crate::{
-    util_libc::{open_readonly, sys_fill_exact},
+    util_libc::{cstr, open_readonly, sys_fill_exact},
     Error,
 };
 use core::{
@@ -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: cstr::Ref = cstr::unwrap_const_from_bytes_with_nul(b"/dev/urandom\0");
 const FD_UNINIT: usize = usize::max_value();
 
 pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
@@ -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);
@@ -68,8 +68,9 @@ fn get_rng_fd() -> Result<libc::c_int, Error> {
 // Succeeds once /dev/urandom is safe to read from
 #[cfg(any(target_os = "android", target_os = "linux"))]
 fn wait_until_rng_ready() -> Result<(), Error> {
+    const DEV_RANDOM: cstr::Ref = cstr::unwrap_const_from_bytes_with_nul(b"/dev/random\0");
     // 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(DEV_RANDOM)?;
     let mut pfd = libc::pollfd {
         fd,
         events: libc::POLLIN,

src/util_cstr.rs

@@ -0,0 +1,77 @@
+//! Work around lack of `core::ffi::CStr` prior to Rust 1.64, and the lack of
+//! `const fn` support for `CStr` in later versions.
+
+// TODO(MSRV 1.64): Use `core::ffi::c_char`.
+use libc::c_char;
+
+// TODO(MSRV 1.64): Replace with `&core::ffi::CStr`.
+pub struct Ref(&'static [u8]);
+
+impl Ref {
+    #[inline(always)]
+    pub fn as_ptr(&self) -> *const c_char {
+        const _SAME_ALIGNMENT: () =
+            assert!(core::mem::align_of::<u8>() == core::mem::align_of::<c_char>());
+        const _SAME_SIZE: () =
+            assert!(core::mem::size_of::<u8>() == core::mem::size_of::<c_char>());
+
+        // It is safe to cast a `*const u8` to a `const c_char` as they are the
+        // same size and alignment.
+        self.0.as_ptr().cast()
+    }
+
+    // SAFETY: Same as `CStr::from_bytes_with_nul_unchecked`.
+    const unsafe fn from_bytes_with_nul_unchecked(value: &'static [u8]) -> Self {
+        Self(value)
+    }
+}
+
+pub const fn unwrap_const_from_bytes_with_nul(value: &'static [u8]) -> Ref {
+    // XXX: We cannot use `unwrap_const` since `Ref`/`CStr` is not `Copy`.
+    match const_from_bytes_with_nul(value) {
+        Some(r) => r,
+        None => panic!("const_from_bytes_with_nul failed"),
+    }
+}
+
+// TODO(MSRV 1.72): Replace with `CStr::from_bytes_with_nul`.
+#[inline(always)]
+const fn const_from_bytes_with_nul(value: &'static [u8]) -> Option<Ref> {
+    const fn const_contains(mut value: &[u8], needle: &u8) -> bool {
+        while let [head, tail @ ..] = value {
+            if *head == *needle {
+                return true;
+            }
+            value = tail;
+        }
+        false
+    }
+
+    // TODO(MSRV 1.69): Use `core::ffi::CStr::from_bytes_until_nul`
+    match value {
+        [before_nul @ .., 0] if !const_contains(before_nul, &0) => {
+            // SAFETY:
+            //   * `value` is nul-terminated according to the slice pattern.
+            //   * `value` doesn't contain any interior null, by the guard.
+            // TODO(MSRV 1.64): Use `CStr::from_bytes_with_nul_unchecked`
+            Some(unsafe { Ref::from_bytes_with_nul_unchecked(value) })
+        }
+        _ => None,
+    }
+}
+
+mod tests {
+    use super::const_from_bytes_with_nul;
+
+    // Bad.
+    const _EMPTY_UNTERMINATED: () = assert!(const_from_bytes_with_nul(b"").is_none());
+    const _EMPTY_DOUBLE_TERMINATED: () = assert!(const_from_bytes_with_nul(b"\0\0").is_none());
+    const _DOUBLE_NUL: () = assert!(const_from_bytes_with_nul(b"\0\0").is_none());
+    const _LEADINGL_NUL: () = assert!(const_from_bytes_with_nul(b"\0a\0").is_none());
+    const _INTERNAL_NUL_UNTERMINATED: () = assert!(const_from_bytes_with_nul(b"\0a").is_none());
+
+    // Good.
+    const EMPTY_TERMINATED: () = assert!(const_from_bytes_with_nul(b"\0").is_some());
+    const _NONEMPTY: () = assert!(const_from_bytes_with_nul(b"asdf\0").is_some());
+    const _1_CHAR: () = assert!(const_from_bytes_with_nul(b"a\0").is_some());
+}

src/util_libc.rs

@@ -8,6 +8,9 @@ use core::{
 };
 use libc::c_void;
 
+#[path = "util_cstr.rs"]
+pub(crate) mod cstr;
+
 cfg_if! {
     if #[cfg(any(target_os = "netbsd", target_os = "openbsd", target_os = "android"))] {
         use libc::__errno as errno_location;
@@ -133,15 +136,11 @@ impl Weak {
     }
 }
 
-// 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));
+// Memory leak hazard: The returned file descriptor must be manually closed to
+// avoid a file descriptor leak.
+pub fn open_readonly(path: cstr::Ref) -> Result<libc::c_int, Error> {
     loop {
-        // XXX/FIXME: Unchecked UTF-8-to-c_char cast.
-        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(), libc::O_RDONLY | libc::O_CLOEXEC) };
         if fd >= 0 {
             return Ok(fd);
         }