# HG changeset patch # User Paul Fisher # Date 1751601469 14400 # Node ID 33b9622ed6d246df1751f48c5a01827ed89aaeff # Parent 999bf07efbcbbd2f5d8f63cd90ac2d898aa3e920 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. diff -r 999bf07efbcb -r 33b9622ed6d2 libpam-sys/libpam-sys-helpers/src/memory.rs --- 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 PtrPtrVec { /// Takes ownership of the given Vec and creates a vec of pointers to it. pub fn new(data: Vec) -> 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 { +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; + fn into_ptr(this: Self) -> NonNull; /// "Adopts" the memory at the given pointer, taking it under management. /// @@ -216,9 +261,9 @@ /// /// ``` /// # use libpam_sys_helpers::memory::Buffer; - /// # fn test>(bytes: usize) { + /// # fn test(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, bytes: usize) -> Self; + unsafe fn from_ptr(ptr: NonNull, bytes: usize) -> Self; } -impl Buffer for Vec { +impl Buffer for Vec { 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 { - let mut me = ManuallyDrop::new(self); + fn into_ptr(this: Self) -> NonNull { + 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, bytes: usize) -> Self { + unsafe fn from_ptr(ptr: NonNull, 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`] 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); +pub struct OwnedBinaryPayload(Owner); -impl> OwnedBinaryPayload { +impl OwnedBinaryPayload { /// 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 { + pub fn new(data: &[u8], type_: u8) -> Result { 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 { - 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::>::from_ptr` is not allowed. pub unsafe fn from_ptr(ptr: NonNull) -> 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() ) } diff -r 999bf07efbcb -r 33b9622ed6d2 libpam-sys/libpam-sys-test/build.rs --- a/libpam-sys/libpam-sys-test/build.rs Thu Jul 03 20:55:40 2025 -0400 +++ b/libpam-sys/libpam-sys-test/build.rs Thu Jul 03 23:57:49 2025 -0400 @@ -28,7 +28,6 @@ "__LINUX_PAM_MINOR__", "PAM_AUTHTOK_RECOVER_ERR", ], - ..Default::default() }, PamImpl::OpenPam => TestConfig { headers: vec![ @@ -95,8 +94,10 @@ .map(|item| { let name = item.ident.to_string(); if let Some(stripped) = name.strip_prefix(&format!("{REDIR_FD}_")) { - format!("assert_eq!(generated::{name} as i32, libpam_sys::{REDIR_FD}::{stripped}.into());"); - format!("assert_eq!((generated::{name} as i32).try_into(), Ok(libpam_sys::{REDIR_FD}::{stripped}));") + format!("\ + assert_eq!(generated::{name} as i32, libpam_sys::{REDIR_FD}::{stripped}.into());\ + assert_eq!((generated::{name} as i32).try_into(), Ok(libpam_sys::{REDIR_FD}::{stripped}));\ + ") } else { format!("assert_eq!(generated::{name} as i32, libpam_sys::{name});") } diff -r 999bf07efbcb -r 33b9622ed6d2 libpam-sys/libpam-sys-test/tests/runner.rs --- a/libpam-sys/libpam-sys-test/tests/runner.rs Thu Jul 03 20:55:40 2025 -0400 +++ b/libpam-sys/libpam-sys-test/tests/runner.rs Thu Jul 03 23:57:49 2025 -0400 @@ -6,8 +6,13 @@ }; } +#[allow( + dead_code, + non_camel_case_types, + non_upper_case_globals, + clippy::unnecessary_cast +)] mod constants { - #[allow(dead_code, non_camel_case_types, non_upper_case_globals)] mod generated { include_test!("bindgen.rs"); } @@ -20,13 +25,18 @@ } } -#[allow(clippy::all)] +#[allow( + clippy::disallowed_names, + clippy::needless_range_loop, + clippy::nonminimal_bool +)] mod ctest { use libc::*; use libpam_sys::*; include_test!("ctest.rs"); #[test] + #[cfg_attr(miri, ignore)] fn test_c() { main(); } diff -r 999bf07efbcb -r 33b9622ed6d2 src/constants.rs --- a/src/constants.rs Thu Jul 03 20:55:40 2025 -0400 +++ b/src/constants.rs Thu Jul 03 23:57:49 2025 -0400 @@ -228,12 +228,12 @@ fn test_enums() { assert_eq!(Ok(()), ErrorCode::result_from(0)); assert_eq!( - pam_constants::PAM_SESSION_ERR as i32, + pam_constants::PAM_SESSION_ERR, ErrorCode::result_to_c::<()>(Err(ErrorCode::SessionError)) ); assert_eq!( Err(ErrorCode::Abort), - ErrorCode::result_from(pam_constants::PAM_ABORT as i32) + ErrorCode::result_from(pam_constants::PAM_ABORT) ); assert_eq!(Err(ErrorCode::SystemError), ErrorCode::result_from(423)); } diff -r 999bf07efbcb -r 33b9622ed6d2 src/libpam/answer.rs --- a/src/libpam/answer.rs Thu Jul 03 20:55:40 2025 -0400 +++ b/src/libpam/answer.rs Thu Jul 03 23:57:49 2025 -0400 @@ -2,8 +2,9 @@ use crate::libpam::conversation::OwnedExchange; use crate::libpam::memory; -use crate::libpam::memory::{CBinaryData, CHeapBox, CHeapString, Immovable}; +use crate::libpam::memory::{CHeapBox, CHeapPayload, CHeapString, Immovable}; use crate::{ErrorCode, Result}; +use libpam_sys_helpers::memory::BinaryPayload; use std::ffi::{c_int, c_void, CStr}; use std::mem::ManuallyDrop; use std::ops::{Deref, DerefMut}; @@ -23,7 +24,7 @@ /// Builds an Answers out of the given answered Message Q&As. pub fn build(value: Vec) -> Result { let mut outputs = Self { - base: memory::calloc(value.len())?, + base: memory::calloc(value.len()), count: value.len(), }; // Even if we fail during this process, we still end up freeing @@ -193,21 +194,22 @@ /// /// The referenced data is copied to the C heap. /// We do not take ownership of the original data. - pub fn fill(dest: &mut Answer, data_and_type: (&[u8], u8)) -> Result<()> { - let allocated = CBinaryData::alloc(data_and_type)?; - let _ = dest.data.replace(unsafe { CHeapBox::cast(allocated) }); + pub fn fill(dest: &mut Answer, (data, type_): (&[u8], u8)) -> Result<()> { + let payload = CHeapPayload::new(data, type_).map_err(|_| ErrorCode::BufferError)?; + let _ = dest + .data + .replace(unsafe { CHeapBox::cast(payload.into_inner()) }); Ok(()) } /// Gets the binary data in this answer. - pub fn data(&self) -> Option> { + pub fn contents(&self) -> Option<(&[u8], u8)> { // SAFETY: We either got this data from PAM or allocated it ourselves. // Either way, we trust that it is either valid data or null. self.0 .data .as_ref() - .map(CHeapBox::as_ptr) - .map(NonNull::cast) + .map(|data| unsafe { BinaryPayload::contents(CHeapBox::as_ptr(data).cast().as_ptr()) }) } /// Zeroes out the answer data, frees it, and points our data to `null`. @@ -217,10 +219,12 @@ /// but it will clear potentially sensitive data. pub fn zero_contents(&mut self) { // SAFETY: We know that our data pointer is either valid or null. - // Once we're done, it's null and the answer is safe. - unsafe { - if let Some(ptr) = self.0.data.as_ref() { - CBinaryData::zero_contents(CHeapBox::as_ptr(ptr).cast()) + if let Some(data) = self.0.data.as_mut() { + unsafe { + let total = BinaryPayload::total_bytes(CHeapBox::as_ptr(data).cast().as_ref()); + let data: &mut [u8] = + slice::from_raw_parts_mut(CHeapBox::as_raw_ptr(data).cast(), total); + data.fill(0) } } } @@ -290,13 +294,9 @@ if let [bin, radio] = &mut answers[..] { let up = unsafe { BinaryAnswer::upcast(bin) }; - assert_eq!(BinaryData::from((&[1, 2, 3][..], 99)), unsafe { - CBinaryData::as_binary_data(up.data().unwrap()) - }); + assert_eq!((&[1, 2, 3][..], 99), up.contents().unwrap()); up.zero_contents(); - assert_eq!(BinaryData::default(), unsafe { - CBinaryData::as_binary_data(up.data().unwrap()) - }); + assert_eq!((&[][..], 0), up.contents().unwrap()); assert_text_answer("beep boop", radio); } else { @@ -323,9 +323,7 @@ let real_data = BinaryData::new([1, 2, 3, 4, 5, 6, 7, 8], 9); BinaryAnswer::fill(&mut answer, (&real_data).into()).expect("alloc should succeed"); let bin_answer = unsafe { BinaryAnswer::upcast(&mut answer) }; - assert_eq!(real_data, unsafe { - CBinaryData::as_binary_data(bin_answer.data().unwrap()) - }); + assert_eq!(real_data, bin_answer.contents().unwrap().into()); } #[test] diff -r 999bf07efbcb -r 33b9622ed6d2 src/libpam/conversation.rs --- a/src/libpam/conversation.rs Thu Jul 03 20:55:40 2025 -0400 +++ b/src/libpam/conversation.rs Thu Jul 03 23:57:49 2025 -0400 @@ -2,12 +2,11 @@ use crate::conv::{Conversation, ErrorMsg, Exchange, InfoMsg, MaskedQAndA, QAndA}; use crate::libpam::answer::BinaryAnswer; use crate::libpam::answer::{Answer, Answers, TextAnswer}; -use crate::libpam::memory::CBinaryData; use crate::libpam::question::Question; use crate::ErrorCode; use crate::Result; +use libpam_sys::AppData; use libpam_sys_helpers::memory::PtrPtrVec; -use libpam_sys::AppData; use std::ffi::c_int; use std::iter; use std::marker::PhantomData; @@ -153,8 +152,8 @@ Exchange::BinaryPrompt(qa) => { let bin_resp = unsafe { BinaryAnswer::upcast(resp) }; qa.set_answer(Ok(bin_resp - .data() - .map(|d| unsafe { CBinaryData::as_binary_data(d) }) + .contents() + .map(|d| d.into()) .unwrap_or_default())); bin_resp.zero_contents() } diff -r 999bf07efbcb -r 33b9622ed6d2 src/libpam/environ.rs --- a/src/libpam/environ.rs Thu Jul 03 20:55:40 2025 -0400 +++ b/src/libpam/environ.rs Thu Jul 03 23:57:49 2025 -0400 @@ -211,7 +211,7 @@ } fn env_list(strings: &[&'static str]) -> EnvList<'static> { - let ptrs: NonNull> = memory::calloc(strings.len() + 1).unwrap(); + let ptrs: NonNull> = memory::calloc(strings.len() + 1); unsafe { for (idx, &text) in strings.iter().enumerate() { ptr::write( diff -r 999bf07efbcb -r 33b9622ed6d2 src/libpam/memory.rs --- 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 for ErrorCode { - fn from(_: NoMem) -> Self { - ErrorCode::BufferError - } -} - /// Allocates `count` elements to hold `T`. #[inline] -pub fn calloc(count: usize) -> StdResult, NoMem> { +pub fn calloc(count: usize) -> NonNull { // SAFETY: it's always safe to allocate! Leaking memory is fun! - NonNull::new(unsafe { libc::calloc(count, mem::size_of::()) }.cast()).ok_or(NoMem) + unsafe { NonNull::new_unchecked(libc::calloc(count, mem::size_of::()).cast()) } } /// Wrapper for [`libc::free`] to make debugging calls/frees easier. @@ -79,7 +58,7 @@ impl CHeapBox { /// Creates a new CHeapBox holding the given data. pub fn new(value: T) -> Result { - 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 { + 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 { + CHeapBox::into_ptr(this) + } + + unsafe fn from_ptr(ptr: NonNull, _: usize) -> Self { + CHeapBox::from_ptr(ptr) + } +} + +pub type CHeapPayload = OwnedBinaryPayload>; + impl Deref for CHeapBox { 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 = calloc(data.len() + 1)?; + let data_alloc: NonNull = 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> { - 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 = calloc::(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) -> *mut u8 { - unsafe { - ptr.as_ptr() - .cast::() - .byte_offset(offset_of!(Self, data) as isize) - } - } - - unsafe fn data_slice<'a>(ptr: NonNull) -> &'a mut [u8] { - unsafe { slice::from_raw_parts_mut(Self::data_ptr(ptr), ptr.as_ref().length()) } - } - - pub unsafe fn data<'a>(ptr: NonNull) -> (&'a [u8], u8) { - unsafe { (Self::data_slice(ptr), ptr.as_ref().data_type) } - } - - pub unsafe fn zero_contents(ptr: NonNull) { - 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) -> 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 = Cell::new(0); + + struct Dropper<'a>(&'a Cell); - 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() { diff -r 999bf07efbcb -r 33b9622ed6d2 src/libpam/question.rs --- a/src/libpam/question.rs Thu Jul 03 20:55:40 2025 -0400 +++ b/src/libpam/question.rs Thu Jul 03 23:57:49 2025 -0400 @@ -2,13 +2,15 @@ #[cfg(feature = "linux-pam-ext")] use crate::conv::{BinaryQAndA, RadioQAndA}; +use libpam_sys_helpers::memory::{BinaryPayload, TooBigError}; use crate::conv::{ErrorMsg, Exchange, InfoMsg, MaskedQAndA, QAndA}; use crate::libpam::conversation::OwnedExchange; -use crate::libpam::memory::{CBinaryData, CHeapBox, CHeapString}; +use crate::libpam::memory::{CHeapBox, CHeapPayload, CHeapString}; use crate::ErrorCode; use crate::Result; use num_enum::{IntoPrimitive, TryFromPrimitive}; use std::ffi::{c_int, c_void, CStr}; +use std::ptr::NonNull; mod style_const { pub use libpam_sys::*; @@ -46,7 +48,7 @@ /// A question sent by PAM or a module to an application. /// /// PAM refers to this as a "message", but we call it a question -/// to avoid confusion with [`Message`](crate::conv::Exchange). +/// to avoid confusion. /// /// This question, and its internal data, is owned by its creator /// (either the module or PAM itself). @@ -60,7 +62,7 @@ /// For most requests, this will be an owned [`CStr`], /// but for requests with style `PAM_BINARY_PROMPT`, /// this will be `CBinaryData` (a Linux-PAM extension). - pub data: Option>, + pub data: Option>, } impl Question { @@ -72,7 +74,7 @@ unsafe fn string_data(&self) -> Result<&str> { match self.data.as_ref() { None => Ok(""), - Some(data) => CStr::from_ptr(CHeapBox::as_ptr(data).cast().as_ptr()) + Some(data) => CStr::from_ptr(data.as_ptr().cast()) .to_str() .map_err(|_| ErrorCode::ConversationError), } @@ -82,7 +84,7 @@ unsafe fn binary_data(&self) -> (&[u8], u8) { self.data .as_ref() - .map(|data| CBinaryData::data(CHeapBox::as_ptr(data).cast())) + .map(|data| BinaryPayload::contents(data.as_ptr().cast())) .unwrap_or_default() } } @@ -104,9 +106,11 @@ #[cfg(feature = "linux-pam-ext")] Exchange::RadioPrompt(p) => alloc(Style::RadioType, p.question()), #[cfg(feature = "linux-pam-ext")] - Exchange::BinaryPrompt(p) => Ok((Style::BinaryPrompt, unsafe { - CHeapBox::cast(CBinaryData::alloc(p.question())?) - })), + Exchange::BinaryPrompt(p) => { + let (data, typ) = p.question(); + let payload = CHeapPayload::new(data, typ)?.into_inner(); + Ok((Style::BinaryPrompt, unsafe { CHeapBox::cast(payload) })) + }, #[cfg(not(feature = "linux-pam-ext"))] Exchange::RadioPrompt(_) | Exchange::BinaryPrompt(_) => { Err(ErrorCode::ConversationError) @@ -114,7 +118,7 @@ }?; Ok(Self { style: style.into(), - data: Some(data), + data: Some(CHeapBox::into_ptr(data)), }) } } @@ -131,22 +135,22 @@ #[cfg(feature = "linux-pam-ext")] Style::BinaryPrompt => self .data - .as_ref() - .map(|p| CBinaryData::zero_contents(CHeapBox::as_ptr(p).cast())), + .as_mut() + .map(|p| BinaryPayload::zero(p.as_ptr().cast())), #[cfg(feature = "linux-pam-ext")] Style::RadioType => self .data - .as_ref() - .map(|p| CHeapString::zero(CHeapBox::as_ptr(p).cast())), + .as_mut() + .map(|p| CHeapString::zero(p.cast())), Style::TextInfo | Style::ErrorMsg | Style::PromptEchoOff - | Style::PromptEchoOn => self - .data - .as_ref() - .map(|p| CHeapString::zero(CHeapBox::as_ptr(p).cast())), + | Style::PromptEchoOn => { + self.data.as_mut().map(|p| CHeapString::zero(p.cast())) + } }; }; + let _ = self.data.map(|p| CHeapBox::from_ptr(p)); } } } @@ -178,6 +182,13 @@ } } +#[cfg(feature = "linux-pam-ext")] +impl From for ErrorCode { + fn from(_: TooBigError) -> Self { + ErrorCode::BufferError + } +} + #[cfg(test)] mod tests { use super::*;