Enforce no-move rule of ReentrantMutex using Pin and fix UB in stdio

[m-ou-se]

A sys_common::ReentrantMutex may not be moved after initializing it with .init(). This was not enforced, but only stated as a requirement in the comments on the unsafe functions. This change enforces this no-moving rule using Pin, by changing &self to a Pin in the init() and lock() functions.

This uncovered a bug I introduced in #77154: stdio.rs (the only user of ReentrantMutex) called init() on its ReentrantMutexes while constructing them in the intializer of SyncOnceCell::get_or_init, which would move them afterwards. Interestingly, the ReentrantMutex unit tests already had the same bug, so this invalid usage has been tested on all (CI-tested) platforms for a long time. Apparently this doesn't break badly on any of the major platforms, but it does break the rules.*

To be able to keep using SyncOnceCell, this adds a SyncOnceCell::get_or_init_pin function, which makes it possible to work with pinned values inside a (pinned) SyncOnceCell. Whether this function should be public or not and what its exact behaviour and interface should be if it would be public is something I'd like to leave for a separate issue or PR. In this PR, this function is internal-only and marked with pub(crate).

* Note: That bug is now included in 1.48, while this patch can only make it to 1.49 1.50. We should consider the implications of 1.48 shipping with a wrong usage of pthread_mutex_t / CRITICAL_SECTION / .. which technically invokes UB according to their specification. The risk is very low, considering the objects are not 'used' (locked) before the move, and the ReentrantMutex unit tests have verified this works fine in practice.

Edit: This has been backported and included in 1.48. And soon 1.49 too.

---

In future changes, I want to push this usage of Pin further inside sys instead of only sys_common, and apply it to all 'unmovable' objects there (Mutex, Condvar, RwLock). Also, while sys_common's mutexes and condvars are already taken care of by #77147 and #77648, its RwLock should still be made movable or get pinned.

[rust-highfive]

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

[withoutboats]

As I wrote in zulip, I'd rather use &'static ReentrantMutex to prove that it won't be moved again, instead of having to maintain the additional Pin APIs around ReentrantMutex and SyncOnceCell, which run the risk of becoming unsound by the addition of other APIs. Since this is all internal for use by stdio, and stdio has &'static ReentrantMutex already, the additional flexibility of pin (which guarantees something is in place even if it doesn't live for all of 'static) isn't needed.

I'd change the SyncOnceCell API to something like this:

fn get_or_init_in_place(&'static self, f: impl FnOnce() -> T, g: impl FnOnce(&'static T)) -> &'static T

And the only change I would make to ReentrantMutex is to change init to fn init(&'static self).

Do you think there's an argument for having the extra flexibility of Pin?

[Mark-Simulacrum]

I think we should backport this to beta as well (1.48), patch here is not too large and seems like it's definitely in the hot path so should get plenty of testing. beta-nominated.

[withoutboats]

fn get_or_init_in_place(&'static self, f: impl FnOnce() -> T, g: impl FnOnce(&'static T)) -> &'static T

Another option for this API, which seems more easy to make public, might be to use MaybeUninit:

fn get_or_init_in_place<'a>(&'a self, f: impl for<'b> FnOnce(&'b mut MaybeUninit<T>)) -> &'a T

[m-ou-se]

Another option for this API, which seems more easy to make public, might be to use MaybeUninit:

That's exactly what I had in my first version (before pushing) ^^. I can probably find it somewhere in my git reflog if you really want that one. Though, I found it a bit tricky to properly explain the safety requirements that version, so I changed it to the one with Pin instead. This Pin version is way less dangerous, as it can't be misused from the outside without unsafe: This get_or_init_pin is a safe function.

Do you think there's an argument for having the extra flexibility of Pin?

