# HG changeset patch # User Paul Fisher # Date 1747801638 14400 # Node ID 3f4a77aa88be6b20ddb858cfed2d47d6a8e207e8 # Parent 868a278a362c15b323e0288c836e06ff1732604e Fix string copyting and improve error situation. This change is too big and includes several things: - Fix copying strings from PAM by fixing const and mut on pam funcs. - Improve error enums by simplifying conversions and removing unnecessary and ambiguous "success" variants. - Make a bunch of casts nicer. - Assorted other cleanup. diff -r 868a278a362c -r 3f4a77aa88be src/constants.rs --- a/src/constants.rs Mon May 05 00:16:04 2025 -0400 +++ b/src/constants.rs Wed May 21 00:27:18 2025 -0400 @@ -1,7 +1,9 @@ use bitflags::bitflags; use libc::{c_int, c_uint}; -use num_derive::{FromPrimitive, ToPrimitive}; -use num_traits::{FromPrimitive, ToPrimitive}; +use num_derive::FromPrimitive; +use num_traits::FromPrimitive; +use std::any; +use std::marker::PhantomData; // TODO: Import constants from C header file at compile time. // The Linux-PAM flags @@ -21,7 +23,7 @@ } /// Styles of message that are shown to the user. -#[derive(Debug, PartialEq, FromPrimitive, ToPrimitive)] +#[derive(Debug, PartialEq, FromPrimitive)] #[non_exhaustive] // non-exhaustive because C might give us back anything! pub enum MessageStyle { /// Requests information from the user; will be masked when typing. @@ -39,9 +41,16 @@ BinaryPrompt = 7, } +impl TryFrom for MessageStyle { + type Error = InvalidEnum; + fn try_from(value: c_int) -> Result { + Self::from_i32(value).ok_or(value.into()) + } +} + impl From for c_int { fn from(val: MessageStyle) -> Self { - val.to_i32().unwrap_or(0) + val as Self } } @@ -51,7 +60,7 @@ /// For more detailed information, see /// `/usr/include/security/_pam_types.h`. #[allow(non_camel_case_types, dead_code)] -#[derive(Copy, Clone, Debug, PartialEq, thiserror::Error, FromPrimitive, ToPrimitive)] +#[derive(Copy, Clone, Debug, PartialEq, thiserror::Error, FromPrimitive)] #[non_exhaustive] // C might give us anything! pub enum ErrorCode { #[error("dlopen() failure when dynamically loading a service module")] @@ -129,16 +138,51 @@ } /// Converts a C result code into a PamResult, with success as Ok. + /// Invalid values are returned as a [Self::SystemError]. pub fn result_from(value: c_int) -> PamResult<()> { match value { 0 => Ok(()), - value => Err(Self::from_i64(value as i64).unwrap_or(Self::ConversationError)) + value => Err(value.try_into().unwrap_or(Self::SystemError)), } } } +impl TryFrom for ErrorCode { + type Error = InvalidEnum; + + fn try_from(value: c_int) -> Result { + Self::from_i32(value).ok_or(value.into()) + } +} + impl From for c_int { fn from(val: ErrorCode) -> Self { - val.to_i32().unwrap_or(0) + val as Self + } +} + +/// Error returned when attempting to coerce an invalid C integer into an enum. +#[derive(thiserror::Error)] +#[error("{} is not a valid {}", .0, any::type_name::())] +#[derive(Debug, PartialEq)] +pub struct InvalidEnum(std::ffi::c_int, PhantomData); + +impl From for InvalidEnum { + fn from(value: std::ffi::c_int) -> Self { + Self(value, PhantomData) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_enums() { + assert_eq!(Ok(MessageStyle::ErrorMsg), 3.try_into()); + assert_eq!(Err(999.into()), ErrorCode::try_from(999)); + assert_eq!(Ok(()), ErrorCode::result_from(0)); + assert_eq!(Err(ErrorCode::Abort), ErrorCode::result_from(26)); + assert_eq!(Err(ErrorCode::SystemError), ErrorCode::result_from(423)); + } +} diff -r 868a278a362c -r 3f4a77aa88be src/conv.rs --- a/src/conv.rs Mon May 05 00:16:04 2025 -0400 +++ b/src/conv.rs Wed May 21 00:27:18 2025 -0400 @@ -2,9 +2,9 @@ use std::ffi::{CStr, CString}; use std::ptr; +use crate::constants::ErrorCode; use crate::constants::MessageStyle; use crate::constants::PamResult; -use crate::constants::ErrorCode; use crate::items::Item; #[repr(C)] @@ -60,6 +60,7 @@ msg_style: style, msg: msg_cstr.as_ptr(), }; + // TODO: These need to be freed! let ret = (self.0.conv)(1, &&msg, &mut resp_ptr, self.0.appdata_ptr); ErrorCode::result_from(ret)?; diff -r 868a278a362c -r 3f4a77aa88be src/items.rs --- a/src/items.rs Mon May 05 00:16:04 2025 -0400 +++ b/src/items.rs Wed May 21 00:27:18 2025 -0400 @@ -1,12 +1,12 @@ -use num_derive::{FromPrimitive, ToPrimitive}; -use num_traits::{FromPrimitive, ToPrimitive}; +use crate::constants::InvalidEnum; +use num_derive::FromPrimitive; +use num_traits::FromPrimitive; use std::ffi::{c_int, CStr}; -#[derive(FromPrimitive, ToPrimitive)] +#[derive(FromPrimitive)] #[repr(i32)] +#[non_exhaustive] // because C could give us anything! pub enum ItemType { - /// Unset. This should never be used. - Unset = 0, /// The service name Service = 1, /// The user name @@ -35,19 +35,20 @@ AuthTokType = 13, } -impl From for ItemType { - fn from(value: c_int) -> Self { - Self::from_i32(value).unwrap_or(Self::Unset) +impl TryFrom for ItemType { + type Error = InvalidEnum; + fn try_from(value: c_int) -> Result { + Self::from_i32(value).ok_or(value.into()) } } impl From for c_int { fn from(val: ItemType) -> Self { - val.to_i32().unwrap_or(0) + val as Self } } -// A type that can be requested by `pam::Handle::get_item`. +/// A type that can be requested by [crate::Handle::get_item]. pub trait Item { /// The `repr(C)` type that is returned (by pointer) by the underlying `pam_get_item` function. type Raw; @@ -101,7 +102,7 @@ cstr_item!(User); cstr_item!(Tty); cstr_item!(RemoteHost); -// Conv +// Conversation is not included here since it's special. cstr_item!(AuthTok); cstr_item!(OldAuthTok); cstr_item!(RemoteUser); diff -r 868a278a362c -r 3f4a77aa88be src/macros.rs --- a/src/macros.rs Mon May 05 00:16:04 2025 -0400 +++ b/src/macros.rs Wed May 21 00:27:18 2025 -0400 @@ -36,7 +36,7 @@ mod pam_hooks_scope { use std::ffi::CStr; use std::os::raw::{c_char, c_int}; - use $crate::constants::{Flags, ErrorCode}; + use $crate::constants::{ErrorCode, Flags}; use $crate::module::{PamHandle, PamHooks}; fn extract_argv<'a>(argc: c_int, argv: *const *const c_char) -> Vec<&'a CStr> { diff -r 868a278a362c -r 3f4a77aa88be src/module.rs --- a/src/module.rs Mon May 05 00:16:04 2025 -0400 +++ b/src/module.rs Wed May 21 00:27:18 2025 -0400 @@ -1,10 +1,10 @@ //! Functions for use in pam modules. -use crate::constants::{Flags, PamResult, ErrorCode}; +use crate::constants::{ErrorCode, Flags, PamResult}; use crate::items::{Item, ItemType}; use libc::c_char; +use secure_string::SecureString; use std::ffi::{c_int, CStr, CString}; -use secure_string::SecureString; /// Opaque type, used as a pointer when making pam API calls. /// @@ -27,7 +27,7 @@ fn pam_set_data( pamh: *const PamHandle, module_data_name: *const c_char, - data: *mut libc::c_void, + data: *const libc::c_void, cleanup: extern "C" fn( pamh: *const PamHandle, data: *mut libc::c_void, @@ -43,12 +43,16 @@ fn pam_set_item(pamh: *mut PamHandle, item_type: c_int, item: *const libc::c_void) -> c_int; - fn pam_get_user(pamh: *const PamHandle, user: &*mut c_char, prompt: *const c_char) -> c_int; + fn pam_get_user( + pamh: *const PamHandle, + user: &mut *const c_char, + prompt: *const c_char, + ) -> c_int; fn pam_get_authtok( pamh: *const PamHandle, item_type: c_int, - data: &*mut c_char, + data: &mut *const c_char, prompt: *const c_char, ) -> c_int; @@ -60,7 +64,7 @@ /// You should never call this yourself. extern "C" fn cleanup(_: *const PamHandle, c_data: *mut libc::c_void, _: c_int) { unsafe { - let _data: Box = Box::from_raw(c_data.cast::()); + let _data: Box = Box::from_raw(c_data.cast()); } } @@ -88,7 +92,7 @@ match ptr.is_null() { true => Ok(None), false => { - let typed_ptr = ptr.cast::(); + let typed_ptr = ptr.cast(); Ok(Some(&*typed_ptr)) } } @@ -109,7 +113,7 @@ pam_set_data( self, c_key.as_ptr(), - Box::into_raw(data).cast::(), + Box::into_raw(data).cast(), cleanup::, ) }; @@ -133,7 +137,7 @@ let out = unsafe { let ret = pam_get_item(self, T::type_id().into(), &mut ptr); ErrorCode::result_from(ret)?; - let typed_ptr = ptr.cast::(); + let typed_ptr: *const T::Raw = ptr.cast(); match typed_ptr.is_null() { true => None, false => Some(T::from_raw(typed_ptr)), @@ -151,8 +155,7 @@ /// /// Returns an error if the underlying PAM function call fails. pub fn set_item(&mut self, item: T) -> PamResult<()> { - let ret = - unsafe { pam_set_item(self, T::type_id().into(), item.into_raw().cast::()) }; + let ret = unsafe { pam_set_item(self, T::type_id().into(), item.into_raw().cast()) }; ErrorCode::result_from(ret) } @@ -168,8 +171,8 @@ /// Returns an error if the underlying PAM function call fails. pub fn get_user(&self, prompt: Option<&str>) -> PamResult { let prompt = option_cstr(prompt)?; - let output: *mut c_char = std::ptr::null_mut(); - let ret = unsafe { pam_get_user(self, &output, prompt_ptr(prompt.as_ref())) }; + let mut output: *const c_char = std::ptr::null_mut(); + let ret = unsafe { pam_get_user(self, &mut output, prompt_ptr(prompt.as_ref())) }; ErrorCode::result_from(ret)?; copy_pam_string(output) } @@ -186,12 +189,12 @@ /// Returns an error if the underlying PAM function call fails. pub fn get_authtok(&self, prompt: Option<&str>) -> PamResult { let prompt = option_cstr(prompt)?; - let output: *mut c_char = std::ptr::null_mut(); + let mut output: *const c_char = std::ptr::null_mut(); let res = unsafe { pam_get_authtok( self, ItemType::AuthTok.into(), - &output, + &mut output, prompt_ptr(prompt.as_ref()), ) };