changeset 59:3f4a77aa88be

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.
author Paul Fisher <paul@pfish.zone>
date Wed, 21 May 2025 00:27:18 -0400
parents 868a278a362c
children 05cc2c27334f
files src/constants.rs src/conv.rs src/items.rs src/macros.rs src/module.rs
diffstat 5 files changed, 84 insertions(+), 35 deletions(-) [+]
line wrap: on
line diff
--- 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<c_int> for MessageStyle {
+    type Error = InvalidEnum<Self>;
+    fn try_from(value: c_int) -> Result<Self, Self::Error> {
+        Self::from_i32(value).ok_or(value.into())
+    }
+}
+
 impl From<MessageStyle> 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<c_int> for ErrorCode {
+    type Error = InvalidEnum<Self>;
+
+    fn try_from(value: c_int) -> Result<Self, Self::Error> {
+        Self::from_i32(value).ok_or(value.into())
+    }
+}
+
 impl From<ErrorCode> 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::<T>())]
+#[derive(Debug, PartialEq)]
+pub struct InvalidEnum<T>(std::ffi::c_int, PhantomData<T>);
+
+impl<T> From<std::ffi::c_int> for InvalidEnum<T> {
+    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));
+    }
+}
--- 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)?;
 
--- 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<c_int> for ItemType {
-    fn from(value: c_int) -> Self {
-        Self::from_i32(value).unwrap_or(Self::Unset)
+impl TryFrom<c_int> for ItemType {
+    type Error = InvalidEnum<Self>;
+    fn try_from(value: c_int) -> Result<Self, Self::Error> {
+        Self::from_i32(value).ok_or(value.into())
     }
 }
 
 impl From<ItemType> 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);
--- 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> {
--- 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<T>(_: *const PamHandle, c_data: *mut libc::c_void, _: c_int) {
     unsafe {
-        let _data: Box<T> = Box::from_raw(c_data.cast::<T>());
+        let _data: Box<T> = Box::from_raw(c_data.cast());
     }
 }
 
@@ -88,7 +92,7 @@
         match ptr.is_null() {
             true => Ok(None),
             false => {
-                let typed_ptr = ptr.cast::<T>();
+                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::<libc::c_void>(),
+                Box::into_raw(data).cast(),
                 cleanup::<T>,
             )
         };
@@ -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::<T::Raw>();
+            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<T: Item>(&mut self, item: T) -> PamResult<()> {
-        let ret =
-            unsafe { pam_set_item(self, T::type_id().into(), item.into_raw().cast::<libc::c_void>()) };
+        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<String> {
         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<SecureString> {
         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()),
             )
         };