diff src/libpam/memory.rs @ 139:33b9622ed6d2

Remove redundant memory management in nonstick::libpam; fix UB. - Uses the libpam-sys-helpers BinaryPayload / OwnedBinaryPayload structs to handle memory management and parsing for Linux-PAM binary messages. - Gets rid of the (technically) undefined behavior in PtrPtrVec due to pointer provenance. - Don't check for malloc failing. It won't, even if it does. - Formatting/cleanups/etc.
author Paul Fisher <paul@pfish.zone>
date Thu, 03 Jul 2025 23:57:49 -0400
parents 49d9e2b5c189
children a508a69c068a
line wrap: on
line diff
--- a/src/libpam/memory.rs	Thu Jul 03 20:55:40 2025 -0400
+++ b/src/libpam/memory.rs	Thu Jul 03 23:57:49 2025 -0400
@@ -1,41 +1,20 @@
 //! Things for dealing with memory.
 
+use crate::ErrorCode;
 use crate::Result;
-use crate::{BinaryData, ErrorCode};
-use memoffset::offset_of;
-use std::error::Error;
+use libpam_sys_helpers::memory::{Buffer, OwnedBinaryPayload};
 use std::ffi::{c_char, CStr, CString};
-use std::fmt::{Display, Formatter, Result as FmtResult};
 use std::marker::{PhantomData, PhantomPinned};
 use std::mem::ManuallyDrop;
 use std::ops::{Deref, DerefMut};
 use std::ptr::NonNull;
-use std::result::Result as StdResult;
 use std::{mem, ptr, slice};
 
-/// Raised from `calloc` when you have no memory!
-#[derive(Debug)]
-pub struct NoMem;
-
-impl Display for NoMem {
-    fn fmt(&self, f: &mut Formatter<'_>) -> FmtResult {
-        write!(f, "out of memory!")
-    }
-}
-
-impl Error for NoMem {}
-
-impl From<NoMem> for ErrorCode {
-    fn from(_: NoMem) -> Self {
-        ErrorCode::BufferError
-    }
-}
-
 /// Allocates `count` elements to hold `T`.
 #[inline]
-pub fn calloc<T>(count: usize) -> StdResult<NonNull<T>, NoMem> {
+pub fn calloc<T>(count: usize) -> NonNull<T> {
     // SAFETY: it's always safe to allocate! Leaking memory is fun!
-    NonNull::new(unsafe { libc::calloc(count, mem::size_of::<T>()) }.cast()).ok_or(NoMem)
+    unsafe { NonNull::new_unchecked(libc::calloc(count, mem::size_of::<T>()).cast()) }
 }
 
 /// Wrapper for [`libc::free`] to make debugging calls/frees easier.
@@ -79,7 +58,7 @@
 impl<T> CHeapBox<T> {
     /// Creates a new CHeapBox holding the given data.
     pub fn new(value: T) -> Result<Self> {
-        let memory = calloc(1)?;
+        let memory = calloc(1);
         unsafe { ptr::write(memory.as_ptr(), value) }
         // SAFETY: We literally just allocated this.
         Ok(Self(memory))
@@ -107,6 +86,11 @@
         this.0
     }
 
+    /// Because it's annoying to type `CHeapBox.as_ptr(...).as_ptr()`.
+    pub fn as_raw_ptr(this: &Self) -> *mut T {
+        this.0.as_ptr()
+    }
+
     /// Converts this into a Box of a different type.
     ///
     /// # Safety
@@ -123,6 +107,31 @@
     }
 }
 
+impl Buffer for CHeapBox<u8> {
+    fn allocate(len: usize) -> Self {
+        // SAFETY: This is all freshly-allocated memory!
+        unsafe { Self::from_ptr(calloc(len)) }
+    }
+
+    fn as_ptr(this: &Self) -> *const u8 {
+        this.0.as_ptr()
+    }
+
+    unsafe fn as_mut_slice(this: &mut Self, len: usize) -> &mut [u8] {
+        slice::from_raw_parts_mut(this.0.as_ptr(), len)
+    }
+
+    fn into_ptr(this: Self) -> NonNull<u8> {
+        CHeapBox::into_ptr(this)
+    }
+
+    unsafe fn from_ptr(ptr: NonNull<u8>, _: usize) -> Self {
+        CHeapBox::from_ptr(ptr)
+    }
+}
+
+pub type CHeapPayload = OwnedBinaryPayload<CHeapBox<u8>>;
+
 impl<T> Deref for CHeapBox<T> {
     type Target = T;
     fn deref(&self) -> &Self::Target {
@@ -164,10 +173,11 @@
             return Err(ErrorCode::ConversationError);
         }
         // +1 for the null terminator
-        let data_alloc: NonNull<c_char> = calloc(data.len() + 1)?;
+        let data_alloc: NonNull<c_char> = calloc(data.len() + 1);
         // SAFETY: we just allocated this and we have enough room.
         unsafe {
-            libc::memcpy(data_alloc.as_ptr().cast(), data.as_ptr().cast(), data.len());
+            let dest = slice::from_raw_parts_mut(data_alloc.as_ptr().cast(), data.len());
+            dest.copy_from_slice(data);
             Ok(Self(CHeapBox::from_ptr(data_alloc)))
         }
     }
