Mercurial > crates > nonstick
diff src/libpam/memory.rs @ 93:efc2b56c8928
Remove undefined behavior per MIRI.
This replaces a bunch of raw pointers with NonNull and removes all the
undefined behavior that we can find with MIRI.
We also remove the `SecureString` dependency (since it doesn't work with MIRI,
and because it's not really necessary).
author | Paul Fisher <paul@pfish.zone> |
---|---|
date | Mon, 23 Jun 2025 13:02:58 -0400 |
parents | 5aa1a010f1e8 |
children | 51c9d7e8261a |
line wrap: on
line diff
--- a/src/libpam/memory.rs Sun Jun 22 19:29:32 2025 -0400 +++ b/src/libpam/memory.rs Mon Jun 23 13:02:58 2025 -0400 @@ -4,7 +4,9 @@ use crate::{BinaryData, ErrorCode}; use std::ffi::{c_char, CStr, CString}; use std::marker::{PhantomData, PhantomPinned}; -use std::{ptr, slice}; +use std::mem::offset_of; +use std::ptr::NonNull; +use std::{mem, ptr, slice}; /// Allocates `count` elements to hold `T`. #[inline] @@ -50,30 +52,22 @@ /// # Safety /// /// It's on you to provide a valid string. -pub unsafe fn copy_pam_string(result_ptr: *const libc::c_char) -> Result<String> { - // We really shouldn't get a null pointer back here, but if we do, return nothing. - if result_ptr.is_null() { - return Ok(String::new()); - } - let bytes = unsafe { CStr::from_ptr(result_ptr) }; - bytes - .to_str() +pub unsafe fn copy_pam_string(result_ptr: *const c_char) -> Result<String> { + Ok(wrap_string(result_ptr)? .map(String::from) - .map_err(|_| ErrorCode::ConversationError) + .unwrap_or_default()) } /// Wraps a string returned from PAM as an `Option<&str>`. -pub unsafe fn wrap_string<'a>(data: *const libc::c_char) -> Result<Option<&'a str>> { - let ret = if data.is_null() { - None - } else { - Some( - CStr::from_ptr(data) +pub unsafe fn wrap_string<'a>(data: *const c_char) -> Result<Option<&'a str>> { + match NonNull::new(data.cast_mut()) { + Some(data) => Ok(Some( + CStr::from_ptr(data.as_ptr()) .to_str() .map_err(|_| ErrorCode::ConversationError)?, - ) - }; - Ok(ret) + )), + None => Ok(None), + } } /// Allocates a string with the given contents on the C heap. @@ -82,7 +76,7 @@ /// /// - it allocates data on the C heap with [`libc::malloc`]. /// - it doesn't take ownership of the data passed in. -pub fn malloc_str(text: &str) -> Result<*mut c_char> { +pub fn malloc_str(text: &str) -> Result<NonNull<c_char>> { let data = text.as_bytes(); if data.contains(&0) { return Err(ErrorCode::ConversationError); @@ -92,8 +86,8 @@ // SAFETY: we just allocated this and we have enough room. unsafe { libc::memcpy(data_alloc.cast(), data.as_ptr().cast(), data.len()); + Ok(NonNull::new_unchecked(data_alloc)) } - Ok(data_alloc) } /// Writes zeroes over the contents of a C string. @@ -105,7 +99,10 @@ /// It's up to you to provide a valid C string. pub unsafe fn zero_c_string(cstr: *mut c_char) { if !cstr.is_null() { - libc::memset(cstr.cast(), 0, libc::strlen(cstr.cast())); + let len = libc::strlen(cstr.cast()); + for x in 0..len { + ptr::write_volatile(cstr.byte_offset(x as isize), mem::zeroed()) + } } } @@ -128,17 +125,21 @@ impl CBinaryData { /// Copies the given data to a new BinaryData on the heap. - pub fn alloc((data, data_type): (&[u8], u8)) -> Result<*mut CBinaryData> { + pub fn alloc((data, data_type): (&[u8], u8)) -> Result<NonNull<CBinaryData>> { let buffer_size = u32::try_from(data.len() + 5).map_err(|_| ErrorCode::ConversationError)?; // SAFETY: We're only allocating here. let dest = unsafe { - let dest_buffer: *mut CBinaryData = calloc::<u8>(buffer_size as usize).cast(); - let dest = &mut *dest_buffer; + let mut dest_buffer: NonNull<Self> = + NonNull::new_unchecked(calloc::<u8>(buffer_size as usize).cast()); + let dest = dest_buffer.as_mut(); dest.total_length = buffer_size.to_be_bytes(); dest.data_type = data_type; - let dest = dest.data.as_mut_ptr(); - libc::memcpy(dest.cast(), data.as_ptr().cast(), data.len()); + libc::memcpy( + Self::data_ptr(dest_buffer).cast(), + data.as_ptr().cast(), + data.len(), + ); dest_buffer }; Ok(dest) @@ -148,39 +149,33 @@ u32::from_be_bytes(self.total_length).saturating_sub(5) as usize } - /// Clears this data and frees it. - pub unsafe fn zero_contents(&mut self) { - let contents = slice::from_raw_parts_mut(self.data.as_mut_ptr(), self.length()); - for v in contents { - *v = 0 + fn data_ptr(ptr: NonNull<Self>) -> *mut u8 { + unsafe { + ptr.as_ptr() + .cast::<u8>() + .byte_offset(offset_of!(Self, data) as isize) } - self.data_type = 0; - self.total_length = [0; 4]; } -} + + unsafe fn data_slice<'a>(ptr: NonNull<Self>) -> &'a mut [u8] { + unsafe { slice::from_raw_parts_mut(Self::data_ptr(ptr), ptr.as_ref().length()) } + } -impl<'a> From<&'a CBinaryData> for (&'a [u8], u8) { - fn from(value: &'a CBinaryData) -> Self { - ( - unsafe { slice::from_raw_parts(value.data.as_ptr(), value.length()) }, - value.data_type, - ) + pub unsafe fn data<'a>(ptr: NonNull<Self>) -> (&'a [u8], u8) { + unsafe { (Self::data_slice(ptr), ptr.as_ref().data_type) } } -} -impl From<&'_ CBinaryData> for BinaryData { - fn from(value: &'_ CBinaryData) -> Self { - // This is a dumb trick but I like it because it is simply the presence - // of `.map(|z: (_, _)| z)` in the middle of this that gives - // type inference the hint it needs to make this work. - let [ret] = [value].map(Into::into).map(|z: (_, _)| z).map(Into::into); - ret + pub unsafe fn zero_contents(ptr: NonNull<Self>) { + for byte in Self::data_slice(ptr) { + ptr::write_volatile(byte as *mut u8, mem::zeroed()); + } + ptr::write_volatile(ptr.as_ptr(), mem::zeroed()); } -} -impl From<Option<&'_ CBinaryData>> for BinaryData { - fn from(value: Option<&CBinaryData>) -> Self { - value.map(Into::into).unwrap_or_default() + #[allow(clippy::wrong_self_convention)] + pub unsafe fn as_binary_data(ptr: NonNull<Self>) -> BinaryData { + let (data, data_type) = unsafe { (CBinaryData::data_slice(ptr), ptr.as_ref().data_type) }; + (Vec::from(data), data_type).into() } } @@ -193,6 +188,7 @@ #[test] fn test_strings() { let str = malloc_str("hello there").unwrap(); + let str = str.as_ptr(); malloc_str("hell\0 there").unwrap_err(); unsafe { let copied = copy_pam_string(str).unwrap();