changeset 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 999bf07efbcb
children add7228adb2f
files libpam-sys/libpam-sys-helpers/src/memory.rs libpam-sys/libpam-sys-test/build.rs libpam-sys/libpam-sys-test/tests/runner.rs src/constants.rs src/libpam/answer.rs src/libpam/conversation.rs src/libpam/environ.rs src/libpam/memory.rs src/libpam/question.rs
diffstat 9 files changed, 229 insertions(+), 207 deletions(-) [+]
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()
         )
     }
 
--- 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});")
                 }
--- 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();
     }
--- 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));
     }
--- 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<OwnedExchange>) -> Result<Self> {
         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<NonNull<CBinaryData>> {
+    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]
--- 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()
         }
--- 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<Option<CHeapString>> = memory::calloc(strings.len() + 1).unwrap();
+        let ptrs: NonNull<Option<CHeapString>> = memory::calloc(strings.len() + 1);
         unsafe {
             for (idx, &text) in strings.iter().enumerate() {
                 ptr::write(
--- 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() {
--- 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<CHeapBox<c_void>>,
+    pub data: Option<NonNull<c_void>>,
 }
 
 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<TooBigError> for ErrorCode {
+    fn from(_: TooBigError) -> Self {
+        ErrorCode::BufferError
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;