diff libpam-sys/libpam-sys-helpers/src/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 efbc235f01d3
children add7228adb2f
line wrap: on
line diff
--- a/libpam-sys/libpam-sys-helpers/src/memory.rs	Thu Jul 03 20:55:40 2025 -0400
+++ b/libpam-sys/libpam-sys-helpers/src/memory.rs	Thu Jul 03 23:57:49 2025 -0400
@@ -1,12 +1,10 @@
 //! Helpers to deal with annoying memory management in the PAM API.
-//!
-//!
 
 use std::error::Error;
 use std::marker::{PhantomData, PhantomPinned};
 use std::mem::ManuallyDrop;
 use std::ptr::NonNull;
-use std::{any, fmt, mem, slice};
+use std::{any, fmt, mem, ptr, slice};
 
 /// A pointer-to-pointer-to-message container for PAM's conversation callback.
 ///
@@ -73,7 +71,54 @@
 impl<T> PtrPtrVec<T> {
     /// Takes ownership of the given Vec and creates a vec of pointers to it.
     pub fn new(data: Vec<T>) -> Self {
-        let pointers: Vec<_> = data.iter().map(|r| r as *const T).collect();
+        let start = data.as_ptr();
+        // We do this slightly tricky little dance to satisfy Miri:
+        //
+        // A pointer extracted from a reference can only legally access
+        // that reference's memory. This means that if we say:
+        //     pointers[0] = &data[0] as *const T;
+        // we can't traverse through pointers[0] to reach data[1],
+        // we can only use pointers[1].
+        //
+        // However, if we use the start-of-vec pointer from the `data` vector,
+        // its "provenance"* is valid for the entire array (even if the address
+        // of the pointer is the same). This avoids some behavior which is
+        // technically undefined. While the CPU sees no difference between
+        // those two pointers, the compiler is allowed to make optimizations
+        // based on that provenance (even if, in this case, it isn't likely
+        // to do so).
+        //
+        //       data.as_ptr() points here, and is valid for the whole Vec.
+        //       ┃
+        //       ┠─────────────────╮
+        //       ┌─────┬─────┬─────┐
+        //  data │ [0] │ [1] │ [2] │
+        //       └─────┴─────┴─────┘
+        //       ┠─────╯     ┊
+        //       ┃     ┊     ┊
+        //       (&data[0] as *const T) points to the same place, but is valid
+        //       only for that 0th element.
+        //             ┊     ┊
+        //             ┠─────╯
+        //             ┃
+        //             (&data[1] as *const T) points here, and is only valid
+        //             for that element.
+        //
+        // We only have to do this for pointers[0] because only that pointer
+        // is used for accessing elements other than data[0] (in XSSO).
+        //
+        // * "provenance" is kind of like if every pointer in your program
+        // remembered where it came from and, based on that, it had an implied
+        // memory range it was valid for, separate from its address.
+        // https://doc.rust-lang.org/std/ptr/#provenance
+        // (It took a long time for me to understand this.)
+        let mut pointers = Vec::with_capacity(data.len());
+        // Ensure the 0th pointer has provenance from the entire vec
+        // (even though it's numerically identical to &data[0] as *const T).
+        pointers.push(start);
+        // The 1st and everything thereafter only need to have the provenance
+        // of their own memory.
+        pointers.extend(data[1..].iter().map(|r| r as *const T));
         Self { data, pointers }
     }
 
@@ -194,21 +239,21 @@
 ///
 /// For an implementation example, see the implementation of this trait
 /// for [`Vec`].
-pub trait Buffer<T: Default> {
+pub trait Buffer {
     /// Allocates a buffer of `len` elements, filled with the default.
     fn allocate(len: usize) -> Self;
 
-    fn as_ptr(&self) -> *const T;
+    fn as_ptr(this: &Self) -> *const u8;
 
     /// Returns a slice view of `size` elements of the given memory.
     ///
     /// # Safety
     ///
     /// The caller must not request more elements than are allocated.
-    unsafe fn as_mut_slice(&mut self, len: usize) -> &mut [T];
+    unsafe fn as_mut_slice(this: &mut Self, len: usize) -> &mut [u8];
 
     /// Consumes this ownership and returns a pointer to the start of the arena.
-    fn into_ptr(self) -> NonNull<T>;
+    fn into_ptr(this: Self) -> NonNull<u8>;
 
     /// "Adopts" the memory at the given pointer, taking it under management.
     ///
@@ -216,9 +261,9 @@
     ///
     /// ```
     /// # use libpam_sys_helpers::memory::Buffer;
-    /// # fn test<T: Default, OwnerType: Buffer<T>>(bytes: usize) {
+    /// # fn test<T: Default, OwnerType: Buffer>(bytes: usize) {
     /// let owner = OwnerType::allocate(bytes);
-    /// let ptr = owner.into_ptr();
+    /// let ptr = OwnerType::into_ptr(owner);
     /// let owner = unsafe { OwnerType::from_ptr(ptr, bytes) };
     /// # }
     /// ```
@@ -229,30 +274,29 @@
     ///
     /// The pointer must be valid, and the caller must provide the exact size
     /// of the given arena.
-    unsafe fn from_ptr(ptr: NonNull<T>, bytes: usize) -> Self;
+    unsafe fn from_ptr(ptr: NonNull<u8>, bytes: usize) -> Self;
 }
 
-impl<T: Default> Buffer<T> for Vec<T> {
+impl Buffer for Vec<u8> {
     fn allocate(bytes: usize) -> Self {
-        (0..bytes).map(|_| Default::default()).collect()
+        vec![0; bytes]
     }
 
-    fn as_ptr(&self) -> *const T {
-        Vec::as_ptr(self)
+    fn as_ptr(this: &Self) -> *const u8 {
+        Vec::as_ptr(this)
     }
 
-    unsafe fn as_mut_slice(&mut self, bytes: usize) -> &mut [T] {
-        debug_assert!(bytes <= self.len());
-        Vec::as_mut(self)
+    unsafe fn as_mut_slice(this: &mut Self, bytes: usize) -> &mut [u8] {
+        &mut this[..bytes]
     }
 
-    fn into_ptr(self) -> NonNull<T> {
-        let mut me = ManuallyDrop::new(self);
+    fn into_ptr(this: Self) -> NonNull<u8> {
+        let mut me = ManuallyDrop::new(this);
         // SAFETY: a Vec is guaranteed to have a nonzero pointer.
         unsafe { NonNull::new_unchecked(me.as_mut_ptr()) }
     }
 
-    unsafe fn from_ptr(ptr: NonNull<T>, bytes: usize) -> Self {
+    unsafe fn from_ptr(ptr: NonNull<u8>, bytes: usize) -> Self {
         Vec::from_raw_parts(ptr.as_ptr(), bytes, bytes)
     }
 }
@@ -281,7 +325,7 @@
     /// This uses [`copy_from_slice`](slice::copy_from_slice) internally,
     /// so `buf` must be exactly 5 bytes longer than `data`, or this function
     /// will panic.
-    pub fn fill(buf: &mut [u8], data_type: u8, data: &[u8]) {
+    pub fn fill(buf: &mut [u8], data: &[u8], data_type: u8) {
         let ptr: *mut Self = buf.as_mut_ptr().cast();
         // SAFETY: We're given a slice, which always has a nonzero pointer.
         let me = unsafe { ptr.as_mut().unwrap_unchecked() };
@@ -291,8 +335,9 @@
     }
 
     /// The total storage needed for the message, including header.
-    pub fn total_bytes(&self) -> usize {
-        u32::from_be_bytes(self.total_bytes_u32be) as usize
+    pub unsafe fn total_bytes(this: *const Self) -> usize {
+        let header = this.as_ref().unwrap_unchecked();
+        u32::from_be_bytes(header.total_bytes_u32be) as usize
     }
 
     /// Gets the total byte buffer of the BinaryMessage stored at the pointer.
@@ -304,8 +349,7 @@
     /// - The pointer must point to a valid `BinaryPayload`.
     /// - The borrowed data must not outlive the pointer's validity.
     pub unsafe fn buffer_of<'a>(ptr: *const Self) -> &'a [u8] {
-        let header: &Self = ptr.as_ref().unwrap_unchecked();
-        slice::from_raw_parts(ptr.cast(), header.total_bytes().max(5))
+        slice::from_raw_parts(ptr.cast(), Self::total_bytes(ptr).max(5))
     }
 
     /// Gets the contents of the BinaryMessage stored at the given pointer.
@@ -322,9 +366,24 @@
     ///
     /// - The pointer must point to a valid `BinaryPayload`.
     /// - The borrowed data must not outlive the pointer's validity.
-    pub unsafe fn contents<'a>(ptr: *const Self) -> (u8, &'a [u8]) {
+    pub unsafe fn contents<'a>(ptr: *const Self) -> (&'a [u8], u8) {
         let header: &Self = ptr.as_ref().unwrap_unchecked();
-        (header.data_type, &Self::buffer_of(ptr)[5..])
+        (&Self::buffer_of(ptr)[5..], header.data_type)
+    }
+    
+    /// Zeroes out the data of this payload.
+    /// 
+    /// # Safety
+    /// 
+    /// - The pointer must point to a valid `BinaryPayload`.
+    /// - The binary payload must not be used in the future,
+    ///   since its length metadata is gone and so its buffer is unknown.
+    pub unsafe fn zero(ptr: *mut Self) {
+        let size = Self::total_bytes(ptr);
+        let ptr: *mut u8 = ptr.cast();
+        for x in 0..size {
+            ptr::write_volatile(ptr.byte_add(x), mem::zeroed())
+        }
     }
 }
 
@@ -335,14 +394,14 @@
 /// [`Vec<u8>`] is one such manager and can be used when ownership
 /// of the data does not need to transit through PAM.
 #[derive(Debug)]
-pub struct OwnedBinaryPayload<Owner: Buffer<u8>>(Owner);
+pub struct OwnedBinaryPayload<Owner: Buffer>(Owner);
 
-impl<O: Buffer<u8>> OwnedBinaryPayload<O> {
+impl<O: Buffer> OwnedBinaryPayload<O> {
     /// Allocates a new OwnedBinaryPayload.
     ///
     /// This will return a [`TooBigError`] if you try to allocate too much
     /// (more than [`BinaryPayload::MAX_SIZE`]).
-    pub fn new(data_type: u8, data: &[u8]) -> Result<Self, TooBigError> {
+    pub fn new(data: &[u8], type_: u8) -> Result<Self, TooBigError> {
         let total_len: u32 = (data.len() + 5).try_into().map_err(|_| TooBigError {
             size: data.len(),
             max: BinaryPayload::MAX_SIZE,
@@ -350,18 +409,22 @@
         let total_len = total_len as usize;
         let mut buf = O::allocate(total_len);
         // SAFETY: We just allocated this exact size.
-        BinaryPayload::fill(unsafe { buf.as_mut_slice(total_len) }, data_type, data);
+        BinaryPayload::fill(
+            unsafe { Buffer::as_mut_slice(&mut buf, total_len) },
+            data,
+            type_,
+        );
         Ok(Self(buf))
     }
 
     /// The contents of the buffer.
-    pub fn contents(&self) -> (u8, &[u8]) {
+    pub fn contents(&self) -> (&[u8], u8) {
         unsafe { BinaryPayload::contents(self.as_ptr()) }
     }
 
     /// The total bytes needed to store this, including the header.
     pub fn total_bytes(&self) -> usize {
-        unsafe { BinaryPayload::buffer_of(self.0.as_ptr().cast()).len() }
+        unsafe { BinaryPayload::buffer_of(Buffer::as_ptr(&self.0).cast()).len() }
     }
 
     /// Unwraps this into the raw storage backing it.
@@ -371,7 +434,7 @@
 
     /// Gets a const pointer to the start of the message's buffer.
     pub fn as_ptr(&self) -> *const BinaryPayload {
-        self.0.as_ptr().cast()
+        Buffer::as_ptr(&self.0).cast()
     }
 
     /// Consumes ownership of this message and converts it to a raw pointer
@@ -380,7 +443,7 @@
     /// To clean this up, you should eventually pass it into [`Self::from_ptr`]
     /// with the same `O` ownership type.
     pub fn into_ptr(self) -> NonNull<BinaryPayload> {
-        self.0.into_ptr().cast()
+        Buffer::into_ptr(self.0).cast()
     }
 
     /// Takes ownership of the given pointer.
@@ -391,7 +454,7 @@
     /// allocated by) [`Self::new`]. For instance, passing a pointer allocated
     /// by `malloc` to `OwnedBinaryPayload::<Vec<u8>>::from_ptr` is not allowed.
     pub unsafe fn from_ptr(ptr: NonNull<BinaryPayload>) -> Self {
-        Self(O::from_ptr(ptr.cast(), ptr.as_ref().total_bytes()))
+        Self(O::from_ptr(ptr.cast(), BinaryPayload::total_bytes(ptr.as_ptr())))
     }
 }
 
@@ -407,25 +470,25 @@
         let simple_message = &[0u8, 0, 0, 16, 0xff, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
         let empty = &[0u8; 5];
 
-        assert_eq!((0xff, &[0u8, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10][..]), unsafe {
+        assert_eq!((&[0u8, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10][..], 0xff), unsafe {
             BinaryPayload::contents(simple_message.as_ptr().cast())
         });
-        assert_eq!((0x00, &[][..]), unsafe {
+        assert_eq!((&[][..], 0x00), unsafe {
             BinaryPayload::contents(empty.as_ptr().cast())
         });
     }
 
     #[test]
     fn test_owned_binary_payload() {
-        let (typ, data) = (
-            112,
+        let (data, typ) = (
             &[0, 1, 1, 8, 9, 9, 9, 8, 8, 1, 9, 9, 9, 1, 1, 9, 7, 2, 5, 3][..],
+            112,
         );
-        let payload = VecPayload::new(typ, data).unwrap();
-        assert_eq!((typ, data), payload.contents());
+        let payload = VecPayload::new(data, typ).unwrap();
+        assert_eq!((data, typ), payload.contents());
         let ptr = payload.into_ptr();
         let payload = unsafe { VecPayload::from_ptr(ptr) };
-        assert_eq!((typ, data), payload.contents());
+        assert_eq!((data, typ), payload.contents());
     }
 
     #[test]
@@ -437,7 +500,7 @@
                 max: 0xffff_fffa,
                 size: 0x1_0000_0001
             },
-            VecPayload::new(5, &data).unwrap_err()
+            VecPayload::new(&data, 5).unwrap_err()
         )
     }