fix weak memory bug in TLS on Windows

[RalfJung]

We need to store the key after we register the dtor.

Now I hope there isn't also some other reason why we have to actually register the dtor last... @joboet is there a reason you picked this particular order in #102655?

Fixes #123583

[rustbot]

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[joboet]

Ah yes, this is clearly a bug.

Now I hope there isn't also some other reason why we have to actually register the dtor last... @joboet is there a reason you picked this particular order in #102655?

There was: the destructor may be called as soon as it is registered so I wanted to make sure that the key was set at that point, as TlsGetValue needs to read from a correct key.

You can fix this by skipping the TLS variable in run_dtors if the stored key is zero.

[RalfJung]

Ah, makes sense. And in the Miri testcase, we don't have a thread-exit racing with a key-init so we wouldn't hit this.

I have updated the code.

[RalfJung]

I extended the Miri test to also cover this, that triggered the bug immediately:

//! Regression test for <https://github.com/rust-lang/rust/issues/123583>.
use std::thread;

fn with_thread_local1() {
    thread_local! { static X: Box<u8> = Box::new(0); }
    X.with(|_x| {})
}


fn with_thread_local2() {
    thread_local! { static Y: Box<u8> = Box::new(0); }
    Y.with(|_y| {})
}

fn main() {
    // Here we have two threads racing on initializing the thread-local
    // and (on Windows without target_thread_local) adding it to the global dtor list.
    let t = thread::spawn(with_thread_local1);
    with_thread_local1();
    t.join().unwrap();

    // Here we have one thread running the destructors racing with another thread initializing a thread-local.
    // The second thread adds a destructor that could be picked up by the first thread.
    let t = thread::spawn(|| { /* immediately just run destructors */});
    with_thread_local2(); // initialize thread-local
    t.join().unwrap();
}

[joboet]

Thank you very much, especially for the detailed comments!
@bors r+

[bors]

📌 Commit d5d714b has been approved by joboet

It is now in the queue for this repository.

[RalfJung]

The regression tests are being added on the Miri side in rust-lang/miri#3501.