Amanieu / corosensei

A fast and safe implementation of stackful coroutines in Rust
Apache License 2.0
178 stars 13 forks source link

`ScopedCoroutine` is unsound #27

Open zetanumbers opened 4 months ago

zetanumbers commented 4 months ago
use std::{mem::forget, sync::mpsc};

use corosensei::ScopedCoroutine;

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 = ScopedCoroutine::new(|yielder, ()| {
            std::thread::scope(move |scope| {
                let _thrd = scope.spawn(move || {
                    rx1.recv().unwrap();
                    tx2.send(s.to_owned()).unwrap();
                });
                let () = yielder.suspend(());
            });
        });
        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:27:5] &s = "Sad"
[src/main.rs:27:5] &data_race_s = "Sadpy"
zetanumbers commented 4 months ago

To my estimates Coroutine should not be affected by this.

zetanumbers commented 3 weeks ago

Also the regular Coroutine conflicts with the scoped-tls crate:

#!/usr/bin/env -S cargo +nightly -Zscript
---
[package]
edition = "2021"
[dependencies]
corosensei = "0.1.4"
scoped-tls = "1.0.1"
---
use std::{mem::forget, sync::mpsc};

use corosensei::Coroutine;
use scoped_tls::scoped_thread_local;

scoped_thread_local!(static S: String);

fn main() {
    let mut s = "Happy".to_owned();
    let (tx1, rx1) = mpsc::sync_channel(1);
    let (tx2, rx2) = mpsc::sync_channel(1);
    S.set(&s, || {
        let mut coroutine = Coroutine::new(|yielder, ()| {
            S.with(|s| {
                let s = s.as_str();
                std::thread::scope(move |scope| {
                    let _thrd = scope.spawn(move || {
                        rx1.recv().unwrap();
                        tx2.send(s.to_owned()).unwrap();
                    });
                    let () = yielder.suspend(());
                });
            })
        });
        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);
}
zetanumbers commented 3 weeks ago

Surprisingly my scope-lock library isn't susceptible to this kind of soundness issue, it just deadlocks before lock_scope ever returns:

#!/usr/bin/env -S cargo +nightly -Zscript
---
[package]
edition = "2021"
[dependencies]
corosensei = "0.1.4"
scope-lock = "0.2.5"
---
use std::{mem::forget, sync::mpsc};

use corosensei::Coroutine;

fn main() {
    let mut s = "Happy".to_owned();
    let (tx1, rx1) = mpsc::sync_channel(1);
    let (tx2, rx2) = mpsc::sync_channel(1);
    scope_lock::lock_scope(|e| {
        let s = s.as_str();
        let clone_s = e.fn_once(Box::new(|()| s.to_owned()));
        let mut coroutine = Coroutine::new(|yielder, ()| {
            std::thread::scope(move |scope| {
                let _thrd = scope.spawn(move || {
                    rx1.recv().unwrap();
                    tx2.send(clone_s(())).unwrap();
                });
                let () = yielder.suspend(());
            });
        });
        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);
}

I guess the issue is that the scoped-tls assumes the input reference's lifetime cannot end before ScopedKey::with returns due to the ordered nature of subroutines (and it assumes every Rust function is a subroutine). Ideally I personally wouldn't make that assumption about Rust but that would be an interpretation issue I guess, which does not exactly have an objective answer I believe.

Interestingly if you implement scoped-tls by using scope-lock, that would also solve the issue. It just shows that there's a fix for this. Maybe there could be a smaller crate to regulate which interpretation is correct at compile time.