Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Queue::exec_async can be mis-used with just safe code #689

Closed
jesdazrez opened this issue Jan 6, 2025 · 3 comments · Fixed by #690
Closed

Queue::exec_async can be mis-used with just safe code #689

jesdazrez opened this issue Jan 6, 2025 · 3 comments · Fixed by #690
Labels
A-dispatch2 Affects the `dispatch2` crate bug Something isn't working I-unsound A soundness hole

Comments

@jesdazrez
Copy link
Contributor

If the closure passed to exec_async contains a reference, it might be executed even if the referenced value is gone / has changed. It's also possible to end up with +1 &mut T.

Wonder if there's something that could be done, like a 'static bound or something similar?

Example:

use core_foundation::runloop::CFRunLoopRun;
use dispatch2::Queue;
use std::thread;

struct X {
    y: u8,
}

impl Drop for X {
    fn drop(&mut self) {
        println!("dropping X {{ {} }}", self.y);
    }
}

fn mutate(x: &mut X) {
    let th = thread::current();
    let th = th.name();
    println!("mutating x ({}) from {th:?}", x.y);
    x.y += 1;
    println!(
        "x mutated (to ({})) from {th:?}, releasing exclusive access",
        x.y
    );
}

fn main() {
    thread::spawn(|| {
        let mut x = X { y: 1 };
        let queue = Queue::main();
        queue.exec_async(|| {
            mutate(&mut x);
        });
        mutate(&mut x);
    });
    unsafe { CFRunLoopRun() };
}

Output example (it might segfault instead):
Screenshot 2025-01-06 at 9 37 30 a m

@madsmtm madsmtm added I-unsound A soundness hole A-dispatch2 Affects the `dispatch2` crate bug Something isn't working labels Jan 6, 2025
@madsmtm
Copy link
Owner

madsmtm commented Jan 6, 2025

Indeed, we need a + 'static bound on the closure.

(I haven't really gotten around to fully reviewing dispatch2 (tracked in #77), so that's why this issue exists)

@jesdazrez
Copy link
Contributor Author

thanks for the quick answer!, let me know if you want some help with a PR

@madsmtm
Copy link
Owner

madsmtm commented Jan 6, 2025

I'm somewhat busy with finishing a long overdue release, so a PR would be nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dispatch2 Affects the `dispatch2` crate bug Something isn't working I-unsound A soundness hole
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants