From e92be945cc727b302ef10862caa6ca0146e0cb2a Mon Sep 17 00:00:00 2001 From: Khyber Sen Date: Thu, 25 Apr 2024 17:44:43 -0700 Subject: [PATCH 1/3] `fn dav1d_default_picture_{alloc,release}`: Make safe. --- include/dav1d/picture.rs | 4 ++++ src/picture.rs | 30 +++++++++++++++++------------- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/include/dav1d/picture.rs b/include/dav1d/picture.rs index adee830bd..a0f068c12 100644 --- a/include/dav1d/picture.rs +++ b/include/dav1d/picture.rs @@ -344,6 +344,8 @@ pub(crate) struct Rav1dPicAllocator { /// /// # Safety /// + /// `pic` is passed as a `&mut`. + /// /// If frame threading is used, accesses to [`Self::cookie`] must be thread-safe, /// i.e. [`Self::cookie`] must be [`Send`]` + `[`Sync`]. pub alloc_picture_callback: @@ -353,6 +355,8 @@ pub(crate) struct Rav1dPicAllocator { /// /// # Safety /// + /// `pic` is passed as a `&mut`. + /// /// If frame threading is used, accesses to [`Self::cookie`] must be thread-safe, /// i.e. [`Self::cookie`] must be [`Send`]` + `[`Sync`]. pub release_picture_callback: diff --git a/src/picture.rs b/src/picture.rs index 3f8790e62..98ffc83cc 100644 --- a/src/picture.rs +++ b/src/picture.rs @@ -87,11 +87,12 @@ struct MemPoolBuf { buf: Vec, } -pub unsafe extern "C" fn dav1d_default_picture_alloc( +pub extern "C" fn dav1d_default_picture_alloc( p_c: *mut Dav1dPicture, cookie: *mut c_void, ) -> Dav1dResult { - let p = p_c.read().to::(); + // SAFETY: `Rav1dPicAllocator::alloc_picture_callback`'s safety guarantees that `p_c` was a `&mut`. + let p = unsafe { p_c.read() }.to::(); let hbd = (p.p.bpc > 8) as c_int; let aligned_w = p.p.w + 127 & !127; let aligned_h = p.p.h + 127 & !127; @@ -111,7 +112,8 @@ pub unsafe extern "C" fn dav1d_default_picture_alloc( let uv_sz = (uv_stride * (aligned_h >> ss_ver) as isize) as usize; let pic_size = y_sz + 2 * uv_sz; - let pool = &*cookie.cast::>>>(); + // SAFETY: When `Rav1dPicAllocator::alloc_picture_callback == dav1d_default_picture_alloc`, we set `Rav1dPicAllocator::cookie` to an `&Arc>`. + let pool = unsafe { &*cookie.cast::>>>() }; let pool = pool.clone(); let pic_cap = pic_size + RAV1D_PICTURE_ALIGNMENT; // TODO fallible allocation @@ -131,9 +133,11 @@ pub unsafe extern "C" fn dav1d_default_picture_alloc( // but this way is simpler and more uniform, especially when we move to slices. let data = [data0, data1, data2].map(|data| data.as_mut_ptr().cast()); - (*p_c).stride = stride; - (*p_c).data = data.map(NonNull::new); - (*p_c).allocator_data = NonNull::new(Box::into_raw(buf).cast::()); + // SAFETY: `Rav1dPicAllocator::alloc_picture_callback`'s safety guarantees that `p_c` was a `&mut`. + let p_c = unsafe { &mut *p_c }; + p_c.stride = stride; + p_c.data = data.map(NonNull::new); + p_c.allocator_data = NonNull::new(Box::into_raw(buf).cast::()); // The caller will create the real `Rav1dPicture` from the `Dav1dPicture` fields set above, // so we don't want to drop the `Rav1dPicture` we created for convenience here. mem::forget(p); @@ -141,13 +145,13 @@ pub unsafe extern "C" fn dav1d_default_picture_alloc( Rav1dResult::Ok(()).into() } -pub unsafe extern "C" fn dav1d_default_picture_release(p: *mut Dav1dPicture, _cookie: *mut c_void) { - let buf = (*p) - .allocator_data - .unwrap() - .as_ptr() - .cast::>>(); - let buf = Box::from_raw(buf); +pub extern "C" fn dav1d_default_picture_release(p: *mut Dav1dPicture, _cookie: *mut c_void) { + // SAFETY: `Rav1dPicAllocator::release_picture_callback`'s safety guarantees that `p_c` was a `&mut`. + let p = unsafe { &mut *p }; + let buf = p.allocator_data.unwrap().as_ptr(); + // SAFETY: `dav1d_default_picture_alloc` stores `Box::into_raw` of a `MemPoolBuf>` in `Dav1dPicture::allocator_data`, + // and `(Rav1dPicAllocator::release_picture_callback == dav1d_default_picture_release) == (Rav1dPicAllocator::alloc_picture_callback == dav1d_default_picture_alloc)`. + let buf = unsafe { Box::from_raw(buf.cast::>>()) }; let MemPoolBuf { pool, buf } = *buf; pool.push(buf); } From d150b085064ec8e6ed19262454d1fc93058a3f01 Mon Sep 17 00:00:00 2001 From: Khyber Sen Date: Thu, 25 Apr 2024 17:46:02 -0700 Subject: [PATCH 2/3] `mod picture`: Make all `fn`s safe and add `#![deny(unsafe_op_in_unsafe_fn)]`. --- src/picture.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/picture.rs b/src/picture.rs index 98ffc83cc..ae87f67f3 100644 --- a/src/picture.rs +++ b/src/picture.rs @@ -1,3 +1,5 @@ +#![deny(unsafe_op_in_unsafe_fn)] + use crate::include::dav1d::common::Rav1dDataProps; use crate::include::dav1d::dav1d::Rav1dEventFlags; use crate::include::dav1d::headers::DRav1d; @@ -223,7 +225,7 @@ impl Rav1dPicAllocator { } } -unsafe fn picture_alloc_with_edges( +fn picture_alloc_with_edges( logger: &Option, p: &mut Rav1dPicture, w: c_int, @@ -259,7 +261,7 @@ pub fn rav1d_picture_copy_props( // itut_t35 was taken out of the c.itut_t35 originally, but that violates Rust // borrowing rules so we need to pass it to this function explicitly. -pub(crate) unsafe fn rav1d_thread_picture_alloc( +pub(crate) fn rav1d_thread_picture_alloc( c: &Rav1dContext, f: &mut Rav1dFrameData, bpc: c_int, @@ -303,7 +305,7 @@ pub(crate) unsafe fn rav1d_thread_picture_alloc( Ok(()) } -pub(crate) unsafe fn rav1d_picture_alloc_copy( +pub(crate) fn rav1d_picture_alloc_copy( c: &Rav1dContext, dst: &mut Rav1dPicture, w: c_int, From 57485ec2af369ed1162075c65131deeadfeb87ae Mon Sep 17 00:00:00 2001 From: Khyber Sen Date: Sun, 28 Apr 2024 02:53:46 -0700 Subject: [PATCH 3/3] `fn dav1d_default_picture_{alloc,release}`: Mark `unsafe` due to raw ptr args, but make private and ensure they must be used safely. --- src/lib.rs | 11 ++--------- src/picture.rs | 35 +++++++++++++++++++++++++++++------ 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 9331fa5de..a0522301c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -42,8 +42,6 @@ use crate::src::mem::rav1d_freep_aligned; use crate::src::obu::rav1d_parse_obus; use crate::src::obu::rav1d_parse_sequence_header; use crate::src::pal::rav1d_pal_dsp_init; -use crate::src::picture::dav1d_default_picture_alloc; -use crate::src::picture::dav1d_default_picture_release; use crate::src::picture::rav1d_picture_alloc_copy; use crate::src::picture::PictureFlags; use crate::src::picture::Rav1dThreadPicture; @@ -211,9 +209,8 @@ pub(crate) unsafe fn rav1d_open(c_out: &mut *mut Rav1dContext, s: &Rav1dSettings (*c).decode_frame_type = s.decode_frame_type; (*c).cached_error_props = Default::default(); addr_of_mut!((*c).picture_pool).write(Default::default()); - if (*c).allocator.alloc_picture_callback == dav1d_default_picture_alloc - && (*c).allocator.release_picture_callback == dav1d_default_picture_release - { + + if (*c).allocator.is_default() { if !(*c).allocator.cookie.is_null() { return error(c, c_out); } @@ -222,10 +219,6 @@ pub(crate) unsafe fn rav1d_open(c_out: &mut *mut Rav1dContext, s: &Rav1dSettings (*c).allocator.cookie = ptr::from_ref(&(*c).picture_pool) .cast::() .cast_mut(); - } else if (*c).allocator.alloc_picture_callback == dav1d_default_picture_alloc - || (*c).allocator.release_picture_callback == dav1d_default_picture_release - { - return error(c, c_out); } if (::core::mem::size_of::() as c_ulong) < 8 as c_ulong && (s.frame_size_limit).wrapping_sub(1 as c_int as c_uint) >= (8192 * 8192) as c_uint diff --git a/src/picture.rs b/src/picture.rs index ae87f67f3..35cd825ce 100644 --- a/src/picture.rs +++ b/src/picture.rs @@ -89,11 +89,15 @@ struct MemPoolBuf { buf: Vec, } -pub extern "C" fn dav1d_default_picture_alloc( +/// # Safety +/// +/// * `p_c` must be from a `&mut Dav1dPicture`. +/// * `cookie` must be from a `&Arc>>`. +unsafe extern "C" fn dav1d_default_picture_alloc( p_c: *mut Dav1dPicture, cookie: *mut c_void, ) -> Dav1dResult { - // SAFETY: `Rav1dPicAllocator::alloc_picture_callback`'s safety guarantees that `p_c` was a `&mut`. + // SAFETY: Guaranteed by safety preconditions. let p = unsafe { p_c.read() }.to::(); let hbd = (p.p.bpc > 8) as c_int; let aligned_w = p.p.w + 127 & !127; @@ -114,7 +118,7 @@ pub extern "C" fn dav1d_default_picture_alloc( let uv_sz = (uv_stride * (aligned_h >> ss_ver) as isize) as usize; let pic_size = y_sz + 2 * uv_sz; - // SAFETY: When `Rav1dPicAllocator::alloc_picture_callback == dav1d_default_picture_alloc`, we set `Rav1dPicAllocator::cookie` to an `&Arc>`. + // SAFETY: Guaranteed by safety preconditions. let pool = unsafe { &*cookie.cast::>>>() }; let pool = pool.clone(); let pic_cap = pic_size + RAV1D_PICTURE_ALIGNMENT; @@ -135,7 +139,7 @@ pub extern "C" fn dav1d_default_picture_alloc( // but this way is simpler and more uniform, especially when we move to slices. let data = [data0, data1, data2].map(|data| data.as_mut_ptr().cast()); - // SAFETY: `Rav1dPicAllocator::alloc_picture_callback`'s safety guarantees that `p_c` was a `&mut`. + // SAFETY: Guaranteed by safety preconditions. let p_c = unsafe { &mut *p_c }; p_c.stride = stride; p_c.data = data.map(NonNull::new); @@ -147,8 +151,11 @@ pub extern "C" fn dav1d_default_picture_alloc( Rav1dResult::Ok(()).into() } -pub extern "C" fn dav1d_default_picture_release(p: *mut Dav1dPicture, _cookie: *mut c_void) { - // SAFETY: `Rav1dPicAllocator::release_picture_callback`'s safety guarantees that `p_c` was a `&mut`. +/// # Safety +/// +/// * `p` is from a `&mut Dav1dPicture` initialized by [`dav1d_default_picture_alloc`]. +unsafe extern "C" fn dav1d_default_picture_release(p: *mut Dav1dPicture, _cookie: *mut c_void) { + // SAFETY: Guaranteed by safety preconditions. let p = unsafe { &mut *p }; let buf = p.allocator_data.unwrap().as_ptr(); // SAFETY: `dav1d_default_picture_alloc` stores `Box::into_raw` of a `MemPoolBuf>` in `Dav1dPicture::allocator_data`, @@ -162,12 +169,28 @@ impl Default for Rav1dPicAllocator { fn default() -> Self { Self { cookie: ptr::null_mut(), + // SAFETY: `dav1d_default_picture_alloc` requires `p_c` be from a `&mut Dav1dPicture`, + // `Self::alloc_picture_callback` safety preconditions guarantee that. + // `dav1d_default_picture_alloc` also requires that `cookie` be from a `&Arc>>`, + // which is set if `Self::is_default()` in `rav1d_open`. alloc_picture_callback: dav1d_default_picture_alloc, + // SAFETY: `dav1d_default_picture_release` requires `p` be from a `&mut Dav1dPicture` + // initialized by `dav1d_default_picture_alloc`. + // Since these `fn`s are private and we only use them here, that is guaranteed. release_picture_callback: dav1d_default_picture_release, } } } +impl Rav1dPicAllocator { + pub fn is_default(&self) -> bool { + let alloc = self.alloc_picture_callback == dav1d_default_picture_alloc; + let release = self.release_picture_callback == dav1d_default_picture_release; + assert!(alloc == release); // This should be impossible since these `fn`s are private. + alloc && release + } +} + impl Rav1dPicAllocator { pub fn alloc_picture_data( &self,