struct SendSyncNonNull: Add a Send + Sync NonNull type (#1312)

`SendSyncNonNull` is sound through `unsafe` methods, and we use it here
to make the `cookie` fields `Send + Sync`.

include/dav1d/picture.rs

@@ -27,6 +27,7 @@ use crate::src::error::Rav1dError;
 use crate::src::error::Rav1dError::EINVAL;
 use crate::src::error::Rav1dResult;
 use crate::src::pixels::Pixels;
+use crate::src::send_sync_non_null::SendSyncNonNull;
 use crate::src::strided::Strided;
 use crate::src::with_offset::WithOffset;
 use libc::ptrdiff_t;
@@ -109,7 +110,7 @@ pub struct Dav1dPicture {
     pub itut_t35_ref: Option<RawArc<DRav1d<Box<[Rav1dITUTT35]>, Box<[Dav1dITUTT35]>>>>, // opaque, so we can change this
     pub reserved_ref: [uintptr_t; 4],
     pub r#ref: Option<RawArc<Rav1dPictureData>>, // opaque, so we can change this
-    pub allocator_data: Option<NonNull<c_void>>,
+    pub allocator_data: Option<SendSyncNonNull<c_void>>,
 }
 
 #[derive(Clone, FromZeroes, FromBytes, AsBytes)]
@@ -390,7 +391,7 @@ impl<'a> Rav1dPictureDataComponentOffset<'a> {
 
 pub struct Rav1dPictureData {
     pub data: [Rav1dPictureDataComponent; 3],
-    pub(crate) allocator_data: Option<NonNull<c_void>>,
+    pub(crate) allocator_data: Option<SendSyncNonNull<c_void>>,
     pub(crate) allocator: Rav1dPicAllocator,
 }
 
@@ -549,7 +550,15 @@ impl Rav1dPicture {
 #[repr(C)]
 pub struct Dav1dPicAllocator {
     /// Custom data to pass to the allocator callbacks.
-    pub cookie: Option<NonNull<c_void>>,
+    ///
+    /// # Safety
+    ///
+    /// All accesses to [`Self::cookie`] must be thread-safe
+    /// (i.e. [`Self::cookie`] must be [`Send`]` + `[`Sync`]).
+    ///
+    /// If used from Rust, [`Self::cookie`] is a [`SendSyncNonNull`],
+    /// whose constructors ensure this [`Send`]` + `[`Sync`] safety.
+    pub cookie: Option<SendSyncNonNull<c_void>>,
 
     /// Allocate the picture buffer based on the [`Dav1dPictureParameters`].
     ///
@@ -561,7 +570,7 @@ pub struct Dav1dPicAllocator {
     ///
     /// # Safety
     ///
-    /// If frame threading is used, accesses to [`Self::cookie`] must be thread-safe.
+    /// See [`Self::cookie`]'s safety requirements.
     ///
     /// ### Additional `rav1d` requirement:
     ///
@@ -611,7 +620,7 @@ pub struct Dav1dPicAllocator {
     pub alloc_picture_callback: Option<
         unsafe extern "C" fn(
             pic: *mut Dav1dPicture,
-            cookie: Option<NonNull<c_void>>,
+            cookie: Option<SendSyncNonNull<c_void>>,
         ) -> Dav1dResult,
     >,
 
@@ -650,8 +659,9 @@ pub struct Dav1dPicAllocator {
     ///
     /// [`dav1d_get_picture`]: crate::src::lib::dav1d_get_picture
     /// [`alloc_picture_callback`]: Self::alloc_picture_callback
-    pub release_picture_callback:
-        Option<unsafe extern "C" fn(pic: *mut Dav1dPicture, cookie: Option<NonNull<c_void>>) -> ()>,
+    pub release_picture_callback: Option<
+        unsafe extern "C" fn(pic: *mut Dav1dPicture, cookie: Option<SendSyncNonNull<c_void>>) -> (),
+    >,
 }
 
 #[derive(Clone)]
@@ -679,7 +689,7 @@ pub(crate) struct Rav1dPicAllocator {
     ///
     /// [`Rav1dContext::picture_pool`]: crate::src::internal::Rav1dContext::picture_pool
     /// [`Rav1dContext`]: crate::src::internal::Rav1dContext
-    pub cookie: Option<NonNull<c_void>>,
+    pub cookie: Option<SendSyncNonNull<c_void>>,
 
     /// See [`Dav1dPicAllocator::alloc_picture_callback`].
     ///
@@ -691,7 +701,7 @@ pub(crate) struct Rav1dPicAllocator {
     /// i.e. [`Self::cookie`] must be [`Send`]` + `[`Sync`].
     pub alloc_picture_callback: unsafe extern "C" fn(
         pic: *mut Dav1dPicture,
-        cookie: Option<NonNull<c_void>>,
+        cookie: Option<SendSyncNonNull<c_void>>,
     ) -> Dav1dResult,
 
     /// See [`Dav1dPicAllocator::release_picture_callback`].
@@ -703,7 +713,7 @@ pub(crate) struct Rav1dPicAllocator {
     /// 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:
-        unsafe extern "C" fn(pic: *mut Dav1dPicture, cookie: Option<NonNull<c_void>>) -> (),
+        unsafe extern "C" fn(pic: *mut Dav1dPicture, cookie: Option<SendSyncNonNull<c_void>>) -> (),
 }
 
 impl TryFrom<Dav1dPicAllocator> for Rav1dPicAllocator {
@@ -787,7 +797,7 @@ impl Rav1dPicAllocator {
     pub fn dealloc_picture_data(
         &self,
         data: &mut [Rav1dPictureDataComponent; 3],
-        allocator_data: Option<NonNull<c_void>>,
+        allocator_data: Option<SendSyncNonNull<c_void>>,
     ) {
         let data = data.each_mut().map(|data| data.as_dav1d());
         let mut pic_c = Dav1dPicture {

lib.rs

@@ -54,6 +54,7 @@ pub mod src {
     pub(crate) mod pic_or_buf;
     pub(crate) mod pixels;
     pub(crate) mod relaxed_atomic;
+    pub mod send_sync_non_null;
     pub(crate) mod strided;
     pub(crate) mod with_offset;
     pub(crate) mod wrap_fn_ptr;

src/c_box.rs

@@ -1,19 +1,28 @@
 #![deny(unsafe_op_in_unsafe_fn)]
 
+use crate::src::send_sync_non_null::SendSyncNonNull;
 use std::ffi::c_void;
 use std::marker::PhantomData;
 use std::ops::Deref;
 use std::pin::Pin;
 use std::ptr::drop_in_place;
 use std::ptr::NonNull;
 
-pub type FnFree = unsafe extern "C" fn(ptr: *const u8, cookie: Option<NonNull<c_void>>);
+pub type FnFree = unsafe extern "C" fn(ptr: *const u8, cookie: Option<SendSyncNonNull<c_void>>);
 
 /// A `free` "closure", i.e. a [`FnFree`] and an enclosed context [`Self::cookie`].
 #[derive(Debug)]
 pub struct Free {
     pub free: FnFree,
-    pub cookie: Option<NonNull<c_void>>,
+
+    /// # Safety
+    ///
+    /// All accesses to [`Self::cookie`] must be thread-safe
+    /// (i.e. [`Self::cookie`] must be [`Send`]` + `[`Sync`]).
+    ///
+    /// If used from Rust, [`Self::cookie`] is a [`SendSyncNonNull`],
+    /// whose constructors ensure this [`Send`]` + `[`Sync`] safety.
+    pub cookie: Option<SendSyncNonNull<c_void>>,
 }
 
 impl Free {

src/data.rs

@@ -9,6 +9,7 @@ use crate::src::c_box::FnFree;
 use crate::src::c_box::Free;
 use crate::src::error::Rav1dError::EINVAL;
 use crate::src::error::Rav1dResult;
+use crate::src::send_sync_non_null::SendSyncNonNull;
 use std::ffi::c_void;
 use std::ptr::NonNull;
 
@@ -37,7 +38,7 @@ impl Rav1dData {
     pub unsafe fn wrap(
         data: NonNull<[u8]>,
         free_callback: Option<FnFree>,
-        cookie: Option<NonNull<c_void>>,
+        cookie: Option<SendSyncNonNull<c_void>>,
     ) -> Rav1dResult<Self> {
         let free = validate_input!(free_callback.ok_or(EINVAL))?;
         let free = Free { free, cookie };
@@ -54,7 +55,7 @@ impl Rav1dData {
         &mut self,
         user_data: NonNull<u8>,
         free_callback: Option<FnFree>,
-        cookie: Option<NonNull<c_void>>,
+        cookie: Option<SendSyncNonNull<c_void>>,
     ) -> Rav1dResult {
         let free = validate_input!(free_callback.ok_or(EINVAL))?;
         let free = Free { free, cookie };

src/lib.rs

@@ -47,6 +47,7 @@ use crate::src::obu::rav1d_parse_obus;
 use crate::src::obu::rav1d_parse_sequence_header;
 use crate::src::picture::rav1d_picture_alloc_copy;
 use crate::src::picture::PictureFlags;
+use crate::src::send_sync_non_null::SendSyncNonNull;
 use crate::src::thread_task::rav1d_task_delayed_fg;
 use crate::src::thread_task::rav1d_worker_task;
 use crate::src::thread_task::FRAME_ERROR;
@@ -297,7 +298,7 @@ pub(crate) fn rav1d_open(s: &Rav1dSettings) -> Rav1dResult<Arc<Rav1dContext>> {
         // SAFETY: When `allocator.is_default()`, `allocator.cookie` should be a `&c.picture_pool`.
         // See `Rav1dPicAllocator::cookie` docs for more, including an analysis of the lifetime.
         // Note also that we must do this after we created the `Arc` so that `c` has a stable address.
-        c.allocator.cookie = Some(NonNull::from(&c.picture_pool).cast::<c_void>());
+        c.allocator.cookie = Some(SendSyncNonNull::from_ref(&c.picture_pool).cast::<c_void>());
     }
     let c = c;
 
@@ -864,7 +865,7 @@ pub unsafe extern "C" fn dav1d_data_wrap(
     ptr: Option<NonNull<u8>>,
     sz: usize,
     free_callback: Option<FnFree>,
-    user_data: Option<NonNull<c_void>>,
+    user_data: Option<SendSyncNonNull<c_void>>,
 ) -> Dav1dResult {
     || -> Rav1dResult {
         let buf = validate_input!(buf.ok_or(EINVAL))?;
@@ -891,7 +892,7 @@ pub unsafe extern "C" fn dav1d_data_wrap_user_data(
     buf: Option<NonNull<Dav1dData>>,
     user_data: Option<NonNull<u8>>,
     free_callback: Option<FnFree>,
-    cookie: Option<NonNull<c_void>>,
+    cookie: Option<SendSyncNonNull<c_void>>,
 ) -> Dav1dResult {
     || -> Rav1dResult {
         let buf = validate_input!(buf.ok_or(EINVAL))?;

src/log.rs

@@ -1,3 +1,4 @@
+use crate::src::send_sync_non_null::SendSyncNonNull;
 use std::ffi::c_char;
 use std::ffi::c_uint;
 use std::ffi::c_void;
@@ -6,11 +7,10 @@ use std::fmt::Write as _;
 use std::io::stderr;
 use std::io::stdout;
 use std::io::Write as _;
-use std::ptr::NonNull;
 
 pub type Dav1dLoggerCallback = unsafe extern "C" fn(
-    // The above `cookie` field.
-    cookie: Option<NonNull<c_void>>,
+    // [`Dav1dLogger::cookie`].
+    cookie: Option<SendSyncNonNull<c_void>>,
     // A `printf`-style format specifier.
     fmt: *const c_char,
     // `printf`-style variadic args.
@@ -21,7 +21,16 @@ pub type Dav1dLoggerCallback = unsafe extern "C" fn(
 #[repr(C)]
 pub struct Dav1dLogger {
     /// A cookie that's passed as the first argument to the callback below.
-    cookie: Option<NonNull<c_void>>,
+    ///
+    /// # Safety
+    ///
+    /// All accesses to [`Self::cookie`] must be thread-safe
+    /// (i.e. [`Self::cookie`] must be [`Send`]` + `[`Sync`]).
+    ///
+    /// If used from Rust, [`Self::cookie`] is a [`SendSyncNonNull`],
+    /// whose constructors ensure this [`Send`]` + `[`Sync`] safety.
+    cookie: Option<SendSyncNonNull<c_void>>,
+
     /// A `printf`-style function except for an extra first argument that will always be the above `cookie`.
     callback: Option<Dav1dLoggerCallback>,
 }
@@ -32,8 +41,10 @@ impl Dav1dLogger {
     /// `callback`, if non-[`None`]/`NULL` must be safe to call when:
     /// * the first argument is `cookie`
     /// * the rest of the arguments would be safe to call `printf` with
+    ///
+    /// See [`Self::cookie`]'s safety requirements.
     pub const unsafe fn new(
-        cookie: Option<NonNull<c_void>>,
+        cookie: Option<SendSyncNonNull<c_void>>,
         callback: Option<Dav1dLoggerCallback>,
     ) -> Self {
         Self { cookie, callback }

src/picture.rs

@@ -25,6 +25,7 @@ use crate::src::internal::Rav1dFrameData;
 use crate::src::log::Rav1dLog as _;
 use crate::src::log::Rav1dLogger;
 use crate::src::mem::MemPool;
+use crate::src::send_sync_non_null::SendSyncNonNull;
 use bitflags::bitflags;
 use libc::ptrdiff_t;
 use parking_lot::Mutex;
@@ -100,7 +101,7 @@ impl Rav1dPictureParameters {
 /// * `cookie` must be from a `&Arc<MemPool<u8>>`.
 unsafe extern "C" fn dav1d_default_picture_alloc(
     p_c: *mut Dav1dPicture,
-    cookie: Option<NonNull<c_void>>,
+    cookie: Option<SendSyncNonNull<c_void>>,
 ) -> Dav1dResult {
     // SAFETY: Guaranteed by safety preconditions.
     let p = unsafe { p_c.read() }.to::<Rav1dPicture>();
@@ -120,8 +121,9 @@ unsafe extern "C" fn dav1d_default_picture_alloc(
     let [y_sz, uv_sz] = p.p.pic_len(stride);
     let pic_size = y_sz + 2 * uv_sz;
 
+    let pool = cookie.unwrap().cast::<Arc<MemPool<u8>>>();
     // SAFETY: Guaranteed by safety preconditions.
-    let pool = unsafe { cookie.unwrap().cast::<Arc<MemPool<u8>>>().as_ref() };
+    let pool = unsafe { pool.as_ref() };
     let pool = pool.clone();
     let pic_cap = pic_size + RAV1D_PICTURE_ALIGNMENT;
     // TODO fallible allocation
@@ -150,7 +152,7 @@ unsafe extern "C" fn dav1d_default_picture_alloc(
     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::<c_void>());
+    p_c.allocator_data = Some(SendSyncNonNull::from_box(buf).cast::<c_void>());
     // 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);
@@ -163,14 +165,14 @@ unsafe extern "C" fn dav1d_default_picture_alloc(
 /// * `p` is from a `&mut Dav1dPicture` initialized by [`dav1d_default_picture_alloc`].
 unsafe extern "C" fn dav1d_default_picture_release(
     p: *mut Dav1dPicture,
-    _cookie: Option<NonNull<c_void>>,
+    _cookie: Option<SendSyncNonNull<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<u8>` in `Dav1dPicture::allocator_data`,
+    let buf = p.allocator_data.unwrap().cast::<MemPoolBuf<u8>>();
+    // SAFETY: `dav1d_default_picture_alloc` stores `SendSyncNonNull::from_box` of a `Box<MemPoolBuf<u8>>` 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::<MemPoolBuf<u8>>()) };
+    let buf = unsafe { buf.into_box() };
     let MemPoolBuf { pool, buf } = *buf;
     pool.push(buf);
 }