From e37b9b4ba6f0357106e7f9787e1dfd0ba52509c0 Mon Sep 17 00:00:00 2001 From: Kabir Kwatra Date: Sat, 3 Feb 2024 14:15:56 -0800 Subject: [PATCH 1/3] fix: check zero size `PlaneData` before allocating Calls to `alloc` with zero sized layouts is undefined behavior. This is particularly problematic when using non-default allocators. Jemalloc panics when a zero sized layout used. rav1e creates zero sized `Planes` in multiple places to avoid the cost of allocation when the field is not used internally. --- src/plane.rs | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/src/plane.rs b/src/plane.rs index 66cfa07..2217a2c 100644 --- a/src/plane.rs +++ b/src/plane.rs @@ -103,7 +103,7 @@ pub struct PlaneOffset { #[derive(Debug, PartialEq, Eq)] #[cfg_attr(not(feature = "serialize"), derive(Serialize, Deserialize))] pub struct PlaneData { - ptr: std::ptr::NonNull, + ptr: Option>, // None when len == 0 _marker: PhantomData, len: usize, } @@ -126,9 +126,12 @@ impl std::ops::Deref for PlaneData { type Target = [T]; fn deref(&self) -> &[T] { + let Some(ptr) = self.ptr else { + return &[]; + }; // SAFETY: we cannot reference out of bounds because we know the length of the data unsafe { - let p = self.ptr.as_ptr(); + let p = ptr.as_ptr(); std::slice::from_raw_parts(p, self.len) } @@ -137,9 +140,12 @@ impl std::ops::Deref for PlaneData { impl std::ops::DerefMut for PlaneData { fn deref_mut(&mut self) -> &mut [T] { + let Some(ptr) = self.ptr else { + return &mut []; + }; // SAFETY: we cannot reference out of bounds because we know the length of the data unsafe { - let p = self.ptr.as_ptr(); + let p = ptr.as_ptr(); std::slice::from_raw_parts_mut(p, self.len) } @@ -149,8 +155,10 @@ impl std::ops::DerefMut for PlaneData { impl std::ops::Drop for PlaneData { fn drop(&mut self) { // SAFETY: we cannot dealloc too much because we know the length of the data - unsafe { - dealloc(self.ptr.as_ptr() as *mut u8, Self::layout(self.len)); + if let Some(ptr) = self.ptr { + unsafe { + dealloc(ptr.as_ptr() as *mut u8, Self::layout(self.len)); + } } } } @@ -200,13 +208,15 @@ impl PlaneData { } unsafe fn new_uninitialized(len: usize) -> Self { - let ptr = { + let ptr = if len == 0 { + None + } else { let layout = Self::layout(len); let ptr = alloc(layout) as *mut T; if ptr.is_null() { handle_alloc_error(layout); } - std::ptr::NonNull::new_unchecked(ptr) + Some(std::ptr::NonNull::new_unchecked(ptr)) }; PlaneData { @@ -1052,6 +1062,11 @@ pub mod test { assert_eq!(&output[..64], &plane.data[..64]); } + #[test] + fn test_plane_zero_len() { + let plane = Plane::::new(0, 0, 0, 0, 0, 0); + assert_eq!(plane.data.len(), 0); + } #[test] fn test_plane_downsample() { From 5cd9f3f576e392ddcec7788375f7ff09f7d0316f Mon Sep 17 00:00:00 2001 From: Kabir Kwatra Date: Sat, 10 Feb 2024 21:40:39 -0800 Subject: [PATCH 2/3] fix: replace `None` ptr with `NotNull::dangling()` --- src/plane.rs | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/plane.rs b/src/plane.rs index 2217a2c..68df6cf 100644 --- a/src/plane.rs +++ b/src/plane.rs @@ -103,7 +103,7 @@ pub struct PlaneOffset { #[derive(Debug, PartialEq, Eq)] #[cfg_attr(not(feature = "serialize"), derive(Serialize, Deserialize))] pub struct PlaneData { - ptr: Option>, // None when len == 0 + ptr: std::ptr::NonNull, _marker: PhantomData, len: usize, } @@ -126,12 +126,9 @@ impl std::ops::Deref for PlaneData { type Target = [T]; fn deref(&self) -> &[T] { - let Some(ptr) = self.ptr else { - return &[]; - }; // SAFETY: we cannot reference out of bounds because we know the length of the data unsafe { - let p = ptr.as_ptr(); + let p = self.ptr.as_ptr(); std::slice::from_raw_parts(p, self.len) } @@ -140,12 +137,9 @@ impl std::ops::Deref for PlaneData { impl std::ops::DerefMut for PlaneData { fn deref_mut(&mut self) -> &mut [T] { - let Some(ptr) = self.ptr else { - return &mut []; - }; // SAFETY: we cannot reference out of bounds because we know the length of the data unsafe { - let p = ptr.as_ptr(); + let p = self.ptr.as_ptr(); std::slice::from_raw_parts_mut(p, self.len) } @@ -155,9 +149,9 @@ impl std::ops::DerefMut for PlaneData { impl std::ops::Drop for PlaneData { fn drop(&mut self) { // SAFETY: we cannot dealloc too much because we know the length of the data - if let Some(ptr) = self.ptr { + if self.len != 0 { unsafe { - dealloc(ptr.as_ptr() as *mut u8, Self::layout(self.len)); + dealloc(self.ptr.as_ptr() as *mut u8, Self::layout(self.len)); } } } @@ -209,14 +203,14 @@ impl PlaneData { unsafe fn new_uninitialized(len: usize) -> Self { let ptr = if len == 0 { - None + std::ptr::NonNull::dangling() } else { let layout = Self::layout(len); let ptr = alloc(layout) as *mut T; if ptr.is_null() { handle_alloc_error(layout); } - Some(std::ptr::NonNull::new_unchecked(ptr)) + std::ptr::NonNull::new_unchecked(ptr) }; PlaneData { From de5183364f80e0611ea88b14de94c9c988e96fc6 Mon Sep 17 00:00:00 2001 From: Kabir Kwatra Date: Sun, 11 Feb 2024 02:43:50 -0800 Subject: [PATCH 3/3] test(plane): Add assertions for `PlaneData::deref` --- src/plane.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/plane.rs b/src/plane.rs index 68df6cf..09a8578 100644 --- a/src/plane.rs +++ b/src/plane.rs @@ -1060,6 +1060,10 @@ pub mod test { fn test_plane_zero_len() { let plane = Plane::::new(0, 0, 0, 0, 0, 0); assert_eq!(plane.data.len(), 0); + assert_eq!(*plane.data, []); + let copy = plane.clone(); + assert_eq!(copy.data.len(), 0); + assert_eq!(*copy.data, []); } #[test]