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

ScopedCoroutine is unsound #27

Closed
zetanumbers opened this issue Jun 4, 2024 · 4 comments · Fixed by #28
Closed

ScopedCoroutine is unsound #27

zetanumbers opened this issue Jun 4, 2024 · 4 comments · Fixed by #28

Comments

@zetanumbers
Copy link
Contributor

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
Copy link
Contributor Author

To my estimates Coroutine should not be affected by this.

Amanieu added a commit that referenced this issue Jun 10, 2024
This is unsound because it allows values on the stack to be forgotten
without running their associated drop code, which is unsound for
constructs like scoped threads.

Fixes #27
Amanieu added a commit that referenced this issue Jun 10, 2024
This is unsound because it allows values on the stack to be forgotten
without running their associated drop code, which is unsound for
constructs like scoped threads.

Fixes #27
@zetanumbers
Copy link
Contributor Author

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
Copy link
Contributor Author

zetanumbers commented Sep 10, 2024

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.

@zetanumbers
Copy link
Contributor Author

zetanumbers commented Oct 5, 2024

Sorry, I've found it just yesterday, didn't report it in #28. Example of UB without ScopeCoroutine:

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);
    {
        let s = s.as_str();
        let mut coroutine = Coroutine::new(|yielder, s: &str| {
            std::thread::scope(move |scope| {
                let _thrd = scope.spawn(move || {
                    rx1.recv().unwrap();
                    tx2.send(s.to_owned()).unwrap();
                });
                yielder.suspend(());
                panic!();
            });
        });
        coroutine.resume(s);
        forget(coroutine);
    }
    s.clear();
    s.push_str("Sad");
    tx1.send(()).unwrap();
    let data_race_s = rx2.recv().unwrap();
    dbg!(&s, &data_race_s);
}

Basically there should be 'static bounds on generic argument types. Should this issue be reopened?

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

Successfully merging a pull request may close this issue.

1 participant