fix weak memory bug in TLS on Windows by RalfJung · Pull Request #124281 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation11 Commits1 Checks12 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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
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
Operating system: Windows
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.
labels
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.
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.
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 = Box::new(0); } X.with(|_x| {}) }
fn with_thread_local2() { thread_local! { static Y: Box = 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();}
Thank you very much, especially for the detailed comments!
@bors r+
📌 Commit d5d714b has been approved by joboet
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
bors added a commit to rust-lang-ci/rust that referenced this pull request
bors added a commit to rust-lang-ci/rust that referenced this pull request
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#124281 - RalfJung:win-tls, r=joboet
fix weak memory bug in TLS on Windows
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 rust-lang#102655?
Fixes rust-lang#123583
Labels
Operating system: Windows
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the library team, which will review and decide on the PR/issue.