diff src/libpam/handle.rs @ 143:ebb71a412b58

Turn everything into OsString and Just Walk Out! for strings with nul. To reduce the hazard surface of the API, this replaces most uses of &str with &OsStr (and likewise with String/OsString). Also, I've decided that instead of dealing with callers putting `\0` in their parameters, I'm going to follow the example of std::env and Just Walk Out! (i.e., panic!()). This makes things a lot less annoying for both me and (hopefully) users.
author Paul Fisher <paul@pfish.zone>
date Sat, 05 Jul 2025 22:12:46 -0400
parents a508a69c068a
children 56b559b7ecea
line wrap: on
line diff
--- a/src/libpam/handle.rs	Sat Jul 05 21:49:27 2025 -0400
+++ b/src/libpam/handle.rs	Sat Jul 05 22:12:46 2025 -0400
@@ -13,8 +13,9 @@
 use libpam_sys_helpers::constants;
 use num_enum::{IntoPrimitive, TryFromPrimitive};
 use std::cell::Cell;
-use std::ffi::{c_char, c_int, CString};
+use std::ffi::{c_char, c_int, CString, OsStr, OsString};
 use std::mem::ManuallyDrop;
+use std::os::unix::ffi::OsStrExt;
 use std::ptr;
 use std::ptr::NonNull;
 
