Audit Dispatch functions for safety

Part of #77.

Author: madsmtm

crates/dispatch2/src/ffi.rs

@@ -226,16 +226,3 @@ enum_with_val! {
 #[cfg_attr(target_vendor = "apple", link(name = "System", kind = "dylib"))]
 #[cfg_attr(not(target_vendor = "apple"), link(name = "dispatch", kind = "dylib"))]
 extern "C" {}
-
-// `dispatch_main` is marked DISPATCH_NOTHROW.
-extern "C" {
-    /// Executes blocks submitted to the main queue.
-    pub fn dispatch_main() -> !;
-}
-
-// Inline function in the header
-// TODO: Mark this as `const`
-pub extern "C" fn dispatch_get_main_queue() -> &'static DispatchQueue {
-    // SAFETY: The main queue is safe to access from anywhere.
-    unsafe { &_dispatch_main_q }
-}

crates/dispatch2/src/group.rs

@@ -17,13 +17,6 @@ dispatch_object!(
 dispatch_object_not_data!(unsafe DispatchGroup);
 
 impl DispatchGroup {
-    /// Creates a new [`DispatchGroup`].
-    pub fn new() -> Option<DispatchRetained<Self>> {
-        // SAFETY: Valid to call.
-        // TODO: Properly allow NULL again (`dispatch_group_create` is incorrectly mapped).
-        Some(unsafe { dispatch_group_create() })
-    }
-
     /// Submit a function to a [`DispatchQueue`] and associates it with the [`DispatchGroup`].
     pub fn exec_async<F>(&self, queue: &DispatchQueue, work: F)
     where
@@ -34,7 +27,7 @@ impl DispatchGroup {
         let work_boxed = Box::into_raw(Box::new(work)).cast::<c_void>();
 
         // Safety: All parameters cannot be null.
-        unsafe { Self::async_f(self, queue, work_boxed, function_wrapper::<F>) };
+        unsafe { Self::exec_async_f(self, queue, work_boxed, function_wrapper::<F>) };
     }
 
     /// Wait synchronously for the previously submitted functions to finish.
@@ -51,8 +44,7 @@ impl DispatchGroup {
             DISPATCH_TIME_FOREVER
         };
 
-        // Safety: object cannot be null and timeout is valid.
-        let result = unsafe { dispatch_group_wait(self, timeout) };
+        let result = dispatch_group_wait(self, timeout);
 
         match result {
             0 => Ok(()),
@@ -75,7 +67,7 @@ impl DispatchGroup {
 
     /// Explicitly indicates that the function has entered the [`DispatchGroup`].
     pub fn enter(&self) -> DispatchGroupGuard {
-        // Safety: object cannot be null.
+        // SAFETY: TODO: Is it a soundness requirement that this is paired with leave?
         unsafe { dispatch_group_enter(self) };
 
         DispatchGroupGuard(self.retain())
@@ -96,7 +88,7 @@ impl DispatchGroupGuard {
 
 impl Drop for DispatchGroupGuard {
     fn drop(&mut self) {
-        // SAFETY: Dispatch group cannot be null.
+        // SAFETY: TODO: Is it a soundness requirement that this is paired with enter?
         unsafe { DispatchGroup::leave(&self.0) };
     }
 }

crates/dispatch2/src/lib.rs

@@ -113,8 +113,8 @@ pub use self::main_thread_bound::{run_on_main, MainThreadBound};
 pub use self::object::{DispatchObject, QualityOfServiceClassFloorError};
 pub use self::once::DispatchOnce;
 pub use self::queue::{
-    DispatchQueue, DispatchQueueAttr, GlobalQueueIdentifier, QueueAfterError, QueueAttribute,
-    QueuePriority,
+    dispatch_main, DispatchQueue, DispatchQueueAttr, GlobalQueueIdentifier, QueueAfterError,
+    QueueAttribute, QueuePriority,
 };
 pub use self::retained::DispatchRetained;
 pub use self::semaphore::{DispatchSemaphore, DispatchSemaphoreGuard};

crates/dispatch2/src/object.rs

@@ -25,6 +25,7 @@ pub unsafe trait DispatchObject {
     ///
     /// This extends the duration in which the object is alive by detaching it
     /// from the lifetime information carried by the reference.
+    #[doc(alias = "dispatch_retain")]
     fn retain(&self) -> DispatchRetained<Self> {
         let ptr: NonNull<Self> = NonNull::from(self);
         // SAFETY:
@@ -94,19 +95,19 @@ pub unsafe trait DispatchObject {
     /// Activate the object.
     fn activate(&mut self) {
         // Safety: object cannot be null.
-        unsafe { dispatch_activate(self.as_raw()) };
+        dispatch_activate(self.as_raw());
     }
 
     /// Suspend the invocation of functions on the object.
     fn suspend(&self) {
         // Safety: object cannot be null.
-        unsafe { dispatch_suspend(self.as_raw()) };
+        dispatch_suspend(self.as_raw());
     }
 
     /// Resume the invocation of functions on the object.
     fn resume(&self) {
         // Safety: object cannot be null.
-        unsafe { dispatch_resume(self.as_raw()) };
+        dispatch_resume(self.as_raw());
     }
 
     #[doc(hidden)]

crates/dispatch2/src/queue.rs

@@ -102,37 +102,40 @@ impl DispatchQueue {
     pub fn new(label: &str, queue_attribute: QueueAttribute) -> DispatchRetained<Self> {
         let label = CString::new(label).expect("Invalid label!");
 
-        // Safety: label and queue_attribute can only be valid.
-        unsafe { dispatch_queue_create(label.as_ptr(), queue_attribute.into()) }
+        // SAFETY: The label is a valid C string.
+        unsafe { Self::__new(label.as_ptr(), queue_attribute.into()) }
     }
 
     /// Create a new [`DispatchQueue`] with a given target [`DispatchQueue`].
     pub fn new_with_target(
         label: &str,
         queue_attribute: QueueAttribute,
-        target: &DispatchQueue,
+        target: Option<&DispatchQueue>,
     ) -> DispatchRetained<Self> {
         let label = CString::new(label).expect("Invalid label!");
 
-        // Safety: label, queue_attribute and target can only be valid.
-        // NOTE: dispatch_queue_create_with_target is in charge of retaining the target DispatchQueue.
-        unsafe {
-            dispatch_queue_create_with_target(label.as_ptr(), queue_attribute.into(), Some(target))
-        }
+        // SAFETY: The label is a valid C string.
+        unsafe { Self::__new_with_target(label.as_ptr(), queue_attribute.into(), target) }
     }
 
     /// Return a system-defined global concurrent [`DispatchQueue`] with the priority derived from [GlobalQueueIdentifier].
     pub fn global_queue(identifier: GlobalQueueIdentifier) -> DispatchRetained<Self> {
         let raw_identifier = identifier.to_identifier();
 
         // Safety: raw_identifier cannot be invalid, flags is reserved.
-        unsafe { dispatch_get_global_queue(raw_identifier, 0) }
+        dispatch_get_global_queue(raw_identifier, 0)
     }
 
     /// Return the main queue.
-    pub fn main() -> DispatchRetained<Self> {
-        // Safety: raw_identifier cannot be invalid, flags is reserved.
-        dispatch_get_main_queue().retain()
+    // TODO: Mark this as `const` once in MSRV.
+    #[inline]
+    #[doc(alias = "dispatch_get_main_queue")]
+    pub fn main() -> &'static Self {
+        // Inline function in the header
+
+        // SAFETY: The main queue is safe to access from anywhere, and is
+        // valid forever.
+        unsafe { &_dispatch_main_q }
     }
 
     /// Submit a function for synchronous execution on the [`DispatchQueue`].
@@ -147,7 +150,7 @@ impl DispatchQueue {
         // it here.
         //
         // Safety: object cannot be null and work is wrapped to avoid ABI incompatibility.
-        unsafe { dispatch_sync_f(self, work_boxed, function_wrapper::<F>) }
+        unsafe { Self::exec_sync_f(self, work_boxed, function_wrapper::<F>) }
     }
 
     /// Submit a function for asynchronous execution on the [`DispatchQueue`].
@@ -160,7 +163,7 @@ impl DispatchQueue {
         let work_boxed = Box::into_raw(Box::new(work)).cast();
 
         // Safety: object cannot be null and work is wrapped to avoid ABI incompatibility.
-        unsafe { dispatch_async_f(self, work_boxed, function_wrapper::<F>) }
+        unsafe { Self::exec_async_f(self, work_boxed, function_wrapper::<F>) }
     }
 
     /// Enqueue a function for execution at the specified time on the [`DispatchQueue`].
@@ -173,9 +176,7 @@ impl DispatchQueue {
         let work_boxed = Box::into_raw(Box::new(work)).cast();
 
         // Safety: object cannot be null and work is wrapped to avoid ABI incompatibility.
-        unsafe {
-            dispatch_after_f(when, self, work_boxed, function_wrapper::<F>);
-        }
+        unsafe { Self::exec_after_f(when, self, work_boxed, function_wrapper::<F>) };
 
         Ok(())
     }
@@ -190,7 +191,7 @@ impl DispatchQueue {
         let work_boxed = Box::into_raw(Box::new(work)).cast();
 
         // Safety: object cannot be null and work is wrapped to avoid ABI incompatibility.
-        unsafe { dispatch_barrier_async_f(self, work_boxed, function_wrapper::<F>) }
+        unsafe { Self::barrier_async_f(self, work_boxed, function_wrapper::<F>) }
     }
 
     /// Enqueue a barrier function for synchronous execution on the [`DispatchQueue`] and wait until that function completes.
@@ -201,7 +202,7 @@ impl DispatchQueue {
         let work_boxed = Box::into_raw(Box::new(work)).cast();
 
         // Safety: object cannot be null and work is wrapped to avoid ABI incompatibility.
-        unsafe { dispatch_barrier_sync_f(self, work_boxed, function_wrapper::<F>) }
+        unsafe { Self::barrier_sync_f(self, work_boxed, function_wrapper::<F>) }
     }
 
     /// Submit a function for synchronous execution and mark the function as a barrier for subsequent concurrent tasks.
@@ -214,7 +215,7 @@ impl DispatchQueue {
         let work_boxed = Box::into_raw(Box::new(work)).cast();
 
         // Safety: object cannot be null and work is wrapped to avoid ABI incompatibility.
-        unsafe { dispatch_barrier_async_and_wait_f(self, work_boxed, function_wrapper::<F>) }
+        unsafe { Self::barrier_async_and_wait_f(self, work_boxed, function_wrapper::<F>) }
     }
 
     /// Sets a function at the given key that will be executed at [`DispatchQueue`] destruction.
@@ -254,6 +255,18 @@ dispatch_object!(
 
 dispatch_object_not_data!(unsafe DispatchQueueAttr);
 
+/// Executes blocks submitted to the main queue.
+pub fn dispatch_main() -> ! {
+    extern "C" {
+        // `dispatch_main` is marked DISPATCH_NOTHROW.
+        fn dispatch_main() -> !;
+    }
+
+    // SAFETY: TODO: Must this be run on the main thread? Do we need to take
+    // `MainThreadMarker`?
+    unsafe { dispatch_main() }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

crates/dispatch2/src/semaphore.rs

@@ -16,20 +16,6 @@ dispatch_object!(
 dispatch_object_not_data!(unsafe DispatchSemaphore);
 
 impl DispatchSemaphore {
-    /// Creates a new [`DispatchSemaphore`] with an initial value.
-    ///
-    /// Returns None if value is negative or if creation failed.
-    pub fn new(value: isize) -> Option<DispatchRetained<Self>> {
-        // Per documentation creating a semaphore with a negative size isn't allowed.
-        if value < 0 {
-            return None;
-        }
-
-        // SAFETY: The value is valid.
-        // TODO: Allow this to be NULL.
-        Some(unsafe { dispatch_semaphore_create(value) })
-    }
-
     /// Attempt to acquire the [`DispatchSemaphore`] and return a [`DispatchSemaphoreGuard`].
     ///
     /// # Errors
@@ -48,7 +34,7 @@ impl DispatchSemaphore {
         };
 
         // Safety: DispatchSemaphore cannot be null.
-        let result = unsafe { Self::wait(self, timeout) };
+        let result = Self::wait(self, timeout);
 
         match result {
             0 => Ok(DispatchSemaphoreGuard(self.retain())),
@@ -67,7 +53,7 @@ impl DispatchSemaphoreGuard {
         let this = ManuallyDrop::new(self);
 
         // SAFETY: DispatchSemaphore cannot be null.
-        let result = unsafe { DispatchSemaphore::signal(&this.0) };
+        let result = this.0.signal();
 
         result != 0
     }
@@ -76,6 +62,6 @@ impl DispatchSemaphoreGuard {
 impl Drop for DispatchSemaphoreGuard {
     fn drop(&mut self) {
         // SAFETY: DispatchSemaphore cannot be null.
-        unsafe { DispatchSemaphore::signal(&self.0) };
+        self.0.signal();
     }
 }

crates/dispatch2/src/utils.rs

@@ -13,8 +13,8 @@ impl TryFrom<Duration> for dispatch_time_t {
         secs.checked_mul(1_000_000_000)
             .and_then(|x| x.checked_add(i64::from(value.subsec_nanos())))
             .map(|delta| {
-                // Safety: delta cannot overflow
-                unsafe { dispatch_time(DISPATCH_TIME_NOW, delta) }
+                // delta cannot overflow
+                dispatch_time(DISPATCH_TIME_NOW, delta)
             })
             .ok_or(())
     }

crates/dispatch2/src/workloop.rs

@@ -29,13 +29,10 @@ impl DispatchWorkloop {
 
     /// Configure how the [`DispatchWorkloop`] manage the autorelease pools for the functions it executes.
     pub fn set_autorelease_frequency(&self, frequency: DispatchAutoReleaseFrequency) {
-        // Safety: object and frequency can only be valid.
-        unsafe {
-            dispatch_workloop_set_autorelease_frequency(
-                self,
-                dispatch_autorelease_frequency_t::from(frequency),
-            );
-        }
+        dispatch_workloop_set_autorelease_frequency(
+            self,
+            dispatch_autorelease_frequency_t::from(frequency),
+        )
     }
 }