Tracking Issue for rwlock_try_upgrade

[ShrigmaDev]

Feature gate: #![feature(rwlock_try_upgrade)]

This is a tracking issue RwLock::try_upgrade, which turns a read guard into a write guard if it is the only lock accessor.

Public API

// src/sync/rwlock.rs
impl<'a, T: ?Sized> RwLockReadGuard<'a, T> {
    pub fn try_upgrade(orig: Self) -> Result<RwLockWriteGuard<'a, T>, RwLockReadGuard<'a, T>>;
}

Steps / History

(Remember to update the S-tracking-* label when checking boxes.)

• ACP: Add Rwlock try_upgrade method libs-team#514
• Partial implementation Rwlock try upgrade #138560
• Clippy lint to avoid calling try_upgrade in a loop (not blocking)
• Miri tie-in (not blocking)
• Implementation: #...
• Final comment period (FCP)¹
• Stabilization PR

Unresolved Questions

• None yet.

Footnotes

1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html ↩

[tgross35]

The original author hasn't had a chance to work on this, so it is open if anybody wants to pick it up. Labeled hard because the queue implementation is pretty tricky.

Cc @connortsui20 in case you're interested in working on this since you handled downgrade.

[connortsui20]

The original author hasn't had a chance to work on this, so it is open if anybody wants to pick it up. Labeled hard because the queue implementation is pretty tricky.

Cc @connortsui20 in case you're interested in working on this since you handled downgrade.

I can pick this up (but only after next week because I need to graduate). I have a good idea of what needs to be done given the discussion in rust-lang/libs-team#514, but I will point out that I have no idea how to add rust-lang/libs-team#514 (comment) nor how to support a clippy lint that tells people not to put try_upgrade in a loop.

[tgross35]

but only after next week because I need to graduate

Certainly no rush, congrats! 🎉

Don't worry about Clippy, that wouldn't block anything here. Miri likely wouldn't either but it might not be that bad, @RalfJung may have some ideas about what to do here (rust-lang/libs-team#514 (comment) for context Ralf).

[connortsui20]

Also see: rust-lang/libs-team#514 (comment)

[connortsui20]

I got bored so I decided to look into this anyways... and now I see the problem that the previous attempt by @ShrigmaDev ran into.

TLDR; the layout of RwLockWriteGuard probably needs to change to support try_upgrade.

Basically, the issue is how to convert a RwLockReadGuard into a RwLockWriteGuard. The reason this is difficult (and it might actually be impossible to do this safely with the given types) is that the RwLockReadGuard doesn't really have a pointer back to the original RwLock (the smart pointer, not the internal one), and it instead just points to the same data that the original RwLock has.

pub struct RwLock<T: ?Sized> {
    inner: sys::RwLock,
    poison: poison::Flag,
    data: UnsafeCell<T>,
}

pub struct RwLockReadGuard<'a, T: ?Sized + 'a> {
    // NB: we use a pointer instead of `&'a T` to avoid `noalias` violations, because a
    // `RwLockReadGuard` argument doesn't hold immutability for its whole scope, only until it drops.
    // `NonNull` is also covariant over `T`, just like we would have with `&T`. `NonNull`
    // is preferable over `const* T` to allow for niche optimization.
    data: NonNull<T>,
    inner_lock: &'a sys::RwLock,
}

pub struct RwLockWriteGuard<'a, T: ?Sized + 'a> {
    lock: &'a RwLock<T>,
    poison: poison::Guard,
}

Observe that the write guard needs a reference back to the original RwLock, but RwLockReadGuard doesn't, which means we would somehow need to get that original RwLock back or reconstruct it. The previous attempt tries to reconstruct the original lock within try_upgrade, and I believe this won't work because that would be a dangling reference (I could be wrong though).

I'm tried to think of ways we could reconstruct this safely, and the only possible method of doing this would be to somehow derive the pointer for the original RwLock via the inner_lock: &'a sys::RwLock and then transmute the pointer. But this sets off so many alarm bells that I don't think this makes sense.

There is luckily another option, which is what I'll try to implement. We can make it so that the RwLockWriteGuard mimics the read guard (and also the mapped guards) and have it only store a NonNull<T> and an inner_lock: &'a sys::RwLock.

pub struct RwLockWriteGuard<'a, T: ?Sized + 'a> {
    data: NonNull<T>,
    inner_lock: &'a sys::RwLock,
    poison_flag: &'a poison::Flag,
    poison: poison::Guard,
    _variance: PhantomData<&'rwlock mut T>,
}

In this case, it is trivial to convert between the read and write guards (same fields). I still need to refresh on the semantics of poison though.

[zachs18]

I don't think RwLockReadGuard::try_upgrade is sound in combination with the (existing) fact that RwLockReadGuard<'a, T> is covariant in T (since 1.64.0), right? You can get a RwLockReadGuard<'a, &'short U> from a &'a RwLock<&'long U>, and then try_upgrade it to a RwLockWriteGuard<'a, &'short U> and then write a &'short U through it, then go back to the RwLock<&'long U> after 'short has expired and you have a use-after-free.

use std::sync::{RwLock, RwLockReadGuard, RwLockWriteGuard};

fn try_upgrade<'a, T: ?Sized>(orig: RwLockReadGuard<'a, T>) -> Result<RwLockWriteGuard<'a, T>, RwLockReadGuard<'a, T>> {
    todo!() // I don't think this function can soundly return a `RwLockWriteGuard` to the inner data with this signature
}

fn main() {
    let rwl: RwLock<&'static str> = RwLock::new("hello, world");
    let s: String = String::from("dynamically allocated");
    let rg = rwl.read().unwrap();
    let mut wg = try_upgrade(rg).unwrap();
    *wg = &s;
    dbg!(&wg); // "dynamically allocated"
    drop(wg);
    drop(s);
    let wg = rwl.write().unwrap();
    dbg!(&wg); // use-after-free
}

This code compiles on stable, so IIUC if this try_upgrade actually did return a write guard (instead of just panicking), this code would have a use-after-free bug.

If we had a separate UpgradableRwLockReadGuard<'a, T> that was invariant in T (so you could never get an UpgradableRwLockReadGuard<'a, &'short U> from a &'a RwLock<&'long U>), then I think try_upgrade on that type would be sound.

Note that all of parking_lot's RwLock guards are invariant in T, even the non-upgradable read guard, and must be because they have a fn RwLockReadGuard<'a, T>::rwlock(&self) -> &'a RwLock<T>.

[RalfJung]

I've skimmed the referenced context but it is not clear to me what the problem with Miri is. The macos rwlock has a subtle provenance bug, is that the issue? Those tests should only be disabled for Miri on macos.

[connortsui20]

I've skimmed the referenced context but it is not clear to me what the problem with Miri is. The macos rwlock has a subtle provenance bug, is that the issue? Those tests should only be disabled for Miri on macos.

I think that was a concern from before, it seems like there is a bigger problem now where it might be impossible to implement this method soundly

[RalfJung]

If Miri complains about an actual soundness problem, then the issue is not with Miri. ;)

I'm afraid I don't currently have the time to dig into the soundness issue and propose a solution.