rust-lang / rust-lang/rust

Inappropriate `panic!`s in `Thread::new` on certain unix platforms

Open
#160,793 7 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

A-global-allocator-reentry A-thread C-bug I-memleak I-unsound needs-triage O-unix P-high T-libs
Dominant language
Rust
Stars
119k
Forks
16.1k
PR merge metrics
PR metrics pending

Description

Found after discussing code surrounding #160219 on zulip.

Edit: It is inappropriate for Thread::new to ever panic, because we guarantee that thread::current, which uses it, does not call the global allocator reentrantly. The memory leak on unwind, which this issue was originally about, is not that important.


The implementation of Thread::new manually initializes a struct behind MaybeUninit, which means that if Parker::new_in_place unwinds, the name's memory (essentially Option<CString>), now held by the previously written name field behind MaybeUninit, is leaked.

https://github.com/rust-lang/rust/blob/4667d75565e47ba5df36c0df598c556b543e8624/library/std/src/thread/thread.rs#L101-L108

Note that this is possible, on certain unix platforms:

https://github.com/rust-lang/rust/blob/4667d75565e47ba5df36c0df598c556b543e8624/library/std/src/sys/sync/thread_parking/pthread.rs#L24-L32

which calls

https://github.com/rust-lang/rust/blob/4667d75565e47ba5df36c0df598c556b543e8624/library/std/src/sys/pal/unix/sync/condvar.rs#L152-L175

which uses assert_eq to ensure that the initialization of the Condvar succeeds (it can fail, e.g. from resource exhaustion). Note that the method itself expects failures are possible here. It uses a guard struct to ensure that the resources of the condattr are not leaked on unwind.


Furthermore, the implementation of std::thread::current seems to assume that Thread::new never unwinds, because it sets a flag to detect reentrance, which is never reset if an unwind occurs:

https://github.com/rust-lang/rust/blob/4667d75565e47ba5df36c0df598c556b543e8624/library/std/src/thread/current.rs#L289-L301


The first issue should be fixable by reordering the fields' initialization logic, and the second one by adding a drop guard that unsets the busy state.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Read library/std/src/thread/thread.rs around Thread::new, then trace Parker::new_in_place through library/std/src/sys/sync/thread_parking/pthread.rs and library/std/src/sys/pal/unix/sync/condvar.rs. Also inspect library/std/src/thread/current.rs around the reentrance flag; done means Thread::new does not panic in the described failure paths and current's state remains correct if unwinding occurs.

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
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.