[FEAT] Rustify wrapped API and remove some transmutes

src/auth.rs

@@ -1,8 +1,8 @@
 use users;
 
-use std::{env, ptr};
+use std::env;
 
-use crate::{conv, wrapped::*, PamError, PamFlag, PamHandle, PamResult, PamReturnCode};
+use crate::{conv, wrapped::*, PamFlag, PamHandle, PamResult, PamReturnCode};
 
 /// Main struct to authenticate a user
 ///
@@ -17,7 +17,7 @@ use crate::{conv, wrapped::*, PamError, PamFlag, PamHandle, PamResult, PamReturn
 ///         .expect("Failed to init PAM client.");
 /// // Preset the login & password we will use for authentication
 /// authenticator.handler_mut().set_credentials("login", "password");
-/// // actually try to authenticate:
+/// // Actually try to authenticate:
 /// authenticator.authenticate().expect("Authentication failed!");
 /// // Now that we are authenticated, it's possible to open a sesssion:
 /// authenticator.open_session().expect("Failed to open a session!");
@@ -52,21 +52,16 @@ impl<'a, C: conv::Conversation> Authenticator<'a, C> {
     pub fn with_handler(service: &str, conversation: C) -> PamResult<Authenticator<'a, C>> {
         let mut conversation = Box::new(conversation);
         let conv = conv::into_pam_conv(&mut *conversation);
-        let mut handle: *mut PamHandle = ptr::null_mut();
-
-        match start(service, None, &conv, &mut handle) {
-            PamReturnCode::SUCCESS => unsafe {
-                Ok(Authenticator {
-                    close_on_drop: true,
-                    handle: &mut *handle,
-                    conversation,
-                    is_authenticated: false,
-                    has_open_session: false,
-                    last_code: PamReturnCode::SUCCESS,
-                })
-            },
-            code => Err(PamError(code)),
-        }
+
+        let handle = start(service, None, &conv)?;
+        Ok(Authenticator {
+            close_on_drop: true,
+            handle,
+            conversation,
+            is_authenticated: false,
+            has_open_session: false,
+            last_code: PamReturnCode::SUCCESS,
+        })
     }
 
     /// Mutable access to the conversation handler of this Authenticator

src/conv.rs

@@ -87,7 +87,7 @@ pub(crate) fn into_pam_conv<C: Conversation>(conv: &mut C) -> pam_conv {
 }
 
 // FIXME: verify this
-pub unsafe extern "C" fn converse<C: Conversation>(
+pub(crate) unsafe extern "C" fn converse<C: Conversation>(
     num_msg: c_int,
     msg: *mut *const PamMessage,
     out_resp: *mut *mut PamResponse,
@@ -105,7 +105,8 @@ pub unsafe extern "C" fn converse<C: Conversation>(
     let mut result: PamReturnCode = PamReturnCode::SUCCESS;
     for i in 0..num_msg as isize {
         // get indexed values
-        let m: &mut PamMessage = &mut mem::transmute(**(msg.offset(i)));
+        // FIXME: check this
+        let m: &mut PamMessage = &mut *(*(msg.offset(i)) as *mut PamMessage);
         let r: &mut PamResponse = &mut *(resp.offset(i));
         let msg = CStr::from_ptr(m.msg);
         // match on msg_style

src/lib.rs

@@ -1,5 +1,7 @@
 // TODO: only for now
 #![allow(non_camel_case_types)]
+// We want to pass PamHandles by ref as they are opaque
+#![allow(clippy::trivially_copy_pass_by_ref)]
 
 mod conv;
 mod enums;

src/wrapped.rs

@@ -11,126 +11,116 @@
 use libc::{c_char, c_int, c_void};
 
 use std::ffi::{CStr, CString};
-use std::ptr::null;
 
 use pam_sys as ffi;
 
-use crate::{PamFlag, PamItemType, PamReturnCode};
+use crate::{PamFlag, PamHandle, PamItemType, PamResult, PamReturnCode};
 
-/* ------------------------ ffi::pam_appl.h -------------------------- */
+/* ------------------------ <security/pam_appl.h> -------------------------- */
 #[inline]
-pub fn start(
+pub fn start<'a>(
     service: &str,
     user: Option<&str>,
     conversation: &ffi::pam_conv,
-    handle: *mut *mut ffi::pam_handle_t,
-) -> PamReturnCode {
+) -> PamResult<&'a mut PamHandle> {
     if let Ok(service) = CString::new(service) {
-        if let Some(usr) = user {
-            if let Ok(user) = CString::new(usr) {
-                unsafe {
-                    From::from(ffi::pam_start(
-                        service.as_ptr(),
-                        user.as_ptr(),
-                        conversation,
-                        handle as *mut *mut ffi::pam_handle_t,
-                    ))
-                }
-            } else {
-                PamReturnCode::BUF_ERR
-            }
-        } else {
-            unsafe {
-                From::from(ffi::pam_start(
-                    service.as_ptr(),
-                    null(),
-                    conversation,
-                    handle as *mut *mut ffi::pam_handle_t,
-                ))
+        // Only service is required -> initialize handle
+        let mut handle: *mut PamHandle = std::ptr::null_mut();
+
+        let user_ptr = try_str_option_to_ptr(user)?;
+        match unsafe { ffi::pam_start(service.as_ptr(), user_ptr, conversation, &mut handle) }
+            .into()
+        {
+            // Reborrow is safe, because we
+            PamReturnCode::SUCCESS => {
+                assert!(
+                    !handle.is_null(),
+                    "Got PAM_SUCCESS from pam_start but handle is null!"
+                );
+                Ok(unsafe { &mut *handle })
             }
+            err => Err(err.into()),
         }
     } else {
-        PamReturnCode::SERVICE_ERR
+        // Invalid service
+        Err(PamReturnCode::BUF_ERR.into())
     }
 }
 
 #[inline]
-pub fn end(handle: &mut ffi::pam_handle_t, status: PamReturnCode) -> PamReturnCode {
-    From::from(unsafe { ffi::pam_end(handle, status as c_int) })
+pub fn end(handle: &mut PamHandle, status: PamReturnCode) -> PamReturnCode {
+    unsafe { ffi::pam_end(handle, status as c_int) }.into()
 }
 
 #[inline]
-pub fn authenticate(handle: &mut ffi::pam_handle_t, flags: PamFlag) -> PamReturnCode {
-    From::from(unsafe { ffi::pam_authenticate(handle, flags as c_int) })
+pub fn authenticate(handle: &mut PamHandle, flags: PamFlag) -> PamReturnCode {
+    unsafe { ffi::pam_authenticate(handle, flags as c_int) }.into()
 }
 
 #[inline]
-pub fn setcred(handle: &mut ffi::pam_handle_t, flags: PamFlag) -> PamReturnCode {
-    From::from(unsafe { ffi::pam_setcred(handle, flags as c_int) })
+pub fn setcred(handle: &mut PamHandle, flags: PamFlag) -> PamReturnCode {
+    unsafe { ffi::pam_setcred(handle, flags as c_int) }.into()
 }
 
 #[inline]
-pub fn acct_mgmt(handle: &mut ffi::pam_handle_t, flags: PamFlag) -> PamReturnCode {
-    From::from(unsafe { ffi::pam_acct_mgmt(handle, flags as c_int) })
+pub fn acct_mgmt(handle: &mut PamHandle, flags: PamFlag) -> PamReturnCode {
+    unsafe { ffi::pam_acct_mgmt(handle, flags as c_int) }.into()
 }
 
 #[inline]
-pub fn open_session(handle: &mut ffi::pam_handle_t, flags: PamFlag) -> PamReturnCode {
-    From::from(unsafe { ffi::pam_open_session(handle, flags as c_int) })
+pub fn open_session(handle: &mut PamHandle, flags: PamFlag) -> PamReturnCode {
+    unsafe { ffi::pam_open_session(handle, flags as c_int) }.into()
 }
 
 #[inline]
-pub fn close_session(handle: &mut ffi::pam_handle_t, flags: PamFlag) -> PamReturnCode {
-    From::from(unsafe { ffi::pam_close_session(handle, flags as c_int) })
+pub fn close_session(handle: &mut PamHandle, flags: PamFlag) -> PamReturnCode {
+    unsafe { ffi::pam_close_session(handle, flags as c_int) }.into()
 }
 
 #[inline]
-pub fn chauthtok(handle: &mut ffi::pam_handle_t, flags: PamFlag) -> PamReturnCode {
-    From::from(unsafe { ffi::pam_chauthtok(handle, flags as c_int) })
+pub fn chauthtok(handle: &mut PamHandle, flags: PamFlag) -> PamReturnCode {
+    unsafe { ffi::pam_chauthtok(handle, flags as c_int) }.into()
 }
-/* ------------------------ ffi::pam_appl.h -------------------------- */
+/* ------------------------ <security/pam_appl.h> -------------------------- */
 
-/* ----------------------- _pam_types.h ------------------------- */
+/* ----------------------- <security/_pam_types.h> ------------------------- */
 #[inline]
-pub fn set_item(
-    handle: &mut ffi::pam_handle_t,
-    item_type: PamItemType,
-    item: &c_void,
-) -> PamReturnCode {
-    From::from(unsafe { ffi::pam_set_item(handle, item_type as c_int, item) })
+pub fn set_item(handle: &mut PamHandle, item_type: PamItemType, item: &c_void) -> PamReturnCode {
+    unsafe { ffi::pam_set_item(handle, item_type as c_int, item) }.into()
 }
 
 #[inline]
-pub fn get_item(
-    handle: &ffi::pam_handle_t,
-    item_type: PamItemType,
-    item: &mut *const c_void,
-) -> PamReturnCode {
-    From::from(unsafe { ffi::pam_get_item(handle, item_type as c_int, item) })
+pub fn get_item<'a>(handle: &PamHandle, item_type: PamItemType) -> PamResult<&'a c_void> {
+    let mut item_ptr: *const c_void = std::ptr::null();
+    match unsafe { ffi::pam_get_item(handle, item_type as c_int, &mut item_ptr) }.into() {
+        PamReturnCode::SUCCESS => Ok(unsafe { &*item_ptr }),
+        err => Err(err.into()),
+    }
 }
 
 #[inline]
-pub fn strerror(handle: &mut ffi::pam_handle_t, errnum: PamReturnCode) -> Option<&str> {
+pub fn strerror(handle: &mut PamHandle, errnum: PamReturnCode) -> &str {
     unsafe { CStr::from_ptr(ffi::pam_strerror(handle, errnum as c_int)) }
         .to_str()
-        .ok()
+        .expect("Got invalid UTF8 string from pam_strerror")
 }
 
 #[inline]
-pub fn putenv(handle: &mut ffi::pam_handle_t, name_value: &str) -> PamReturnCode {
+pub fn putenv(handle: &mut PamHandle, name_value: &str) -> PamReturnCode {
     if let Ok(name_value) = CString::new(name_value) {
-        From::from(unsafe { ffi::pam_putenv(handle, name_value.as_ptr()) })
+        unsafe { ffi::pam_putenv(handle, name_value.as_ptr()) }.into()
     } else {
-        // Not sure whether this is the correct return value
         PamReturnCode::BUF_ERR
     }
 }
 
 #[inline]
-pub fn getenv<'a>(handle: &'a mut ffi::pam_handle_t, name: &str) -> Option<&'a str> {
+pub fn getenv<'a>(handle: &'a mut PamHandle, name: &str) -> Option<&'a str> {
     if let Ok(name) = CString::new(name) {
+        // Get environment variable
         let env = unsafe { ffi::pam_getenv(handle, name.as_ptr()) };
         if !env.is_null() {
+            // Convert to rust &str
             unsafe { CStr::from_ptr(env) }.to_str().ok()
         } else {
             None
@@ -141,16 +131,16 @@ pub fn getenv<'a>(handle: &'a mut ffi::pam_handle_t, name: &str) -> Option<&'a s
 }
 
 /*#[inline]
-pub fn getenvlist(handle: &mut ffi::pam_handle_t) -> *const *const c_char {
+pub fn getenvlist(handle: &mut PamHandle) -> *const *const c_char {
     //TODO: find a convenient way to handle this with Rust types
     unsafe { ffi::pam_getenvlist(handle) }
 }*/
-/* ----------------------- _pam_types.h ------------------------- */
+/* ----------------------- <security/_pam_types.h> ------------------------- */
 
-/* ----------------------- ffi::pam_misc.h --------------------------- */
+/* ----------------------- <security/pam_misc.h> --------------------------- */
 #[inline]
 #[cfg(target_os = "linux")]
-pub fn misc_paste_env(handle: &mut ffi::pam_handle_t, user_env: &[&str]) -> PamReturnCode {
+pub fn misc_paste_env(handle: &mut PamHandle, user_env: &[&str]) -> PamReturnCode {
     // Taken from: https://github.com/rust-lang/rust/issues/9564#issuecomment-95354558
     let user_env: Vec<_> = user_env
         .iter()
@@ -159,70 +149,94 @@ pub fn misc_paste_env(handle: &mut ffi::pam_handle_t, user_env: &[&str]) -> PamR
     let env_ptrs: Vec<_> = user_env
         .iter()
         .map(|env| env.as_ptr())
-        .chain(Some(null()))
+        .chain(Some(std::ptr::null()))
         .collect();
-    From::from(unsafe { ffi::pam_misc_paste_env(handle, env_ptrs.as_ptr()) })
+    unsafe { ffi::pam_misc_paste_env(handle, env_ptrs.as_ptr()) }.into()
 }
 
 /*#[inline]
 #[cfg(target_os = "linux")]
 pub fn misc_drop_env(env: &mut *mut c_char) -> PamReturnCode {
-    From::from(unsafe { ffi::pam_misc_drop_env(env) })
+    unsafe { ffi::pam_misc_drop_env(env) })
 }*/
 
 #[inline]
 #[cfg(target_os = "linux")]
 pub fn misc_setenv(
-    handle: &mut ffi::pam_handle_t,
+    handle: &mut PamHandle,
     name: &str,
     value: &str,
     readonly: bool,
 ) -> PamReturnCode {
     if let (Ok(name), Ok(value)) = (CString::new(name), CString::new(value)) {
-        From::from(unsafe {
+        unsafe {
             ffi::pam_misc_setenv(
                 handle,
                 name.as_ptr(),
                 value.as_ptr(),
                 if readonly { 0 } else { 1 },
             )
-        })
+        }
+        .into()
     } else {
         PamReturnCode::BUF_ERR
     }
 }
-/* ----------------------- ffi::pam_misc.h --------------------------- */
+/* ----------------------- <security/pam_misc.h> --------------------------- */
 
-/* ----------------------- ffi::pam_modules.h ------------------------ */
+/* ----------------------- <security/pam_modules.h> ------------------------ */
 #[inline]
 pub fn set_data(
-    handle: &mut ffi::pam_handle_t,
+    handle: &mut PamHandle,
     module_data_name: &str,
     data: &mut c_void,
-    cleanup: Option<unsafe extern "C" fn(*mut ffi::pam_handle_t, *mut c_void, c_int)>,
+    // FIXME: Remove bare ptrs from closure signature
+    cleanup: Option<unsafe extern "C" fn(*mut PamHandle, *mut c_void, c_int)>,
 ) -> PamReturnCode {
     if let Ok(module_data_name) = CString::new(module_data_name) {
-        From::from(unsafe { ffi::pam_set_data(handle, module_data_name.as_ptr(), data, cleanup) })
+        unsafe { ffi::pam_set_data(handle, module_data_name.as_ptr(), data, cleanup) }.into()
     } else {
         PamReturnCode::BUF_ERR
     }
 }
 
-//pub fn get_data(handle: *const ffi::pam_handle_t, module_data_name: *const c_char, data: *const *const c_void);
+//pub fn get_data(handle: *const PamHandle, module_data_name: *const c_char, data: *const *const c_void);
 
-pub fn get_user(
-    handle: &ffi::pam_handle_t,
-    user: &mut *const c_char,
-    prompt: Option<&CStr>,
-) -> PamReturnCode {
-    From::from(unsafe {
+#[inline]
+pub fn get_user<'a>(handle: &'a PamHandle, prompt: Option<&str>) -> PamResult<&'a str> {
+    let mut user_ptr: *const c_char = std::ptr::null();
+    let prompt_ptr = try_str_option_to_ptr(prompt)?;
+    match unsafe {
         ffi::pam_get_user(
-            // Bindgen generates *mut pam_handle_t but it should be *pam_handle_t
-            std::mem::transmute(handle),
-            user,
-            prompt.map(|str| str.as_ptr()).unwrap_or(std::ptr::null()),
+            handle as *const PamHandle as *mut PamHandle,
+            &mut user_ptr,
+            prompt_ptr,
         )
-    })
+    }
+    .into()
+    {
+        PamReturnCode::SUCCESS => {
+            assert!(
+                !user_ptr.is_null(),
+                "Got PAM_SUCCESS from pam_get_user but ptr is null!"
+            );
+            Ok(unsafe { CStr::from_ptr(user_ptr) }
+                .to_str()
+                .expect("Got invalid UTF8 string from pam_get_user"))
+        }
+        err => Err(err.into()),
+    }
 }
 
-/* ----------------------- ffi::pam_modules.h ------------------------ */
+/* ----------------------- <security/pam_modules.h> ------------------------ */
+
+fn try_str_option_to_ptr(opt: Option<&str>) -> PamResult<*const c_char> {
+    match opt.map(CString::new) {
+        // Valid string given -> Return ptr of the converted CString
+        Some(Ok(content)) => Ok(content.as_ptr()),
+        // No string given -> Return null-ptr
+        None => Ok(std::ptr::null_mut()),
+        // Invalid string given -> Return BUF_ERR
+        _ => Err(PamReturnCode::BUF_ERR.into()),
+    }
+}