From 51c010a21dd446d080b0e9f6c2a75eb7f23e5140 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 29 Aug 2024 16:45:22 -0600 Subject: [PATCH 1/3] Add defensive ddcommon_ffi checks for Slice It seems it is prudent to add checks here. At some point you need to be able to trust the FFI caller to uphold needed invariants, but it's a bit too easy to zero-initialize stuff in C and not fix it up for these Slices. --- ddcommon-ffi/src/slice.rs | 47 +++++++++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 9 deletions(-) diff --git a/ddcommon-ffi/src/slice.rs b/ddcommon-ffi/src/slice.rs index f54a80fe72..c2a4427c4f 100644 --- a/ddcommon-ffi/src/slice.rs +++ b/ddcommon-ffi/src/slice.rs @@ -14,7 +14,9 @@ use std::str::Utf8Error; #[repr(C)] #[derive(Copy, Clone)] pub struct Slice<'a, T: 'a> { - /// Must be non-null and suitably aligned for the underlying type. + /// Should be non-null and suitably aligned for the underlying type. It is + /// allowed but not recommended for the pointer to be null when the len is + /// zero. ptr: *const T, /// The number of elements (not bytes) that `.ptr` points to. Must be less @@ -53,7 +55,13 @@ pub type ByteSlice<'a> = Slice<'a, u8>; /// This exists as an intrinsic, but it is private. pub fn is_aligned_and_not_null(ptr: *const T) -> bool { - !ptr.is_null() && ptr as usize % std::mem::align_of::() == 0 + !ptr.is_null() && is_aligned(ptr) +} + +#[inline] +fn is_aligned(ptr: *const T) -> bool { + debug_assert!(!ptr.is_null()); + ptr as usize % std::mem::align_of::() == 0 } pub trait AsBytes<'a> { @@ -78,7 +86,8 @@ impl<'a> AsBytes<'a> for Slice<'a, u8> { impl<'a> AsBytes<'a> for Slice<'a, i8> { fn as_bytes(&self) -> &'a [u8] { - unsafe { slice::from_raw_parts(self.ptr.cast(), self.len) } + // SAFETY: safe to convert *i8 to *u8 and then read it... I think. + unsafe { Slice::from_raw_parts(self.ptr.cast(), self.len) }.as_slice() } } @@ -95,6 +104,8 @@ impl<'a, T: 'a> Slice<'a, T> { /// # Safety /// Uphold the same safety requirements as [std::str::from_raw_parts]. + /// However, it is allowed but not recommended to provide a null pointer + /// when the len is 0. pub const unsafe fn from_raw_parts(ptr: *const T, len: usize) -> Self { Self { ptr, @@ -111,12 +122,20 @@ impl<'a, T: 'a> Slice<'a, T> { } } - pub const fn as_slice(&self) -> &'a [T] { - unsafe { slice::from_raw_parts(self.ptr, self.len) } + pub fn as_slice(&self) -> &'a [T] { + if !self.ptr.is_null() { + // Crashing immediately is likely better than ignoring this. + assert!(is_aligned(self.ptr)); + unsafe { slice::from_raw_parts(self.ptr, self.len) } + } else { + // Crashing immediately is likely better than ignoring this. + assert_eq!(self.len, 0); + &[] + } } - pub const fn into_slice(self) -> &'a [T] { - unsafe { slice::from_raw_parts(self.ptr, self.len) } + pub fn into_slice(self) -> &'a [T] { + self.as_slice() } } @@ -177,9 +196,9 @@ impl<'a> From<&'a str> for Slice<'a, c_char> { #[cfg(test)] mod tests { + use super::*; use std::os::raw::c_char; - - use crate::slice::*; + use std::ptr; #[test] fn slice_from_into_slice() { @@ -240,4 +259,14 @@ mod tests { assert_eq!(Some(&2), iter.next()); assert_eq!(Some(&3), iter.next()); } + + #[test] + fn test_null_len0() { + let null_len0: Slice = Slice { + ptr: ptr::null(), + len: 0, + _marker: PhantomData, + }; + assert_eq!(null_len0.as_slice(), &[]); + } } From dadfc636763def6ac85fb93c0b039f52f8d48e5d Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 29 Aug 2024 16:49:31 -0600 Subject: [PATCH 2/3] test: add panic test for null but non-zero len --- ddcommon-ffi/src/slice.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/ddcommon-ffi/src/slice.rs b/ddcommon-ffi/src/slice.rs index c2a4427c4f..d0e0534886 100644 --- a/ddcommon-ffi/src/slice.rs +++ b/ddcommon-ffi/src/slice.rs @@ -269,4 +269,15 @@ mod tests { }; assert_eq!(null_len0.as_slice(), &[]); } + + #[should_panic] + #[test] + fn test_null_panic() { + let null_len0: Slice = Slice { + ptr: ptr::null(), + len: 1, + _marker: PhantomData, + }; + _ = null_len0.as_slice(); + } } From 70b6afe9c46cf6f54a5181b46e16c32da7355482 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 4 Sep 2024 15:56:57 -0600 Subject: [PATCH 3/3] Test length too --- ddcommon-ffi/src/slice.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/ddcommon-ffi/src/slice.rs b/ddcommon-ffi/src/slice.rs index d0e0534886..f2cd52098a 100644 --- a/ddcommon-ffi/src/slice.rs +++ b/ddcommon-ffi/src/slice.rs @@ -124,8 +124,9 @@ impl<'a, T: 'a> Slice<'a, T> { pub fn as_slice(&self) -> &'a [T] { if !self.ptr.is_null() { - // Crashing immediately is likely better than ignoring this. + // Crashing immediately is likely better than ignoring these. assert!(is_aligned(self.ptr)); + assert!(self.len <= isize::MAX as usize); unsafe { slice::from_raw_parts(self.ptr, self.len) } } else { // Crashing immediately is likely better than ignoring this. @@ -280,4 +281,15 @@ mod tests { }; _ = null_len0.as_slice(); } + + #[should_panic] + #[test] + fn test_long_panic() { + let dangerous: Slice = Slice { + ptr: ptr::NonNull::dangling().as_ptr(), + len: isize::MAX as usize + 1, + _marker: PhantomData, + }; + _ = dangerous.as_slice(); + } }