@@ -36,20 +37,20 @@
 
 #[derive(Debug, PartialEq)]
 pub struct HandleBuilder {
-    service_name: String,
-    username: Option<String>,
+    service_name: OsString,
+    username: Option<OsString>,
 }
 
 impl HandleBuilder {
     /// Updates the service name.
-    pub fn service_name(mut self, service_name: String) -> Self {
+    pub fn service_name(mut self, service_name: OsString) -> Self {
         self.service_name = service_name;
         self
     }
     /// Sets the username. Setting this will avoid the need for an extra
     /// round trip through the conversation and may otherwise improve
     /// the login experience.
-    pub fn username(mut self, username: String) -> Self {
+    pub fn username(mut self, username: OsString) -> Self {
         self.username = Some(username);
         self
     }
@@ -71,17 +72,18 @@
     ///
     #[doc = stdlinks!(3 pam_start)]
     #[doc = guide!(adg: "adg-interface-by-app-expected.html#adg-pam_start")]
-    pub fn build_with_service(service_name: String) -> HandleBuilder {
+    pub fn build_with_service(service_name: OsString) -> HandleBuilder {
         HandleBuilder {
             service_name,
             username: None,
         }
     }
 
-    fn start(service_name: String, username: Option<String>, conversation: C) -> Result<Self> {
+    fn start(service_name: OsString, username: Option<OsString>, conversation: C) -> Result<Self> {
         let conv = Box::new(OwnedConversation::new(conversation));
-        let service_cstr = CString::new(service_name).map_err(|_| ErrorCode::ConversationError)?;
-        let username_cstr = memory::prompt_ptr(memory::option_cstr(username.as_deref())?.as_ref());
+        let service_cstr = CString::new(service_name.as_bytes()).expect("null is forbidden");
+        let username_cstr = memory::option_cstr_os(username.as_deref());
+        let username_cstr = memory::prompt_ptr(username_cstr.as_deref());
 
         let mut handle: *mut libpam_sys::pam_handle = ptr::null_mut();
         // SAFETY: We've set everything up properly to call `pam_start`.
@@ -191,11 +193,11 @@
     };
     // Then have item getters / setters
     (get = $get:ident$(, set = $set:ident)?) => {
-        delegate!(fn $get(&self) -> Result<Option<String>>);
+        delegate!(fn $get(&self) -> Result<Option<OsString>>);
         $(delegate!(set = $set);)?
     };
     (set = $set:ident) => {
-        delegate!(fn $set(&mut self, value: Option<&str>) -> Result<()>);
+        delegate!(fn $set(&mut self, value: Option<&OsStr>) -> Result<()>);
     };
 }
 
@@ -207,7 +209,7 @@
     delegate!(fn log(&self, level: Level, location: Location<'_>, entry: &str) -> ());
     delegate!(fn environ(&self) -> impl EnvironMap);
     delegate!(fn environ_mut(&mut self) -> impl EnvironMapMut);
-    delegate!(fn username(&mut self, prompt: Option<&str>) -> Result<String>);
+    delegate!(fn username(&mut self, prompt: Option<&OsStr>) -> Result<OsString>);
     delegate!(get = user_item, set = set_user_item);
     delegate!(get = service, set = set_service);
     delegate!(get = user_prompt, set = set_user_prompt);
@@ -221,12 +223,12 @@
 /// Macro to implement getting/setting a CStr-based item.
 macro_rules! cstr_item {
     (get = $getter:ident, item = $item_type:path) => {
-        fn $getter(&self) -> Result<Option<String>> {
+        fn $getter(&self) -> Result<Option<OsString>> {
             unsafe { self.get_cstr_item($item_type) }
         }
     };
     (set = $setter:ident, item = $item_type:path) => {
-        fn $setter(&mut self, value: Option<&str>) -> Result<()> {
+        fn $setter(&mut self, value: Option<&OsStr>) -> Result<()> {
             unsafe { self.set_cstr_item($item_type, value) }
         }
     };
@@ -328,7 +330,7 @@
             Ok(cstr) => cstr,
             _ => return,
         };
-        #[cfg(pam_impl = "linux-pam")]
+        #[cfg(pam_impl = "LinuxPam")]
         {
             _ = loc;
             // SAFETY: We're calling this function with a known value.
@@ -336,7 +338,7 @@
                 libpam_sys::pam_syslog(self, level as c_int, "%s\0".as_ptr().cast(), entry.as_ptr())
             }
         }
-        #[cfg(pam_impl = "openpam")]
+        #[cfg(pam_impl = "OpenPam")]
         {
             let func = CString::new(loc.function).unwrap_or(CString::default());
             // SAFETY: We're calling this function with a known value.
@@ -353,20 +355,18 @@
 
     fn log(&self, _level: Level, _loc: Location<'_>, _entry: &str) {}
 
-    fn username(&mut self, prompt: Option<&str>) -> Result<String> {
-        let prompt = memory::option_cstr(prompt)?;
+    fn username(&mut self, prompt: Option<&OsStr>) -> Result<OsString> {
+        let prompt = memory::option_cstr_os(prompt);
         let mut output: *const c_char = ptr::null();
         let ret = unsafe {
             libpam_sys::pam_get_user(
                 self.raw_mut(),
                 &mut output,
-                memory::prompt_ptr(prompt.as_ref()),
+                memory::prompt_ptr(prompt.as_deref()),
             )
         };
         ErrorCode::result_from(ret)?;
-        unsafe { memory::copy_pam_string(output) }
-            .transpose()
-            .unwrap_or(Err(ErrorCode::ConversationError))
+        Ok(unsafe { memory::copy_pam_string(output).ok_or(ErrorCode::ConversationError)? })
     }
 
     fn environ(&self) -> impl EnvironMap {
@@ -407,11 +407,11 @@
 }
 
 impl PamHandleModule for RawPamHandle {
-    fn authtok(&mut self, prompt: Option<&str>) -> Result<String> {
+    fn authtok(&mut self, prompt: Option<&OsStr>) -> Result<OsString> {
         self.get_authtok(prompt, ItemType::AuthTok)
     }
 
-    fn old_authtok(&mut self, prompt: Option<&str>) -> Result<String> {
+    fn old_authtok(&mut self, prompt: Option<&OsStr>) -> Result<OsString> {
         self.get_authtok(prompt, ItemType::OldAuthTok)
     }
 
@@ -432,8 +432,8 @@
 // Implementations of internal functions.
 impl RawPamHandle {
     #[cfg(any(pam_impl = "LinuxPam", pam_impl = "OpenPam"))]
-    fn get_authtok(&mut self, prompt: Option<&str>, item_type: ItemType) -> Result<String> {
-        let prompt = memory::option_cstr(prompt)?;
+    fn get_authtok(&mut self, prompt: Option<&OsStr>, item_type: ItemType) -> Result<OsString> {
+        let prompt = memory::option_cstr_os(prompt);
         let mut output: *const c_char = ptr::null_mut();
         // SAFETY: We're calling this with known-good values.
         let res = unsafe {
@@ -441,14 +441,12 @@
                 self.raw_mut(),
                 item_type.into(),
                 &mut output,
-                memory::prompt_ptr(prompt.as_ref()),
+                memory::prompt_ptr(prompt.as_deref()),
             )
         };
         ErrorCode::result_from(res)?;
         // SAFETY: We got this string from PAM.
-        unsafe { memory::copy_pam_string(output) }
-            .transpose()
-            .unwrap_or(Err(ErrorCode::ConversationError))
+        unsafe { memory::copy_pam_string(output) }.ok_or(ErrorCode::ConversationError)
     }
 
     #[cfg(not(any(pam_impl = "LinuxPam", pam_impl = "OpenPam")))]
@@ -461,12 +459,12 @@
     /// # Safety
     ///
     /// You better be requesting an item which is a C string.
-    unsafe fn get_cstr_item(&self, item_type: ItemType) -> Result<Option<String>> {
+    unsafe fn get_cstr_item(&self, item_type: ItemType) -> Result<Option<OsString>> {
         let mut output = ptr::null();
         let ret =
             unsafe { libpam_sys::pam_get_item(self.raw_ref(), item_type as c_int, &mut output) };
         ErrorCode::result_from(ret)?;
-        memory::copy_pam_string(output.cast())
+        Ok(memory::copy_pam_string(output.cast()))
     }
 
     /// Sets a C string item.
@@ -474,13 +472,13 @@
     /// # Safety
     ///
     /// You better be setting an item which is a C string.
-    unsafe fn set_cstr_item(&mut self, item_type: ItemType, data: Option<&str>) -> Result<()> {
-        let data_str = memory::option_cstr(data)?;
+    unsafe fn set_cstr_item(&mut self, item_type: ItemType, data: Option<&OsStr>) -> Result<()> {
+        let data_str = memory::option_cstr_os(data);
         let ret = unsafe {
             libpam_sys::pam_set_item(
                 self.raw_mut(),
                 item_type as c_int,
-                memory::prompt_ptr(data_str.as_ref()).cast(),
+                memory::prompt_ptr(data_str.as_deref()).cast(),
             )
         };
         ErrorCode::result_from(ret)