@@ -248,101 +258,31 @@
     Ok(borrowed.map(String::from))
 }
 
-/// Binary data used in requests and responses.
-///
-/// This is an unsized data type whose memory goes beyond its data.
-/// This must be allocated on the C heap.
-///
-/// A Linux-PAM extension.
-#[repr(C)]
-pub struct CBinaryData {
-    /// The total length of the structure; a u32 in network byte order (BE).
-    total_length: [u8; 4],
-    /// A tag of undefined meaning.
-    data_type: u8,
-    /// Pointer to an array of length [`length`](Self::length) − 5
-    data: [u8; 0],
-    _marker: Immovable,
-}
-
-impl CBinaryData {
-    /// Copies the given data to a new BinaryData on the heap.
-    pub fn alloc((data, data_type): (&[u8], u8)) -> Result<CHeapBox<CBinaryData>> {
-        let buffer_size =
-            u32::try_from(data.len() + 5).map_err(|_| ErrorCode::ConversationError)?;
-        // SAFETY: We're only allocating here.
-        unsafe {
-            let mut dest_buffer: NonNull<Self> = 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;
-            libc::memcpy(
-                Self::data_ptr(dest_buffer).cast(),
-                data.as_ptr().cast(),
-                data.len(),
-            );
-            Ok(CHeapBox::from_ptr(dest_buffer))
-        }
-    }
-
-    fn length(&self) -> usize {
-        u32::from_be_bytes(self.total_length).saturating_sub(5) as usize
-    }
-
-    fn data_ptr(ptr: NonNull<Self>) -> *mut u8 {
-        unsafe {
-            ptr.as_ptr()
-                .cast::<u8>()
-                .byte_offset(offset_of!(Self, data) as isize)
-        }
-    }
-
-    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()) }
-    }
-
-    pub unsafe fn data<'a>(ptr: NonNull<Self>) -> (&'a [u8], u8) {
-        unsafe { (Self::data_slice(ptr), ptr.as_ref().data_type) }
-    }
-
-    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());
-    }
-
-    #[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()
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;
-    use std::hint;
+    use std::cell::Cell;
     #[test]
     fn test_box() {
-        #[allow(non_upper_case_globals)]
-        static mut drop_count: u32 = 0;
+        let drop_count: Cell<u32> = Cell::new(0);
+
+        struct Dropper<'a>(&'a Cell<u32>);
 
-        struct Dropper(i32);
-
-        impl Drop for Dropper {
+        impl Drop for Dropper<'_> {
             fn drop(&mut self) {
-                unsafe { drop_count += 1 }
+                self.0.set(self.0.get() + 1)
             }
         }
 
-        let mut dropbox = CHeapBox::new(Dropper(9)).unwrap();
-        hint::black_box(dropbox.0);
-        dropbox = CHeapBox::new(Dropper(10)).unwrap();
-        assert_eq!(1, unsafe { drop_count });
-        hint::black_box(dropbox.0);
+        let mut dropbox = CHeapBox::new(Dropper(&drop_count)).unwrap();
+        _ = dropbox;
+        // ensure the old value is dropped when the new one is assigned.
+        dropbox = CHeapBox::new(Dropper(&drop_count)).unwrap();
+        assert_eq!(1, drop_count.get());
+        *dropbox = Dropper(&drop_count);
+        assert_eq!(2, drop_count.get());
         drop(dropbox);
-        assert_eq!(2, unsafe { drop_count });
+        assert_eq!(3, drop_count.get());
     }
     #[test]
     fn test_strings() {