diff src/libpam/memory.rs @ 93:efc2b56c8928

Remove undefined behavior per MIRI. This replaces a bunch of raw pointers with NonNull and removes all the undefined behavior that we can find with MIRI. We also remove the `SecureString` dependency (since it doesn't work with MIRI, and because it's not really necessary).
author Paul Fisher <paul@pfish.zone>
date Mon, 23 Jun 2025 13:02:58 -0400
parents 5aa1a010f1e8
children 51c9d7e8261a
line wrap: on
line diff
--- a/src/libpam/memory.rs	Sun Jun 22 19:29:32 2025 -0400
+++ b/src/libpam/memory.rs	Mon Jun 23 13:02:58 2025 -0400
@@ -4,7 +4,9 @@
 use crate::{BinaryData, ErrorCode};
 use std::ffi::{c_char, CStr, CString};
 use std::marker::{PhantomData, PhantomPinned};
-use std::{ptr, slice};
+use std::mem::offset_of;
+use std::ptr::NonNull;
+use std::{mem, ptr, slice};
 
 /// Allocates `count` elements to hold `T`.
 #[inline]
@@ -50,30 +52,22 @@
 /// # Safety
 ///
 /// It's on you to provide a valid string.
-pub unsafe fn copy_pam_string(result_ptr: *const libc::c_char) -> Result<String> {
-    // We really shouldn't get a null pointer back here, but if we do, return nothing.
-    if result_ptr.is_null() {
-        return Ok(String::new());
-    }
-    let bytes = unsafe { CStr::from_ptr(result_ptr) };
-    bytes
-        .to_str()
+pub unsafe fn copy_pam_string(result_ptr: *const c_char) -> Result<String> {
+    Ok(wrap_string(result_ptr)?
         .map(String::from)
-        .map_err(|_| ErrorCode::ConversationError)
+        .unwrap_or_default())
 }
 
 /// Wraps a string returned from PAM as an `Option<&str>`.
-pub unsafe fn wrap_string<'a>(data: *const libc::c_char) -> Result<Option<&'a str>> {
-    let ret = if data.is_null() {
-        None
-    } else {
-        Some(
-            CStr::from_ptr(data)
+pub unsafe fn wrap_string<'a>(data: *const c_char) -> Result<Option<&'a str>> {
+    match NonNull::new(data.cast_mut()) {
+        Some(data) => Ok(Some(
+            CStr::from_ptr(data.as_ptr())
                 .to_str()
                 .map_err(|_| ErrorCode::ConversationError)?,
-        )
-    };
-    Ok(ret)
+        )),
+        None => Ok(None),
+    }
 }
 
 /// Allocates a string with the given contents on the C heap.
@@ -82,7 +76,7 @@
 ///
 /// - it allocates data on the C heap with [`libc::malloc`].
 /// - it doesn't take ownership of the data passed in.
-pub fn malloc_str(text: &str) -> Result<*mut c_char> {
+pub fn malloc_str(text: &str) -> Result<NonNull<c_char>> {
     let data = text.as_bytes();
     if data.contains(&0) {
         return Err(ErrorCode::ConversationError);
@@ -92,8 +86,8 @@
     // SAFETY: we just allocated this and we have enough room.
     unsafe {
         libc::memcpy(data_alloc.cast(), data.as_ptr().cast(), data.len());
+        Ok(NonNull::new_unchecked(data_alloc))
     }
-    Ok(data_alloc)
 }
 
 /// Writes zeroes over the contents of a C string.
@@ -105,7 +99,10 @@
 /// It's up to you to provide a valid C string.
 pub unsafe fn zero_c_string(cstr: *mut c_char) {
     if !cstr.is_null() {
-        libc::memset(cstr.cast(), 0, libc::strlen(cstr.cast()));
+        let len = libc::strlen(cstr.cast());
+        for x in 0..len {
+            ptr::write_volatile(cstr.byte_offset(x as isize), mem::zeroed())
+        }
     }
 }
 
@@ -128,17 +125,21 @@
 
 impl CBinaryData {
     /// Copies the given data to a new BinaryData on the heap.
-    pub fn alloc((data, data_type): (&[u8], u8)) -> Result<*mut CBinaryData> {
+    pub fn alloc((data, data_type): (&[u8], u8)) -> Result<NonNull<CBinaryData>> {
         let buffer_size =
             u32::try_from(data.len() + 5).map_err(|_| ErrorCode::ConversationError)?;
         // SAFETY: We're only allocating here.
         let dest = unsafe {
-            let dest_buffer: *mut CBinaryData = calloc::<u8>(buffer_size as usize).cast();
-            let dest = &mut *dest_buffer;
+            let mut dest_buffer: NonNull<Self> =
+                NonNull::new_unchecked(calloc::<u8>(buffer_size as usize).cast());
+            let dest = dest_buffer.as_mut();
             dest.total_length = buffer_size.to_be_bytes();
             dest.data_type = data_type;
-            let dest = dest.data.as_mut_ptr();
-            libc::memcpy(dest.cast(), data.as_ptr().cast(), data.len());
+            libc::memcpy(
+                Self::data_ptr(dest_buffer).cast(),
+                data.as_ptr().cast(),
+                data.len(),
+            );
             dest_buffer
         };
         Ok(dest)
@@ -148,39 +149,33 @@
         u32::from_be_bytes(self.total_length).saturating_sub(5) as usize
     }
 
-    /// Clears this data and frees it.
-    pub unsafe fn zero_contents(&mut self) {
-        let contents = slice::from_raw_parts_mut(self.data.as_mut_ptr(), self.length());
-        for v in contents {
-            *v = 0
+    fn data_ptr(ptr: NonNull<Self>) -> *mut u8 {
+        unsafe {
+            ptr.as_ptr()
+                .cast::<u8>()
+                .byte_offset(offset_of!(Self, data) as isize)
         }
-        self.data_type = 0;
-        self.total_length = [0; 4];
     }
-}
+
+    unsafe fn data_slice<'a>(ptr: NonNull<Self>) -> &'a mut [u8] {
+        unsafe { slice::from_raw_parts_mut(Self::data_ptr(ptr), ptr.as_ref().length()) }
+    }
 
-impl<'a> From<&'a CBinaryData> for (&'a [u8], u8) {
-    fn from(value: &'a CBinaryData) -> Self {
-        (
-            unsafe { slice::from_raw_parts(value.data.as_ptr(), value.length()) },
-            value.data_type,
-        )
+    pub unsafe fn data<'a>(ptr: NonNull<Self>) -> (&'a [u8], u8) {
+        unsafe { (Self::data_slice(ptr), ptr.as_ref().data_type) }
     }
-}
 
-impl From<&'_ CBinaryData> for BinaryData {
-    fn from(value: &'_ CBinaryData) -> Self {
-        // This is a dumb trick but I like it because it is simply the presence
-        // of `.map(|z: (_, _)| z)` in the middle of this that gives
-        // type inference the hint it needs to make this work.
-        let [ret] = [value].map(Into::into).map(|z: (_, _)| z).map(Into::into);
-        ret
+    pub unsafe fn zero_contents(ptr: NonNull<Self>) {
+        for byte in Self::data_slice(ptr) {
+            ptr::write_volatile(byte as *mut u8, mem::zeroed());
+        }
+        ptr::write_volatile(ptr.as_ptr(), mem::zeroed());
     }
-}
 
-impl From<Option<&'_ CBinaryData>> for BinaryData {
-    fn from(value: Option<&CBinaryData>) -> Self {
-        value.map(Into::into).unwrap_or_default()
+    #[allow(clippy::wrong_self_convention)]
+    pub unsafe fn as_binary_data(ptr: NonNull<Self>) -> BinaryData {
+        let (data, data_type) = unsafe { (CBinaryData::data_slice(ptr), ptr.as_ref().data_type) };
+        (Vec::from(data), data_type).into()
     }
 }
 
@@ -193,6 +188,7 @@
     #[test]
     fn test_strings() {
         let str = malloc_str("hello there").unwrap();
+        let str = str.as_ptr();
         malloc_str("hell\0 there").unwrap_err();
         unsafe {
             let copied = copy_pam_string(str).unwrap();