More global allocator reentrancy issues in `park` and `unpark`
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 119k
- Forks
- 16.1k
- PR merge metrics
- PR metrics pending
Description
GlobalAlloc guarantees that implementors can use thread::current, thread::park and thread::Thread::unpark in global allocators without being called reentrantly.
However, thread uses synchronization internally and we have some platforms where the std::sys implementations of that can fail. This causes them to call panic!, which is inappropriate for the thread APIs because panicking invokes the global allocator. See #160793 for how this is violated in thread::current.
The platform implementations of Parker live in library/std/src/sys/sync/thread_parking/ and are chosen using cfg_select.
The following implementations of Parker::park/Parker::unpark can call the global allocator by panicking:
library/std/src/sys/sync/thread_parking/xous.rs
Lots of places this can panic, including indexing.
No idea which of these represent OS errors, and which are checking for logic bugs.
https://github.com/rust-lang/rust/blob/4667d75565e47ba5df36c0df598c556b543e8624/library/std/src/sys/sync/thread_parking/xous.rs#L26-L45
https://github.com/rust-lang/rust/blob/4667d75565e47ba5df36c0df598c556b543e8624/library/std/src/sys/sync/thread_parking/xous.rs#L74-L103
library/std/src/sys/sync/thread_parking/pthread.rs
Only a few panics, but note that even the synchronization primitives, which are from library/std/src/sys/pal/unix/sync/, can panic here (e.g. Mutex::lock panics as a workaround for deadlock-detection on solaris).
Not sure if any of the panics here are checking for actual errors, or if it's all logic bugs.
https://github.com/rust-lang/rust/blob/4667d75565e47ba5df36c0df598c556b543e8624/library/std/src/sys/sync/thread_parking/pthread.rs#L46-L88
https://github.com/rust-lang/rust/blob/4667d75565e47ba5df36c0df598c556b543e8624/library/std/src/sys/sync/thread_parking/pthread.rs#L134-L164
library/std/src/sys/sync/thread_parking/id.rs
This one works using thread ids and more platform-dependent implementations in library/std/src/sys/pal/*/thread_parking.rs.
At least one of those has an unwrap, I did not check the rest:
https://github.com/rust-lang/rust/blob/4667d75565e47ba5df36c0df598c556b543e8624/library/std/src/sys/pal/sgx/thread_parking.rs#L11-L13
What's the error we're unwrapping? No idea!
I did not find panics in the remaining implementations, namely darwin, futex, windows7 and unsupported; with the caveat that futex is split into more platform-specific futex implementations. I'll get around to reviewing those (this issue took much longer to write than I expected).
As an aside, note that thread::park itself handles unwinds from Thread::park using an abort-on-drop guard. However, it should never even panic, since that already violates our guarantees to the global allocator.
Implementation of park
@rustbot label T-libs A-thread I-unsound
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by reading the Parker implementations in library/std/src/sys/sync/thread_parking/xous.rs, pthread.rs, and id.rs, then inspect the SGX implementation under library/std/src/sys/pal/ and the park wrapper in library/std/src/thread/functions.rs. Trace which panic or unwrap paths can invoke the global allocator. Done means the identified park and unpark paths no longer violate the GlobalAlloc reentrancy guarantee, with the affected platform implementations checked.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- operating-systems
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100