[breaking][core-graphics] Fix unsoundness in CGEventTap API (#710)

* Fix unsoundness in CGEventTap API

* Fix lifetime unsoundness in CGEventTap, add ::with

Author: tmandry

core-graphics/src/event.rs

@@ -6,7 +6,7 @@ use bitflags::bitflags;
 use core::ffi::{c_ulong, c_void};
 use core_foundation::{
     base::{CFRelease, CFRetain, CFTypeID, TCFType},
-    mach_port::{CFMachPort, CFMachPortRef},
+    mach_port::{CFMachPort, CFMachPortInvalidate, CFMachPortRef},
 };
 use foreign_types::{foreign_type, ForeignType};
 use std::mem::ManuallyDrop;
@@ -417,7 +417,7 @@ macro_rules! CGEventMaskBit {
 }
 
 pub type CGEventTapProxy = *const c_void;
-pub type CGEventTapCallBackFn<'tap_life> =
+type CGEventTapCallBackFn<'tap_life> =
     Box<dyn Fn(CGEventTapProxy, CGEventType, &CGEvent) -> Option<CGEvent> + 'tap_life>;
 type CGEventTapCallBackInternal = unsafe extern "C" fn(
     proxy: CGEventTapProxy,
@@ -443,51 +443,81 @@ unsafe extern "C" fn cg_event_tap_callback_internal(
 }
 
 /// ```no_run
-///use core_foundation::runloop::{kCFRunLoopCommonModes, CFRunLoop};
-///use core_graphics::event::{CGEventTap, CGEventTapLocation, CGEventTapPlacement, CGEventTapOptions, CGEventType};
-///let current = CFRunLoop::get_current();
-///match CGEventTap::new(
+/// use core_foundation::runloop::{kCFRunLoopCommonModes, CFRunLoop};
+/// use core_graphics::event::{CGEventTap, CGEventTapLocation, CGEventTapPlacement, CGEventTapOptions, CGEventType};
+/// let current = CFRunLoop::get_current();
+///
+/// CGEventTap::with(
 ///     CGEventTapLocation::HID,
 ///     CGEventTapPlacement::HeadInsertEventTap,
 ///     CGEventTapOptions::Default,
 ///     vec![CGEventType::MouseMoved],
-///     |_a, _b, d| {
-///         println!("{:?}", d.location());
+///     |_proxy, _type, event| {
+///         println!("{:?}", event.location());
 ///         None
 ///     },
-/// ) {
-///     Ok(tap) => unsafe {
+///     |tap| {
 ///         let loop_source = tap
-///             .mach_port
+///             .mach_port()
 ///             .create_runloop_source(0)
-///             .expect("Somethings is bad ");
-///         current.add_source(&loop_source, kCFRunLoopCommonModes);
+///             .expect("Runloop source creation failed");
+///         current.add_source(&loop_source, unsafe { kCFRunLoopCommonModes });
 ///         tap.enable();
 ///         CFRunLoop::run_current();
 ///     },
-///     Err(_) => (assert!(false)),
-/// }
+/// ).expect("Failed to install event tap");
 /// ```
 pub struct CGEventTap<'tap_life> {
-    pub mach_port: CFMachPort,
-    pub callback_ref:
-        Box<dyn Fn(CGEventTapProxy, CGEventType, &CGEvent) -> Option<CGEvent> + 'tap_life>,
+    mach_port: CFMachPort,
+    _callback: Box<CGEventTapCallBackFn<'tap_life>>,
 }
 
-impl<'tap_life> CGEventTap<'tap_life> {
-    pub fn new<F: Fn(CGEventTapProxy, CGEventType, &CGEvent) -> Option<CGEvent> + 'tap_life>(
+impl CGEventTap<'static> {
+    pub fn new<F: Fn(CGEventTapProxy, CGEventType, &CGEvent) -> Option<CGEvent> + 'static>(
         tap: CGEventTapLocation,
         place: CGEventTapPlacement,
         options: CGEventTapOptions,
         events_of_interest: std::vec::Vec<CGEventType>,
         callback: F,
-    ) -> Result<CGEventTap<'tap_life>, ()> {
+    ) -> Result<Self, ()> {
+        // SAFETY: callback is 'static so even if this object is forgotten it
+        // will be valid to call.
+        unsafe { Self::new_unchecked(tap, place, options, events_of_interest, callback) }
+    }
+}
+
+impl<'tap_life> CGEventTap<'tap_life> {
+    pub fn with<R>(
+        tap: CGEventTapLocation,
+        place: CGEventTapPlacement,
+        options: CGEventTapOptions,
+        events_of_interest: std::vec::Vec<CGEventType>,
+        callback: impl Fn(CGEventTapProxy, CGEventType, &CGEvent) -> Option<CGEvent> + 'tap_life,
+        with_fn: impl FnOnce(&Self) -> R,
+    ) -> Result<R, ()> {
+        // SAFETY: We are okay to bypass the 'static restriction because the
+        // event tap is dropped before returning. The callback therefore cannot
+        // be called after its lifetime expires.
+        let event_tap: Self =
+            unsafe { Self::new_unchecked(tap, place, options, events_of_interest, callback)? };
+        Ok(with_fn(&event_tap))
+    }
+
+    /// Caller is responsible for ensuring that this object is dropped before
+    /// `'tap_life` expires.
+    pub unsafe fn new_unchecked(
+        tap: CGEventTapLocation,
+        place: CGEventTapPlacement,
+        options: CGEventTapOptions,
+        events_of_interest: std::vec::Vec<CGEventType>,
+        callback: impl Fn(CGEventTapProxy, CGEventType, &CGEvent) -> Option<CGEvent> + 'tap_life,
+    ) -> Result<Self, ()> {
         let event_mask: CGEventMask = events_of_interest
             .iter()
             .fold(CGEventType::Null as CGEventMask, |mask, &etype| {
                 mask | CGEventMaskBit!(etype)
             });
-        let cb = Box::new(Box::new(callback) as CGEventTapCallBackFn);
+        let cb: Box<CGEventTapCallBackFn> = Box::new(Box::new(callback));
         let cbr = Box::into_raw(cb);
         unsafe {
             let event_tap_ref = CGEventTapCreate(
@@ -502,7 +532,7 @@ impl<'tap_life> CGEventTap<'tap_life> {
             if !event_tap_ref.is_null() {
                 Ok(Self {
                     mach_port: (CFMachPort::wrap_under_create_rule(event_tap_ref)),
-                    callback_ref: Box::from_raw(cbr),
+                    _callback: Box::from_raw(cbr),
                 })
             } else {
                 let _ = Box::from_raw(cbr);
@@ -511,11 +541,21 @@ impl<'tap_life> CGEventTap<'tap_life> {
         }
     }
 
+    pub fn mach_port(&self) -> &CFMachPort {
+        &self.mach_port
+    }
+
     pub fn enable(&self) {
         unsafe { CGEventTapEnable(self.mach_port.as_concrete_TypeRef(), true) }
     }
 }
 
+impl Drop for CGEventTap<'_> {
+    fn drop(&mut self) {
+        unsafe { CFMachPortInvalidate(self.mach_port.as_CFTypeRef() as *mut _) };
+    }
+}
+
 foreign_type! {
     #[doc(hidden)]
     pub unsafe type CGEvent {