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.

Sign up for GitHub

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

Jump to bottom

Generator's Send trait should have bounds #27

Closed
ammaraskar opened this issue Nov 16, 2020 · 5 comments
Closed

Generator's Send trait should have bounds #27

ammaraskar opened this issue Nov 16, 2020 · 5 comments

Comments

@ammaraskar
Copy link

Hi there, we (Rust group @sslab-gatech) are scanning crates on crates.io for potential soundness bugs. We noticed that Generator implements Send as long as the closure has a static lifetime. However, this should also probably be bounded by T: Send, otherwise it's possible to smuggle across non-Send types across thread boundaries.

Here's an example of a data race in safe Rust code through a Generator.

#![forbid(unsafe_code)]

use generator::Gn;
use std::{rc::Rc, thread};

fn main() {
    let rc = Rc::new(());

    let rc_clone = rc.clone();
    let mut generator = Gn::new_scoped(move |_| {
        return rc_clone;
    });

    let child = thread::spawn(move || {
        let smuggled_rc = generator.next().unwrap();

        println!("RC in thread: {:p}", smuggled_rc);
        for _ in 0..1000000000 {
            let x = smuggled_rc.clone();
        }
    });

    println!("RC in main: {:p}", rc);
    for _ in 0..1000000000 {
        let x = rc.clone();
    }

    child.join().unwrap();
    assert_eq!(Rc::strong_count(&rc), 2);
}

Output:

RC in main: 0x5587d9ed6a50
RC in thread: 0x5587d9ed6a50

Return Code: -4 (SIGILL)
@Xudong-Huang
Copy link
Owner

Thanks for this info! I will try to fix that.

Xudong-Huang added a commit that referenced this issue Dec 10, 2020
@Xudong-Huang
🐛 fix generator send bound (#27)
5b499b3
Xudong-Huang added a commit that referenced this issue Dec 10, 2020
@Xudong-Huang
🐛 fix generator send bound (#27)
4a9d4b9
@Xudong-Huang
Copy link
Owner

I think this fix would break some existing code for the more strict bound check. I will create a 0.7.0 release for the changes.

@Xudong-Huang
Copy link
Owner

Xudong-Huang commented Dec 13, 2020 •
edited
Loading

  1. The functor that passed into the creation determines whether the generator could be send. If the functor is not send, then the generator is not send.
  2. The argument type should also be send for that the generator could hold it for any usage
  3. The return type should also be send, the generator could move to another thread, must make sure its return value is send.

We have to do more complex checks to determine if the generator is send. The implementation is not that easy.

Xudong-Huang added a commit that referenced this issue Mar 30, 2021
@Xudong-Huang
📝 fix send issue (#27)
f7d120a
@Xudong-Huang
Copy link
Owner

a new release 0.7 should be published soon

@ammaraskar ammaraskar mentioned this issue Mar 30, 2021
Merged
This was referenced Mar 31, 2021
Open
Open
Open
Open
Closed
Closed
Closed
@github-actions github-actions bot mentioned this issue Apr 15, 2021
Closed
@github-actions github-actions bot mentioned this issue Apr 26, 2021
Closed
@github-actions github-actions bot mentioned this issue Nov 27, 2021
Closed
@github-actions github-actions bot mentioned this issue Feb 27, 2022
Closed
@zetanumbers
Copy link

zetanumbers commented May 20, 2024 •
edited
Loading

Sadly sendable Generator is currently unsound because thread local variables may leak non-sendable values from within the our coroutine.

See https://blaz.is/blog/post/lets-pretend-that-task-equals-thread/ and https://users.rust-lang.org/t/controversial-opinion-keeping-rc-across-await-should-not-make-future-send-by-itself/100554/12 for details.

Opened separate #58

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ammaraskar @Xudong-Huang @zetanumbers