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

[maxdexh]

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.

rust/library/std/src/thread/thread.rs

Lines 101 to 108 in 4667d75

	let inner = unsafe {
	let mut arc = Arc::<Inner, _>::new_uninit_in(System);
	let ptr = Arc::get_mut_unchecked(&mut arc).as_mut_ptr();
	(&raw mut (*ptr).name).write(name);
	(&raw mut (*ptr).id).write(id);
	Parker::new_in_place(&raw mut (*ptr).parker);
	Pin::new_unchecked(arc.assume_init())
	};

Note that this is possible, on certain unix platforms:

rust/library/std/src/sys/sync/thread_parking/pthread.rs

Lines 24 to 32 in 4667d75

	pub unsafe fn new_in_place(parker: *mut Parker) {
	parker.write(Parker {
	state: AtomicUsize::new(EMPTY),
	lock: Mutex::new(),
	cvar: Condvar::new(),
	});
	Pin::new_unchecked(&mut (*parker).cvar).init();
	}

which calls

rust/library/std/src/sys/pal/unix/sync/condvar.rs

Lines 152 to 175 in 4667d75

	pub unsafe fn init(self: Pin<&mut Self>) {
	use crate::mem::MaybeUninit;
	struct AttrGuard<'a>(pub &'a mut MaybeUninit<libc::pthread_condattr_t>);
	impl Drop for AttrGuard<'_> {
	fn drop(&mut self) {
	unsafe {
	let result = libc::pthread_condattr_destroy(self.0.as_mut_ptr());
	assert_eq!(result, 0);
	}
	}
	}
	unsafe {
	let mut attr = MaybeUninit::<libc::pthread_condattr_t>::uninit();
	let r = libc::pthread_condattr_init(attr.as_mut_ptr());
	assert_eq!(r, 0);
	let attr = AttrGuard(&mut attr);
	let r = libc::pthread_condattr_setclock(attr.0.as_mut_ptr(), Self::CLOCK);
	assert_eq!(r, 0);
	let r = libc::pthread_cond_init(self.raw(), attr.0.as_ptr());
	assert_eq!(r, 0);
	}
	}

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:

rust/library/std/src/thread/current.rs

Lines 289 to 301 in 4667d75

	fn init_current(current: *mut ()) -> Thread {
	if current == NONE {
	CURRENT.set(BUSY);
	// If the thread ID was initialized already, use it.
	let id = id::get_or_init();
	let thread = Thread::new(id, None);
	// Make sure that `crate::rt::thread_cleanup` will be run, which will
	// call `drop_current`.
	crate::sys::thread_local::guard::enable();
	CURRENT.set(thread.clone().into_raw().cast_mut());
	thread
	} else if current == BUSY {

---

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.

[maxdexh]

@rustbot label I-memleak A-thread T-libs I-unsound A-global-allocator-reentry

[rustbot]

Error: Unknown labels: A-threads

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip.

[maxdexh]

@RalfJung Sorry for the ping, but GlobalAlloc actually guarantees that std::thread::current does not call the global allocator. This is apparently not the case, because if the assertion fails, we call panic!. So the global allocator and the panic hook are invoked. If the global allocator relies on not being called reentrantly by thread::current, I think this is unsound?

[RalfJung]

Yeah that sounds like we are not holding the promise we are making. That guarantee was added in #144465. It's largely a libs-api question, so I don't really have any further insights here.
Cc @orlp @Amanieu @joboet

[Mark-Simulacrum]

Cc #160776 which this seems like it overlaps with (though finds different issues)