So indeed, this ReentrantMutex is only used right now with a static lifetime (since very recently: #77154), so we could turn this into a StaticReentrantMutex and do the same as for sys_common::StaticMutex: Use the 'static lifetime for the 'don't move me'-guarantee: #77648

However, it does restrict any future usage of this ReentrantMutex to static ones. I like the Pin version better, because then you can also Box::pin it, which is another proper way to make the 'no move' guarantee.

And regardless of ReentrantMutex, I'd also really like to apply the Pin logic to all the sys::Mutex, sys::RWLock, sys::ReentrantMutex and sys::Condvar implementations that need it. Those are extremely unsafe to use now, and Pin could guarantee one of the trickiest safety guarantees to get wrong for them. (Even the unit tests had it wrong.) So even if committing to a 'static-only sys_common::ReentrantMutex, it'd still be good to think about using Pin for these kind of unmovable objects.

Note that a sys_common::ReentrantMutex cannot be created with a const fn, as it requires .init() before any use. So by also requiring 'static, the ways it can be made and used is very limited. It can't be used as a static because it has no const constructor, but it also can't be used in many other ways because of the static lifetime. It'd restrict it basically to a static SyncOnceCell (or a leaked box).

[m-ou-se]

Oh and also: In ReentrantMutex::init I now take a mutable reference (Pin<&mut Self>), to make sure it hasn't been shared yet. It wouldn't be possible to take a &'static mut there, because then it's forever mutably (re)borrowed.

[withoutboats]

This Pin version is way less dangerous, as it can't be misused from the outside without unsafe: This get_or_init_pin is a safe function.

But the only way you use this function still requires unsafe because pin does not capture all of the requirements to initialize this mutex. If I were designing a OnceCell to initialize in place, this is exactly how I'd design it, not with pin. I can't imagine every making get_or_init_pin public.

And regardless of ReentrantMutex, I'd also really like to apply the Pin logic to all the sys::Mutex, sys::RWLock, sys::ReentrantMutex and sys::Condvar implementations that need it.

My own experience trying to use pin this way (for example in the intrusive doubly linked list I used in shifgrethor) has been that it doesn't really buy a lot because there's not a lot of abstraction going on & I end up needing unsafe access into the pin every time I look at it, so it feels like more bookkeeping that doesn't really help maintain correctness. Maybe this will be a different experience, but its why I'm generally suspicious of applying pin to anything that involves immovable values without really clear benefits.

Pin is mainly about limiting the power of &mut, so if you weren't passing &muts around anyway, it doesn't really have a lot of benefits.

[m-ou-se]

still requires unsafe because pin does not capture all of the requirements to initialize this mutex

Sure, but if it captures most of them that's already a big win. Tracking multiple unsafe requirements manually is hard, as evidenced by the bug this PR is fixing.

so it feels like more bookkeeping that doesn't really help maintain correctness

Every single Rust program that uses print!() in Rust 1.48 now technically contains UB. That would've been avoided if ReentrantMutex had used Pin. So I would say this is a good example where it helps with correctness.

Pin is mainly about limiting the power of &mut, so if you weren't passing &muts around anyway, it doesn't really have a lot of benefits.

Here's another example that shows Pin<&T> is also very useful: #77865

Since this is beta-nominated, we shouldn't wait too long with this PR. If you're still opposed to using Pin here, I can try to find an alternative, but every alternative I came up with results in more unsafe requirements to keep track of or less flexibility.

[Mark-Simulacrum]

@m-ou-se This has not yet merged into nightly, and 1.48 releases in less than a week. Do you think there's something more targeted perhaps that we could backport? Otherwise I can try to review it in detail and approve it for beta -- I think @withoutboats' concerns are more towards maintenance than correctness, right? It does seem a bit worrying to let this slip into 1.48 (and now likely 1.49 as well, since that has branched).

[m-ou-se]

Do you think there's something more targeted perhaps that we could backport?

A SyncOnceCell doesn't have any way to move something into it and allow modification there before the cell is considered initialized. So fixing this will definitely require adding a method to SyncOnceCell to allow that.

Avoiding SyncOnceCell entirely would basically be reverting #77154.

A slightly smaller change would be to add a method to SyncOnceCell that allows direct access to the MaybeUninit inside the cell to initialize it. That would not require Pin. But I'm afraid that'd just lead to another accident (now or in the future), as that would be a very unsafe interface.

It does seem a bit worrying to let this slip into 1.48.

Yes.

[Mark-Simulacrum]

Ok, having reviewed this PR I think it's reasonable enough to backport it to beta directly.

[camelid]

Hmm, was this backported but not merged to nightly?

[Mark-Simulacrum]

Going to go ahead and renominate for 1.49 backport (will do that this weekend). Would be great to make some progress here though.

[Mark-Simulacrum]

Okay, I've reviewed the PR implementation in some detail, and I'm going to go ahead and @bors r+ this.

I am not sure how I feel about @withoutboats concerns around Pin<&T> being more overhead rather than benefit. Generally speaking, I do think that there can be some added verbosity with Pin (including unsafe) that's needed, but this PR at least doesn't seem to add too much noise. I think landing this in the meantime fixes the potential UB on nightly (and lets us have that fix on all three channels with a backport, rather than backporting without landing). If we feel strongly about Pin<&T> being the wrong abstraction we can also revert this change down the line and replace it. It probably makes sense for libs team to discus the implications of Pin<&T> becoming more common; if we don't like it in general then we shouldn't use it and perhaps even actively lint against it :)

I am also a little worried by "... which run the risk of becoming unsound by the addition of other APIs" -- it seems like if true, we shouldn't be adding those APIs? But I admit I'm not quite following which APIs, and on what, you refer to :)

[bors]

📌 Commit 912c1f78c70284c3f559ec29ea622057745a5fe0 has been approved by Mark-Simulacrum

[Mark-Simulacrum]

@bors r-

Could you actually squash commits down here a bit too?

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit 67c18fd has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit 67c18fd with merge 8cef65f...

[bors]

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 8cef65f to master...