[breaking][core-graphics] Fix unsoundness in CGEventTap API

[tmandry]

The user callback can be dropped while it is still referenced by the run loop. This PR fixes that by invaliding the event tap's mach port which prevents it from sending further events.

The callback_ref field was the wrong type. Of course it should never have been public, no one wants to access the callback to call it (and if they did they would probably segfault). In practice this just meant the "intermediate" box would leak on deallocation.

Both fields have to be made private for the API to remain sound, because their lifetimes are tied together: the port holds a pointer to the callback. Otherwise the mach port can be swapped out and we would invalidate the wrong one. This led to the first actually unfortunate breakage, which is that everyone needs to call .mach_port() now.

The second is that the callback passed to CGEventTap::new() must be 'static because otherwise the CGEventTap struct can be forgotten and the scope it references exited. An alternate CGEventTap::with() API has been added that accepts a non-'static callback.

Below is a test case that segfaults with the existing API and does nothing with the new one.

use core_foundation::runloop::{kCFRunLoopCommonModes, CFRunLoop};
use core_graphics::{
    event::{
        CGEvent, CGEventTap, CGEventTapLocation, CGEventTapOptions, CGEventTapPlacement,
        CGEventType, CGMouseButton,
    },
    event_source::CGEventSource,
    event_source::CGEventSourceStateID,
    geometry::CG_ZERO_POINT,
};

fn main() {
    let current = CFRunLoop::get_current();
    match CGEventTap::new(
        CGEventTapLocation::AnnotatedSession,
        CGEventTapPlacement::HeadInsertEventTap,
        CGEventTapOptions::ListenOnly,
        vec![CGEventType::LeftMouseUp],
        |_, _, _| {
            println!("Mouse up");
            CFRunLoop::get_current().stop();
            None
        },
    ) {
        Ok(tap) => unsafe {
            let loop_source = tap.mach_port.create_runloop_source(0).unwrap();
            // let loop_source = tap.mach_port().create_runloop_source(0).unwrap();
            current.add_source(&loop_source, kCFRunLoopCommonModes);
            tap.enable();

            CGEvent::new_mouse_event(
                CGEventSource::new(CGEventSourceStateID::CombinedSessionState).unwrap(),
                CGEventType::LeftMouseUp,
                CG_ZERO_POINT,
                CGMouseButton::Left,
            )
            .unwrap()
            .post(CGEventTapLocation::AnnotatedSession);

            // tap is dropped here
        },
        Err(_) => panic!("Could not create mouse event tap"),
    };

    CFRunLoop::run_current();
}

[madsmtm]

Yeah, this is a unfortunate limitation of the CGEventTapCreate API (that it doesn't allow setting either CFMachPortContext or "shouldFreeInfo" when creating the mach port).

The solution here is completely correct, though I wonder if perhaps we could (ab)use CFMachPortSetInvalidationCallBack to do the deallocation of the closure? This would make it so that we wouldn't have to keep the Box around ourselves, but could instead release it in there - That way, we don't have to force the CFMachPortInvalidate in new (would still be required for with), and we could allow the user to still access the mach_port field directly.

[wusyong]

@madsmtm Perhaps it could be future improvement?

We had some publish requests recently. I want to see if we can update some crate versions this week.

[madsmtm]

Perhaps it could be future improvement?

Not that much worth in an improvement in the future if we have already forced users to use .mach_port(), the main motivation would be to still allow the old pattern of accessing the mach_port field directly (thereby lessening the breakage of this PR). In any case, I don't have a stake in it.

[waywardmonkeys]

We had some publish requests recently. I want to see if we can update some crate versions this week.

Where?

[wusyong]

@waywardmonkeys I mean crates in whole repo. See comments in #706

[tmandry]

The solution here is completely correct, though I wonder if perhaps we could (ab)use CFMachPortSetInvalidationCallBack to do the deallocation of the closure? This would make it so that we wouldn't have to keep the Box around ourselves, but could instead release it in there - That way, we don't have to force the CFMachPortInvalidate in new (would still be required for with), and we could allow the user to still access the mach_port field directly.

You know I actually tried this first, but I don't think it can work. The invalidation callback only receives the info member of the CFMachPortContext, which cannot be set with CGEventTapCreate. I had hoped it would be the same as the refcon passed to CGEventTapCreate, but it isn't.

The other possibility I see is to inject a custom allocator that frees the callback when all its allocated objects have been freed.

Not that much worth in an improvement in the future if we have already forced users to use .mach_port(), the main motivation would be to still allow the old pattern of accessing the mach_port field directly (thereby lessening the breakage of this PR).

Actually, a simpler way of doing this might be to duplicate the mach port field and make one field private. We could invalidate the one in the private field, knowing that it hasn't been tampered with.

[madsmtm]

I had hoped it would be the same as the refcon passed to CGEventTapCreate, but it isn't.

Ah, I thought that was the case. Yeah, then there isn't any other way to do this.