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

Keeping references to #[thread_local] statics is allowed across yields. #49682

Open
eddyb opened this Issue Apr 5, 2018 · 2 comments

Comments

Projects
None yet
4 participants
@eddyb
Member

eddyb commented Apr 5, 2018

This does not affect stabilization of async fn unless #[thread_local] is also stabilized

Try on playground:

#![feature(generators, generator_trait, thread_local)]

use std::ops::Generator;
use std::sync::atomic::{AtomicUsize, Ordering};
use std::thread;

#[thread_local]
static FOO: AtomicUsize = AtomicUsize::new(0);

fn foo() -> impl Generator<Yield = (String, usize), Return = ()> {
    static || {
        let x = &FOO;
        loop {
            let s = format!("{:p}", x);
            yield (s, x.fetch_add(1, Ordering::SeqCst));
        }
    }
}

fn main() {
    unsafe {
        let mut g = thread::spawn(|| {
            let mut g = foo();
            println!("&FOO = {:p}; resume() = {:?}", &FOO, g.resume());
            g
        }).join().unwrap();
        println!("&FOO = {:p}; resume() = {:?}", &FOO, g.resume());
        thread::spawn(move || {
            println!("&FOO = {:p}; resume() = {:?}", &FOO, g.resume());
        }).join().unwrap();
    }
}

Sample output:

&FOO = 0x7f48f9bff688; resume() = Yielded(("0x7f48f9bff688", 0))
&FOO = 0x7f48fafd37c8; resume() = Yielded(("0x7f48f9bff688", 1))
&FOO = 0x7f48f9bff688; resume() = Yielded(("0x7f48f9bff688", 0))

You can see by the pointer addresses and values inside FOO that the same location was reused for the second child thread (it's a bit harder to show a crash) - this is clearly an use-after-free.
If we had in-language async, the same problem could be demonstrated using those.

In non-generator functions, such references have function-local lifetimes and cannot escape.
With the stable thread_local! from libstd, user code gets access to the reference in a (non-generator/async) closure, which also doesn't allow escaping the reference.

cc @alexcrichton @withoutboats @Zoxc

@comex

This comment has been minimized.

Show comment
Hide comment
@comex

comex Apr 5, 2018

Contributor

Am I right in assuming the same issue applies to implementations of thread locals in external crates, which aren't limited to unstable? e.g. https://amanieu.github.io/thread_local-rs/thread_local/index.html

edit: I'm wrong. From that page:

Per-thread objects are not destroyed when a thread exits. Instead, objects are only destroyed when the ThreadLocal containing them is destroyed.

Otherwise the API would be blatantly unsound even without generators.

edit2: ignore me; I misunderstood the scope of the problem. For the record, it doesn't apply to thread_local!, or anything else that uses a callback.

Contributor

comex commented Apr 5, 2018

Am I right in assuming the same issue applies to implementations of thread locals in external crates, which aren't limited to unstable? e.g. https://amanieu.github.io/thread_local-rs/thread_local/index.html

edit: I'm wrong. From that page:

Per-thread objects are not destroyed when a thread exits. Instead, objects are only destroyed when the ThreadLocal containing them is destroyed.

Otherwise the API would be blatantly unsound even without generators.

edit2: ignore me; I misunderstood the scope of the problem. For the record, it doesn't apply to thread_local!, or anything else that uses a callback.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 12, 2018

Contributor

triage: P-medium

Should fix, not urgent.

Contributor

nikomatsakis commented Apr 12, 2018

triage: P-medium

Should fix, not urgent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment