fix(std): recover thread handle when current() runs before spawn init on Windows - #158579
fix(std): recover thread handle when current() runs before spawn init on Windows#158579anshjaiswal12 wants to merge 3 commits into
Conversation
On Windows, thread::current() during DLL_THREAD_ATTACH can initialize a temporary handle before thread::spawn sets the real one. Recover from that pre-init state instead of aborting.
Keep the spawn recovery logic without the env-var test shim in library/std.
rustbot
added
S-waiting-on-review
|
Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @clarfonthey (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
Why was this reviewer chosen?The reviewer was selected based on:
|
|
Okay, my bad. I'll keep working on it. Thanks for your feedback. I'll try fixing it again, and then I'll submit it. |
|
Whoops, sorry, I misunderstood the PR and commented too eagerly. I guess this might work? |
| // SAFETY: `current` points to a valid `Thread` created by `init_current`. | ||
| let old = unsafe { Thread::from_raw(current) }; | ||
| if old.name().is_some() { | ||
| return false; |
There was a problem hiding this comment.
This is risky, old will be dropped here, so the TLS pointer is dangling. It's made sound by aborting immediately afterwards, but I still think it's suboptimal.
| /// If `thread::current()` ran before [`super::lifecycle::ThreadInit::init`] on a | ||
| /// newly spawned thread (e.g. during `DLL_THREAD_ATTACH` on Windows), replace | ||
| /// the temporary handle created by [`init_current`] with the one from `thread::spawn`. | ||
| pub(super) fn recover_preinitialized_for_spawn(thread: Thread) -> bool { |
There was a problem hiding this comment.
I think it'd be better to make set_current replace the current thread unconditionally, as it's only use is in ThreadInit::init.
rustbot
removed
the
S-waiting-on-review
|
Reminder, once the PR becomes ready for a review, use |
rustbot
added
the
S-waiting-on-author
Address review feedback: fold recover_preinitialized_for_spawn into set_current, which unconditionally replaces any existing TLS thread handle (as in drop_current) before installing the spawn handle. Fixes rust-lang#156277
|
rustbot
added
S-waiting-on-review
|
r? joboet since you've been doing review |
|
|
| if current > DESTROYED { | ||
| // SAFETY: `current` points to a valid `Thread` created by `init_current`. | ||
| unsafe { | ||
| CURRENT.set(DESTROYED); |
There was a problem hiding this comment.
I'd update the pointer to the new handle immediately.
| "set_current() was called while init_current() is in progress, \ | ||
| which indicates a bug in the Rust threading implementation" | ||
| ); | ||
| } |
There was a problem hiding this comment.
guard::enable only needs to be called if the above two conditions are false.
| } | |
| } else { | |
| guard::enable(); | |
| CURRENT.set(thread.into_raw().cast_mut()); | |
| } |
| /// `thread::spawn`. | ||
| pub(super) fn set_current(thread: Thread) { | ||
| let current = CURRENT.get(); | ||
| if current > DESTROYED { |
There was a problem hiding this comment.
I think we should ensure that there are no outstanding references to the old handle – otherwise you might allow some very confusing behaviour.
rustbot
added
S-waiting-on-author
4 participants
Fixes #156277
On Windows,
thread::spawncan abort ifcurrent()is called from aDLL_THREAD_ATTACHhandler before the new thread's TLS is initialized.Recover the pre-initialized handle instead of aborting.