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

Gn::new_scoped and similar functions are unsound #49

Closed
SkiFire13 opened this issue Sep 29, 2023 · 3 comments
Closed

Gn::new_scoped and similar functions are unsound #49

SkiFire13 opened this issue Sep 29, 2023 · 3 comments

Comments

@SkiFire13
Copy link

Gn::new_scoped allows a coroutine to borrow non-'static data, but it doesn't (and can't) guarantee it will end before the lifetime expires since the coroutine may be leaked during a yield point. This is unsound because Rust guarantees that sync code either reaches the end of its execution (normally or with a panic doesn't matter) or the process ends. Leaking a coroutine prevents the code inside from actually ending normally or with a panic, thus it must be considered as if it is still executing (albeit without making any progress) until the end of the process. This however requires it to not borrow non-'static data, which is however what Gn::new_scoped allows to.

The API that most evidently relies on guarantee mentioned is std::thread::scope, and in fact here is an example of use-after-free due ot its assumptions being violated:

let foo = "foo".to_string();

let mut gen = generator::Gn::new_scoped(|mut s| {
    std::thread::scope(|s2| {
        s2.spawn(|| {
            std::thread::sleep(std::time::Duration::from_millis(500));
            println!("{foo}");
        });
        s.yield_(());
    });
    generator::done!();
});

gen.next();
std::mem::forget(gen);
drop(foo);
std::thread::sleep(std::time::Duration::from_millis(1000));
@Xudong-Huang
Copy link
Owner

oh, seems that we have to remove the non-static lifetime bound, or change the related API to unsafe

@Xudong-Huang
Copy link
Owner

Xudong-Huang commented Oct 4, 2023

now only static bound capture is safe to yield, please review if the change make sense @SkiFire13

@SkiFire13
Copy link
Author

Yes the issue seems to be solved with that commit, or at least I don't see an obvious way to circumvent the restrictions you added.

hack3ric added a commit to hack3ric/system76-scheduler that referenced this issue Nov 19, 2024
Fixes pop-os#116. Since currently config::configuration_files is only used with 'static, we change all of its lifetime to 'static.

Ref: Xudong-Huang/generator-rs#49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants