Xudong-Huang / generator-rs

rust stackful generator library
Apache License 2.0
286 stars 35 forks source link

Current scoped API is unsound #59

Open zetanumbers opened 3 weeks ago

zetanumbers commented 3 weeks ago
use std::{mem::forget, sync::mpsc};

use generator::Gn;

fn main() {
    let mut s = "Happy".to_owned();
    let (tx1, rx1) = mpsc::sync_channel(1);
    let (tx2, rx2) = mpsc::sync_channel(1);
    {
        let s = s.as_str();
        let mut coroutine = Gn::<()>::new_scoped_local(|mut yielder| {
            std::thread::scope(|scope| {
                let _thrd = scope.spawn(move || {
                    rx1.recv().unwrap();
                    tx2.send(s.to_owned()).unwrap();
                });
                yielder.yield_with(());
            });
            panic!();
        });
        coroutine.resume();
        forget(coroutine);
    }
    s.clear();
    s.push_str("Sad");
    tx1.send(()).unwrap();
    let data_race_s = rx2.recv().unwrap();
    dbg!(&s, &data_race_s);
}

Prints out:

[src/main.rs:28:5] &s = "Sad"
[src/main.rs:28:5] &data_race_s = "Sadpy"

Similar to https://github.com/Amanieu/corosensei/issues/27

Xudong-Huang commented 3 weeks ago

Indeed, you define a smart way to leak the local reference to a thread. However this is not a normal use case for std::thread::scope .

I think we should enforce the scoped coroutine type not forgettable, but currently rust doesn't support that mechenism.

Xudong-Huang commented 3 weeks ago

https://hackmd.io/@wg-async/S1Q6Leam0?utm_source=preview-mode&utm_medium=rec

zetanumbers commented 3 weeks ago

I think we should enforce the scoped coroutine type not forgettable, but currently rust doesn't support that mechenism.

That's how I have come to this code sample, I've assumed the generator should implement negative of any auto trait due to inability to analyze local variables within a generator, which would include the missing Forget trait.

However this is not a normal use case for std::thread::scope.

I use roughly the same mechanism as with std::thread::scope in the scope-lock library for lifetime extension, which I think is quite often desirable. Basically the example from the top message implements one way longjmp which I think can be reasonably deemed unsafe or UB cause.

https://hackmd.io/@wg-async/S1Q6Leam0?utm_source=preview-mode&utm_medium=rec

Hey, that's actually my proposal! 😄 I appreciate this reference.