Mercurial > crates > nonstick
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() ) }