Using Thread local storage is unsafe

[ghost]

This is probably not the only api that is affected.

Right now:

• A coroutine may migrate from one thread to another
• A coroutine may yield inside of TLS with block.

Because of this, it is easy to build a simple test case that violates TLS rules.

extern crate coroutine;

use coroutine::{spawn, sched};
use std::cell::RefCell;

thread_local!(static LOCAL: RefCell<u32> = RefCell::new(7));

fn main() {
    let coro = spawn(move|| {
        LOCAL.with(|x| {
            *x.borrow_mut() += 1;
            let ptr: *const RefCell<u32> = x;
            println!("before: {:?} {:?} thread: {:?}", ptr, x, std::thread::current());
            sched();
            let ptr: *const RefCell<u32> = x;
            println!("after: {:?} {:?} thread: {:?}", ptr, x, std::thread::current());
        });
    });

    coro.resume().ok().expect("Failed to resume");

    std::thread::spawn(move || {
        coro.resume().ok().expect("Failed to resume");
    }).join().unwrap();
}

This will print:

before: 0x7f940205a728 RefCell { value: 8 } thread: Some("<main>")
after: 0x7f940205a728 RefCell { value: 8 } thread: None

[zonyitoo]

WOW.... That's true. Do you have any good ideas?

[zonyitoo]

One of my suggestion is

spawn(move|env: &mut Environemnt| {
    // Pass the environment into the coroutine
});

But one of our goal is to implement M:N model, so this may not work...

[ghost]

I think I have a scheme that will work. It requires changes + new API in the standard library, but does not break the standard library.

Rust adds a borrow count to the TLS structure. Every time a with block is opened, the number is bumped, and when it returns the number is decremented. An additions api is added that gives read only access to this number. get_tls_borrow_count().

When a coroutine is called, before the stack switch the borrow count is read. When the coroutine sched() the parent can then read read the borrow_count. If the borrow count is the same value as before, it safe to Send the stack since the stack does not contain a TLS reference.

There are one of two ways to expose this to the API user. First, you can panic!() if the count changes since the coroutine has violated a rule. Secondly, you can record the number of TLS references inside of the Handle. If the the count is equal to 0, you can convert a Handle to a SendableHandle which can be sent to another thread. A SendableHandle could then be thawed into a Handle and the life cycle could occur again an again.

[zonyitoo]

That looks good. But I don't think Rust team will accept this change, because they are not going to support this kind of stackful coroutines in the language.

This really is a problem. @doomsplayer Do you have any idea?

[dovahcrow]

Taking mid term exams these days.I'll consider this later.

On 2015年5月2日周六 上午1:43 Y. T. CHUNG [email protected] wrote:

That looks good. But I don't think Rust team will accept this change,
because they are not going to support this kind of stackful coroutines in
the language.

This really is a problem. @doomsplayer https://github.com/doomsplayer
Do you have any idea?

—
Reply to this email directly or view it on GitHub
#31 (comment).

[polyfractal]

I think this is, unfortunately, just "expected behavior" given the state of Clang. It's a known issue but has not been fixed / addressed. MSVC has a /GT flag which prevents caching of the TLS addresses.

Is there a way to ban the thread_local!() macro in coroutines? Or perhaps provide a specialized fiber_local!() macro that is similar but has extra logic to migrate with the fiber between threads?

[zonyitoo]

@polyfractal I just thought about your second idea today. Provide a customized macro, such as fiber_local!() as you said, is a good way to solve this issue. But the problem is still exist, and I guessed that all those libraries, which use similar mechanism as LocalKey::with, may be affected.

One radical solution, as discussed with @doomsplayer, is to rewrite the whole std and core crates, which will supports non-blocking I/O and use fibers as the default concurrency mechanism with compatible APIs. Or find a way to monkey patch those libraries in runtime (is that possible?).

[therustmonk]

@zonyitoo Maybe we have to reject Send implementation for Closure? Which consequences we will take? Send means safe for TLS, but we can't ensure that.

Another thing is Rc cannot be used, because it doesn't implement Send but, I think it's really safe to use non-atomic reference counter inside coroutine, doesn't it?

We can add own ClosureSend marker-trait and implement it for Rc and other unsafe for sending structs, but I'm not sure it will work.

[zonyitoo]

@deniskolodin Well, forbidding Send for the Closure is not very user friendly, which will generate many compile errors...

I am fully aware that using TLS is very unsafe, which is critical in zonyitoo/coio-rs#56 .