Thread locals do not guard against recursive initialization.

[eddyb]

use std::cell::Cell;
struct Foo;
thread_local!(static INIT: Cell<bool> = Cell::new(false));
thread_local!(static FOO: Foo = Foo::init());
impl Foo {
    fn init() -> Foo {
        INIT.with(|i| {
            let init = i.get();
            i.set(true);
            if !init {
                // Trigger recursive initialization, just once.
                FOO.with(|_| {});
            }
        });
        Foo
    }
}
impl Drop for Foo {
    fn drop(&mut self) {
        FOO.with(|foo| alias_check(self, foo));
    }
}

fn alias_check<T>(m: &mut T, r: &T) {
    println!("alias_check({:p}, {:p})", m, r);
    if m as *mut _ as usize == r as *const _ as usize {
        panic!("aliasing detected!");
    }
}

fn main() {
    FOO.with(|_| {});
}

In current stable (1.4), beta, and nightly the above snippet produces aliased references instead of triggering library asserts.

Currently, libstd assumes there is no value already in the slot when assigning the one produced by the latest initializer call.

It should either drop one of the two values (perhaps after putting the TLS slot in "destructor being called" mode) or panic, if a multiple initialization condition is detected.

[alexcrichton]

triage: I-nominated

Seems bad! Haven't had too much of a chance to digest yet, but doesn't seem earth-shatteringly bad on the surface at least.

[alexcrichton]

I... actually don't think this is related to thread locals. The line you've highlighted I believe is indeed the "one at fault", but this seems like it may be a codegen problem?

Currently, for code like this:

pub fn foo(a: &mut Box<i32>, b: Box<i32>) {
    *a = b;
}

This is codegen'd as:

1. Drop the contents of a
2. Copy the contents of b into a

Note that this is precisely what's happening on that line of *ptr = Some(value). The destructor is running first, and then the value is being moved into place. This means that the destructor for the TLS value is running without setting the "I'm running a destructor flag", which is not intended! This means that destructors can access the value while it's being destroyed, causing the unsafety here.

This seems like an interesting phenomenon, however, because if the destructor in the function above panicked, that means that we've possibly got a hole in memory! As a proof of concept, let's take a look at this:

struct A(Box<i32>);
struct B<'a>(&'a mut A);

impl Drop for A {
    fn drop(&mut self) {
        if *self.0 == 0 {
            panic!("oh no");
        } else {
            // ...
        }
    }
}

impl<'a> Drop for B<'a> {
    fn drop(&mut self) {
        let a = &self.0;
        println!("{}", a.0);
    }
}

fn foo(b: &mut B) {
    *b.0 = A(Box::new(1));
}

fn main() {
    let mut bomb = A(Box::new(0));
    let mut b = B(&mut bomb);
    foo(&mut b);
}

Here we create a bomb that's "ready to go off" via A(Box::new(0)), and then it's packaged up into another value with a destructor that can later witness its value (e.g. B(&mut bomb)). We then pass it off to a function where b's value is overwritten with a non-bomb (e.g. A(Box::new(1))). Due to the way this is codegen'd, however, the destructor for A is run before the pointer is overwritten, which means that the pointer is never actually overwritten, which in turn means that the destructor for B can later witness this, causing a use-after-free.

When running this program, I get:

thread '<main>' panicked at 'oh no', <anon>:7
-337192192

(of course results may vary).

tl;dr; I think this is a codegen bug, not a TLS bug. Recategorizing as T-compiler, but we should probably still fix thread::local in the meantime to protect against this.

[alexcrichton]

Ah, cc @rust-lang/compiler, tagging this with I-unsound due to my code example above.

[eddyb]

That sounds like there are two problems here:

• TLS not guarding certain kinds of assignment drops (initialization)
• panicking not guarding assignment drops

I agree that the more general problem is that assignment drops need special handling, but doing it, via swaps for example, in every case seems like overkill and can potentially ruin performance.

It might be worth splitting this into two issues (or 3, one of them tracking the various instances of the general case).

As for the panic case, it might be worth implementing *a = b; as:

try {
    drop_in_place(a);
} finally {
    ptr::write(a, b);
}