Mercurial > crates > nonstick
changeset 89:dd3e9c4bcde3
Simplify memory management in Questions.
When we're sending Questions to the client, we don't need them to be
C-managed, we just need the pointers going to the right place.
This replaces a bunch of Question management cruft with Vecs and Boxes.
| author | Paul Fisher <paul@pfish.zone> |
|---|---|
| date | Fri, 13 Jun 2025 05:22:48 -0400 |
| parents | c9fc7e6257d3 |
| children | f6186e41399b |
| files | src/libpam/conversation.rs src/libpam/pam_ffi.rs src/libpam/question.rs |
| diffstat | 3 files changed, 156 insertions(+), 238 deletions(-) [+] |
line wrap: on
line diff
--- a/src/libpam/conversation.rs Tue Jun 10 05:36:58 2025 -0400 +++ b/src/libpam/conversation.rs Fri Jun 13 05:22:48 2025 -0400 @@ -5,7 +5,8 @@ use crate::libpam::memory::Immovable; use crate::libpam::pam_ffi::AppData; pub use crate::libpam::pam_ffi::LibPamConversation; -use crate::libpam::question::{Indirect, IndirectTrait, Question, Questions}; +use crate::libpam::question::QuestionsTrait; +use crate::libpam::question::{Question, Questions}; use crate::ErrorCode; use crate::Result; use std::ffi::c_int; @@ -38,12 +39,11 @@ .cast::<C>() .as_mut() .ok_or(ErrorCode::ConversationError)?; - let indirect = Indirect::borrow_ptr(questions).ok_or(ErrorCode::ConversationError)?; + let indirect = Questions::borrow_ptr(questions, count as usize); let answers_ptr = answers.as_mut().ok_or(ErrorCode::ConversationError)?; // Build our owned list of Q&As from the questions we've been asked let messages: Vec<OwnedMessage> = indirect - .iter(count as usize) .map(TryInto::try_into) .collect::<Result<_>>() .map_err(|_| ErrorCode::ConversationError)?; @@ -71,7 +71,7 @@ let result = unsafe { (self.callback)( messages.len() as c_int, - questions.indirect(), + questions.ptr(), &mut response_pointer, self.appdata, )
--- a/src/libpam/pam_ffi.rs Tue Jun 10 05:36:58 2025 -0400 +++ b/src/libpam/pam_ffi.rs Fri Jun 13 05:22:48 2025 -0400 @@ -30,9 +30,11 @@ /// Pointer to the data returned in an answer. /// For most answers, this will be a [`CStr`](std::ffi::CStr), /// but for [`BinaryQAndA`](crate::conv::BinaryQAndA)s (a Linux-PAM extension), - /// this will be [`CBinaryData`](crate::libpam::memory::CBinaryData) + /// this will be [`CBinaryData`](crate::libpam::memory::CBinaryData). + /// + /// No matter what, this can be freed with a simple [`libc::free`]. pub data: *mut c_void, - /// Unused. + /// Unused. Just here for the padding. return_code: c_int, _marker: Immovable, } @@ -40,34 +42,35 @@ /// 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::Message). +/// to avoid confusion with [`Message`](crate::conv::Message). /// /// This question, and its internal data, is owned by its creator /// (either the module or PAM itself). #[repr(C)] +#[derive(Debug)] pub struct Question { /// The style of message to request. pub style: c_uint, /// A description of the data requested. /// - /// For most requests, this will be an owned [`CStr`](std::ffi::CStr), but for requests - /// with [`Style::BinaryPrompt`], this will be [`CBinaryData`] - /// (a Linux-PAM extension). + /// For most requests, this will be an owned [`CStr`](std::ffi::CStr), + /// but for requests with style `PAM_BINARY_PROMPT`, + /// this will be `CBinaryData` (a Linux-PAM extension). pub data: *mut c_void, pub _marker: Immovable, } /// The callback that PAM uses to get information in a conversation. /// -/// - `num_msg` is the number of messages in the `pam_message` array. +/// - `num_msg` is the number of messages in the `questions` array. /// - `questions` is a pointer to the [`Question`]s being sent to the user. /// For information about its structure, -/// see [`GenericQuestions`](super::question::GenericQuestions). +/// see [`QuestionsTrait`](super::question::QuestionsTrait). /// - `answers` is a pointer to an array of [`Answer`]s, /// which PAM sets in response to a module's request. /// This is an array of structs, not an array of pointers to a struct. -/// There should always be exactly as many `answers` as `num_msg`. -/// - `appdata` is the `appdata` field of the [`LibPamConversation`] we were passed. +/// There must always be exactly as many `answers` as `num_msg`. +/// - `appdata` is the `appdata` field of the [`LibPamConversation`]. pub type ConversationCallback = unsafe extern "C" fn( num_msg: c_int, questions: *const *const Question,
--- a/src/libpam/question.rs Tue Jun 10 05:36:58 2025 -0400 +++ b/src/libpam/question.rs Fri Jun 13 05:22:48 2025 -0400 @@ -11,7 +11,7 @@ use crate::Result; use num_enum::{IntoPrimitive, TryFromPrimitive}; use std::ffi::{c_void, CStr}; -use std::{iter, ptr, slice}; +use std::{ptr, slice}; /// Abstraction of a collection of questions to be sent in a PAM conversation. /// @@ -32,15 +32,15 @@ /// the pointer passed to `pam_conv`.) /// /// ```text -/// ╔═ Questions ═╗ points to ┌─ Indirect ─┐ ╔═ Question ═╗ -/// ║ indirect ┄┄┄╫┄┄┄┄┄┄┄┄┄┄┄> │ base[0] ┄┄┄┼┄┄┄┄┄> ║ style ║ -/// ║ count ║ │ base[1] ┄┄┄┼┄┄┄╮ ║ data ┄┄┄┄┄┄╫┄┄> ... -/// ╚═════════════╝ │ ... │ ┆ ╚════════════╝ -/// ┆ -/// ┆ ╔═ Question ═╗ -/// ╰┄┄> ║ style ║ -/// ║ data ┄┄┄┄┄┄╫┄┄> ... -/// ╚════════════╝ +/// points to ┌───────────────┐ ╔═ Question ═╗ +/// questions ┄┄┄┄┄┄┄┄┄┄> │ questions[0] ┄┼┄┄┄┄> ║ style ║ +/// │ questions[1] ┄┼┄┄┄╮ ║ data ┄┄┄┄┄┄╫┄┄> ... +/// │ ... │ ┆ ╚════════════╝ +/// ┆ +/// ┆ ╔═ Question ═╗ +/// ╰┄┄> ║ style ║ +/// ║ data ┄┄┄┄┄┄╫┄┄> ... +/// ╚════════════╝ /// ``` /// /// On OpenPAM and other compatible implementations (like Solaris), @@ -48,209 +48,135 @@ /// the correct implementation as required by the XSSO specification. /// /// ```text -/// ╔═ Questions ═╗ points to ┌─ Indirect ─┐ ╔═ Question[] ═╗ -/// ║ indirect ┄┄┄╫┄┄┄┄┄┄┄┄┄┄┄> │ base ┄┄┄┄┄┄┼┄┄┄┄┄> ║ style ║ -/// ║ count ║ └────────────┘ ║ data ┄┄┄┄┄┄┄┄╫┄┄> ... -/// ╚═════════════╝ ╟──────────────╢ -/// ║ style ║ -/// ║ data ┄┄┄┄┄┄┄┄╫┄┄> ... -/// ╟──────────────╢ -/// ║ ... ║ +/// points to ┌─────────────┐ ╔═ Question[] ═╗ +/// questions ┄┄┄┄┄┄┄┄┄┄> │ *questions ┄┼┄┄┄┄┄> ║ style ║ +/// └─────────────┘ ║ data ┄┄┄┄┄┄┄┄╫┄┄> ... +/// ╟──────────────╢ +/// ║ style ║ +/// ║ data ┄┄┄┄┄┄┄┄╫┄┄> ... +/// ╟──────────────╢ +/// ║ ... ║ /// ``` -#[derive(Debug)] -pub struct GenericQuestions<I: IndirectTrait> { - /// An indirection to the questions themselves, stored on the C heap. - indirect: *mut I, - /// The number of questions. - count: usize, -} - -impl<I: IndirectTrait> GenericQuestions<I> { - /// Stores the provided questions on the C heap. - pub fn new(messages: &[Message]) -> Result<Self> { - let count = messages.len(); - let mut ret = Self { - indirect: I::alloc(count), - count, - }; - // Even if we fail partway through this, all our memory will be freed. - for (question, message) in iter::zip(ret.iter_mut(), messages) { - question.try_fill(message)? - } - Ok(ret) - } +pub trait QuestionsTrait { + /// Allocates memory for this indirector and all its members. + fn new(messages: &[Message]) -> Result<Self> + where + Self: Sized; - /// The pointer to the thing with the actual list. - pub fn indirect(&self) -> *const *const Question { - self.indirect.cast() - } - - pub fn iter(&self) -> impl Iterator<Item = &Question> { - // SAFETY: we're iterating over an amount we know. - unsafe { (*self.indirect).iter(self.count) } - } - pub fn iter_mut(&mut self) -> impl Iterator<Item = &mut Question> { - // SAFETY: we're iterating over an amount we know. - unsafe { (*self.indirect).iter_mut(self.count) } - } -} + /// Gets the pointer that is passed . + fn ptr(&self) -> *const *const Question; -impl<I: IndirectTrait> Drop for GenericQuestions<I> { - fn drop(&mut self) { - // SAFETY: We are valid and have a valid pointer. - // Once we're done, everything will be safe. - unsafe { - if let Some(indirect) = self.indirect.as_mut() { - indirect.free_contents(self.count) - } - memory::free(self.indirect); - self.indirect = ptr::null_mut(); - } - } -} - -/// The trait that each of the `Indirect` implementations implement. -/// -/// Basically a slice but with more meat. -pub trait IndirectTrait { - /// Converts a pointer into a borrowed `Self`. + /// Converts a pointer into a borrowed list of Questions. /// /// # Safety /// /// You have to provide a valid pointer. - unsafe fn borrow_ptr<'a>(ptr: *const *const Question) -> Option<&'a Self> - where - Self: Sized, - { - ptr.cast::<Self>().as_ref() - } - - /// Allocates memory for this indirector and all its members. - fn alloc(count: usize) -> *mut Self; - - /// Returns an iterator yielding the given number of messages. - /// - /// # Safety - /// - /// You have to provide the right count. - unsafe fn iter(&self, count: usize) -> impl Iterator<Item = &Question>; - - /// Returns a mutable iterator yielding the given number of messages. - /// - /// # Safety - /// - /// You have to provide the right count. - unsafe fn iter_mut(&mut self, count: usize) -> impl Iterator<Item = &mut Question>; - - /// Frees everything this points to. - /// - /// # Safety - /// - /// You have to pass the right size. - unsafe fn free_contents(&mut self, count: usize); + unsafe fn borrow_ptr<'a>( + ptr: *const *const Question, + count: usize, + ) -> impl Iterator<Item = &'a Question>; } -/// An indirect reference to messages. -/// -/// This is kept separate to provide a place where we can separate -/// the pointer-to-pointer-to-list from pointer-to-list-of-pointers. #[cfg(pam_impl = "linux-pam")] -pub type Indirect = LinuxPamIndirect; +pub type Questions = LinuxPamQuestions; -/// An indirect reference to messages. -/// -/// This is kept separate to provide a place where we can separate -/// the pointer-to-pointer-to-list from pointer-to-list-of-pointers. #[cfg(not(pam_impl = "linux-pam"))] -pub type Indirect = StandardIndirect; +pub type Questions = XSsoQuestions; -pub type Questions = GenericQuestions<Indirect>; - -/// The XSSO standard version of the indirection layer between Question and Questions. +/// The XSSO standard version of the pointer train to questions. #[derive(Debug)] #[repr(C)] -pub struct StandardIndirect { - base: *mut Question, +pub struct XSsoQuestions { + /// Points to the memory address where the meat of `questions` is. + /// **The memory layout of Vec is not specified**, and we need to return + /// a pointer to the pointer, hence we have to store it here. + pointer: *const Question, + questions: Vec<Question>, _marker: Immovable, } -impl IndirectTrait for StandardIndirect { - fn alloc(count: usize) -> *mut Self { - let questions = memory::calloc(count); - let me_ptr: *mut Self = memory::calloc(1); - // SAFETY: We just allocated this, and we're putting a valid pointer in. - unsafe { - let me = &mut *me_ptr; - me.base = questions; - } - me_ptr +impl XSsoQuestions { + fn len(&self) -> usize { + self.questions.len() + } + fn iter_mut(&mut self) -> impl Iterator<Item = &mut Question> { + self.questions.iter_mut() + } +} + +impl QuestionsTrait for XSsoQuestions { + fn new(messages: &[Message]) -> Result<Self> { + let questions: Result<Vec<_>> = messages.iter().map(Question::try_from).collect(); + let questions = questions?; + Ok(Self { + pointer: questions.as_ptr(), + questions, + _marker: Default::default(), + }) } - unsafe fn iter(&self, count: usize) -> impl Iterator<Item = &Question> { - (0..count).map(|idx| &*self.base.add(idx)) - } - - unsafe fn iter_mut(&mut self, count: usize) -> impl Iterator<Item = &mut Question> { - (0..count).map(|idx| &mut *self.base.add(idx)) + fn ptr(&self) -> *const *const Question { + &self.pointer as *const *const Question } - unsafe fn free_contents(&mut self, count: usize) { - let msgs = slice::from_raw_parts_mut(self.base, count); - for msg in msgs { - msg.clear() - } - memory::free(self.base); - self.base = ptr::null_mut() + unsafe fn borrow_ptr<'a>( + ptr: *const *const Question, + count: usize, + ) -> impl Iterator<Item = &'a Question> { + slice::from_raw_parts(*ptr, count).iter() } } -/// The Linux version of the indirection layer between Question and Questions. +/// The Linux version of the pointer train to questions. #[derive(Debug)] #[repr(C)] -pub struct LinuxPamIndirect { - base: [*mut Question; 0], +pub struct LinuxPamQuestions { + #[allow(clippy::vec_box)] // we need to do this. + /// The place where the questions are. + questions: Vec<Box<Question>>, _marker: Immovable, } -impl IndirectTrait for LinuxPamIndirect { - fn alloc(count: usize) -> *mut Self { - // SAFETY: We're only allocating, and when we're done, - // everything will be in a known-good state. - let me_ptr: *mut Self = memory::calloc::<*mut Question>(count).cast(); - unsafe { - let me = &mut *me_ptr; - let ptr_list = slice::from_raw_parts_mut(me.base.as_mut_ptr(), count); - for entry in ptr_list { - *entry = memory::calloc(1); - } - } - me_ptr +impl LinuxPamQuestions { + fn len(&self) -> usize { + self.questions.len() } - unsafe fn iter(&self, count: usize) -> impl Iterator<Item = &Question> { - (0..count).map(|idx| &**self.base.as_ptr().add(idx)) + fn iter_mut(&mut self) -> impl Iterator<Item = &mut Question> { + self.questions.iter_mut().map(AsMut::as_mut) } +} - unsafe fn iter_mut(&mut self, count: usize) -> impl Iterator<Item = &mut Question> { - (0..count).map(|idx| &mut **self.base.as_mut_ptr().add(idx)) +impl QuestionsTrait for LinuxPamQuestions { + fn new(messages: &[Message]) -> Result<Self> { + let questions: Result<_> = messages + .iter() + .map(|msg| Question::try_from(msg).map(Box::new)) + .collect(); + Ok(Self { + questions: questions?, + _marker: Default::default(), + }) } - unsafe fn free_contents(&mut self, count: usize) { - let msgs = slice::from_raw_parts_mut(self.base.as_mut_ptr(), count); - for msg in msgs { - if let Some(msg) = msg.as_mut() { - msg.clear(); - } - memory::free(*msg); - *msg = ptr::null_mut(); - } + fn ptr(&self) -> *const *const Question { + self.questions.as_ptr().cast() + } + + unsafe fn borrow_ptr<'a>( + ptr: *const *const Question, + count: usize, + ) -> impl Iterator<Item = &'a Question> { + slice::from_raw_parts(ptr.cast::<&Question>(), count) + .iter() + .copied() } } /// The C enum values for messages shown to the user. #[derive(Debug, PartialEq, TryFromPrimitive, IntoPrimitive)] #[repr(u32)] -pub enum Style { +enum Style { /// Requests information from the user; will be masked when typing. PromptEchoOff = pam_ffi::PAM_PROMPT_ECHO_OFF, /// Requests information from the user; will not be masked. @@ -270,47 +196,7 @@ BinaryPrompt = pam_ffi::PAM_BINARY_PROMPT, } -impl Default for Question { - fn default() -> Self { - Self { - style: Default::default(), - data: ptr::null_mut(), - _marker: Default::default(), - } - } -} - impl Question { - /// Replaces the contents of this question with the question - /// from the message. - /// - /// If the message is not valid (invalid message type, bad contents, etc.), - /// this will fail. - pub fn try_fill(&mut self, msg: &Message) -> Result<()> { - let alloc = |style, text| Ok((style, memory::malloc_str(text)?.cast())); - // We will only allocate heap data if we have a valid input. - let (style, data): (_, *mut c_void) = match *msg { - Message::MaskedPrompt(p) => alloc(Style::PromptEchoOff, p.question()), - Message::Prompt(p) => alloc(Style::PromptEchoOn, p.question()), - Message::Error(p) => alloc(Style::ErrorMsg, p.question()), - Message::Info(p) => alloc(Style::TextInfo, p.question()), - #[cfg(feature = "linux-pam-extensions")] - Message::RadioPrompt(p) => alloc(Style::RadioType, p.question()), - #[cfg(feature = "linux-pam-extensions")] - Message::BinaryPrompt(p) => Ok(( - Style::BinaryPrompt, - CBinaryData::alloc(p.question())?.cast(), - )), - #[cfg(not(feature = "linux-pam-extensions"))] - Message::RadioPrompt(_) | Message::BinaryPrompt(_) => Err(ErrorCode::ConversationError), - }?; - // Now that we know everything is valid, fill ourselves in. - self.clear(); - self.style = style.into(); - self.data = data; - Ok(()) - } - /// Gets this message's data pointer as a string. /// /// # Safety @@ -334,9 +220,38 @@ .map(Into::into) .unwrap_or_default() } +} - /// Zeroes out the data stored here. - fn clear(&mut self) { +impl TryFrom<&Message<'_>> for Question { + type Error = ErrorCode; + fn try_from(msg: &Message) -> Result<Self> { + let alloc = |style, text| Ok((style, memory::malloc_str(text)?.cast())); + // We will only allocate heap data if we have a valid input. + let (style, data): (_, *mut c_void) = match *msg { + Message::MaskedPrompt(p) => alloc(Style::PromptEchoOff, p.question()), + Message::Prompt(p) => alloc(Style::PromptEchoOn, p.question()), + Message::Error(p) => alloc(Style::ErrorMsg, p.question()), + Message::Info(p) => alloc(Style::TextInfo, p.question()), + #[cfg(feature = "linux-pam-extensions")] + Message::RadioPrompt(p) => alloc(Style::RadioType, p.question()), + #[cfg(feature = "linux-pam-extensions")] + Message::BinaryPrompt(p) => Ok(( + Style::BinaryPrompt, + CBinaryData::alloc(p.question())?.cast(), + )), + #[cfg(not(feature = "linux-pam-extensions"))] + Message::RadioPrompt(_) | Message::BinaryPrompt(_) => Err(ErrorCode::ConversationError), + }?; + Ok(Self { + style: style.into(), + data, + _marker: Default::default(), + }) + } +} + +impl Drop for Question { + fn drop(&mut self) { // SAFETY: We either created this data or we got it from PAM. // After this function is done, it will be zeroed out. unsafe { @@ -409,7 +324,7 @@ use super::super::*; #[test] fn standard() { - let interrogation = GenericQuestions::<$typ>::new(&[ + let interrogation = <$typ>::new(&[ MaskedQAndA::new("hocus pocus").message(), QAndA::new("what").message(), QAndA::new("who").message(), @@ -417,10 +332,10 @@ ErrorMsg::new("gasp").message(), ]) .unwrap(); - let indirect = interrogation.indirect(); + let indirect = interrogation.ptr(); - let remade = unsafe { $typ::borrow_ptr(indirect) }.unwrap(); - let messages: Vec<OwnedMessage> = unsafe { remade.iter(interrogation.count) } + let remade = unsafe { $typ::borrow_ptr(indirect, interrogation.len()) }; + let messages: Vec<OwnedMessage> = remade .map(TryInto::try_into) .collect::<Result<_>>() .unwrap(); @@ -436,7 +351,7 @@ #[cfg(not(feature = "linux-pam-extensions"))] fn no_linux_extensions() { use crate::conv::{BinaryQAndA, RadioQAndA}; - GenericQuestions::<$typ>::new(&[ + <$typ>::new(&[ BinaryQAndA::new((&[5, 4, 3, 2, 1], 66)).message(), RadioQAndA::new("you must choose").message(), ]).unwrap_err(); @@ -445,14 +360,14 @@ #[test] #[cfg(feature = "linux-pam-extensions")] fn linux_extensions() { - let interrogation = GenericQuestions::<$typ>::new(&[ + let interrogation = <$typ>::new(&[ BinaryQAndA::new((&[5, 4, 3, 2, 1], 66)).message(), RadioQAndA::new("you must choose").message(), ]).unwrap(); - let indirect = interrogation.indirect(); + let indirect = interrogation.ptr(); - let remade = unsafe { $typ::borrow_ptr(indirect) }.unwrap(); - let messages: Vec<OwnedMessage> = unsafe { remade.iter(interrogation.count) } + let remade = unsafe { $typ::borrow_ptr(indirect, interrogation.len()) }; + let messages: Vec<OwnedMessage> = unsafe { remade } .map(TryInto::try_into) .collect::<Result<_>>() .unwrap(); @@ -463,6 +378,6 @@ } }} - tests!(test_xsso<StandardIndirect>); - tests!(test_linux<LinuxPamIndirect>); + tests!(test_xsso<XSsoQuestions>); + tests!(test_linux<LinuxPamQuestions>); }