`Condvar` and `Mutex` in `std::sys::pal::unix::sync` violate aliasing rules

[maxdexh]

View all comments

Initially found by @RalfJung while discussing code surrounding #160219 on zulip, and expanded on in this issue.

Note that the text of this issue is overly focused on Parker::new_in_place's usage of Condvar. It turns out (see comments) that due to the Drop impl of Condvar (and Mutex), it is impossible to use these APIs correctly in their current state.

---

To construct a pinned Parker in-place, Thread::new does so behind MaybeUninit by using Arc::get_mut_unchecked and deriving a pointer from the mutable reference:

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())
	};

This is needed to initialize a pinned Condvar on platforms where Parker uses one, since it cannot be moved after being initialized:

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();
	}

Then Condvar::init ends up calling phread_cond_init:

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);
	}
	}

As discussed on zulip, pthread_cond_init is allowed to store the pointer for later use (this is the reason they need to be pinned). For example, on IBM AIX (which this code is run on), condvars are documented as being held in an intrusive linked list.

Now, this means that the condvar's stored pointer (which was derived from a mutable reference) is invalidated when the Arc inside Thread is borrowed from the next time, e.g. through Thread::id. Any access to it from libc later on will violate the aliasing rules.

@rustbot label T-opsem T-libs I-unsound A-thread -T-bootstrap A-pin

[ais523]

I was looking into this to try to figure out a sound way to initialize it, but it turns out that the problem will be present regardless of the way in which the contents of the Arc are initialized.

The Arc contains an Inner by value, which in turn contains a Parker by value. This means that any attempt to borrow the contents of the Arc will borrow the memory that contains the Parker. The operating system could choose that moment to update the intrinsic linked list, which would cause it to write to memory referenced by the shared reference that was returned by Arc::deref, violating the aliasing rules on Arc::deref's return value (because nothing can soundly write to memory within the range of a shared reference). This means that calling Arc::deref on the resulting Arc is unsound regardless of how the Arc is initialized.

I think the correct fix here involves storing the Parker in an UnsafeCell and accessing it only via raw pointers.

(Whether the currently used initialization pattern is correct once the Parker is placed into a cell may depend on the precise aliasing model: the basic pattern being used here is legal in Tree Borrows but not in Stacked Borrows. You can make it legal in either by deriving the pointer to the UnsafeCell from a shared rather than mutable reference to the UniqueArc, and never forming a mutable reference to the UniqueArc again, because shared references are not invalidated by additional shared references to the Arc.)

[maxdexh]

Yeah I pointed something like this out on zulip as well. Because Condvar::init takes a Pin<&mut T>, any call to init will probably have to require borrowing from some place. The next time that place is used, the pointer stored inside pthreads is going to get invalidated because it is derived from the &mut.

I'm not sure there is any way to use Condvar::init correctly (in SB).

Also, I think the &mut in Condvar::drop is going to invalidate the condvar too.

[maxdexh]

Oh wait I didn't see that the contents of Condvar are wrapped in UnsafeCell. Does the pointer from UnsafeCell::get get invalidated when you take a new mutable ref to the UnsafeCell?

[nia-e]

(cc'ing in, #160817 attempts to fix this)