Unsafe code review: semi-owning Weak<RwLock<T>> guard

[kpreid]

Background:

• My goal is to start with a std::sync::Weak<RwLock<T>> and get an equivalent of RwLock{Read,Write}Guard<T>, without creating any intermediate objects that must be borrowed by the calling code.
• I use ouroboros. It had a soundness issue #88 which is now fixed, but the fix removed support for type parameters.
• My particular use case is not really about borrowing from data owned by the structure itself, but rather a std::sync::Weak that we need to keep alive.
• I tried using yoke instead, but that doesn't work because it insists that mutable access should be through a 'static function, which is highly constraining.

It occurred to me that because my application only really needs to "strong-ify" the Weak — not eliminate all borrowing — it would be possible to write more specific unsafe code that has a simpler argument for soundness, and that's what I'm asking about here.

In particular, instead of synthesizing a lifetime from nowhere, and instead of using 'static anywhere, we can use the actual pointer that Weak makes available — and separately ensure that the pointer stays valid, as described in Weak::as_ptr()'s documentation (and example!). Therefore, the unsafely-produced reference lifetime matches the pointer provenance, and we separately make sure that the reference's referent stays valid (not dropped) as long as it is used.

Then, during drop glue execution, guard (and the reference) is dropped before strong, so the reference (in the guard) will always have been dropped before the referent (T) is possibly dropped. It's still technically executing drop glue for a now-invalid type, but it can never call any function with an invalid lifetime parameter.

So, is this sound? Miri accepts the basic test I wrote, but I'm not sure which directions to push it harder.

#![allow(clippy::result_unit_err)]

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

pub fn read_from_weak<'a, T>(weak: &'a Weak<RwLock<T>>) -> Result<ArcReadGuard<'a, T>, ()> {
    let strong = weak.upgrade().ok_or(())?;

    // Per `as_ptr()` documentation, since the `upgrade()` succeeded, `ptr` is valid.
    let ptr: *const RwLock<T> = weak.as_ptr();

    // SAFETY:
    // * `ptr` is valid now as per above
    // * `ptr` will remain valid until we drop `strong`
    // * we will not drop `strong` until after we drop `reference`
    // * we will not let `reference` escape to be copied and potentially outlive `strong`
    let reference: &'a RwLock<T> = unsafe { &*ptr };

    let guard = reference.read().map_err(|_| ())?;

    Ok(ArcReadGuard { guard, strong })
}

pub struct ArcReadGuard<'a, T> {
    // SAFETY: `guard` must be dropped before `strong`.
    guard: RwLockReadGuard<'a, T>,
    #[allow(dead_code)] // used for drop effect
    strong: Arc<RwLock<T>>,
}

impl<T> std::ops::Deref for ArcReadGuard<'_, T> {
    type Target = T;

    fn deref(&self) -> &Self::Target {
        &self.guard
    }
}

pub fn write_from_weak<'a, T>(weak: &'a Weak<RwLock<T>>) -> Result<ArcWriteGuard<'a, T>, ()> {
    let strong = weak.upgrade().ok_or(())?;

    // Per `as_ptr()` documentation, since the `upgrade()` succeeded, `ptr` is valid.
    let ptr: *const RwLock<T> = weak.as_ptr();

    // SAFETY:
    // * `ptr` is valid now as per above
    // * `ptr` will remain valid until we drop `strong`
    // * we will not drop `strong` until after we drop `reference`
    // * we will not let `reference` escape to be copied and potentially outlive `strong`
    let reference: &'a RwLock<T> = unsafe { &*ptr };

    let guard = reference.write().map_err(|_| ())?;

    Ok(ArcWriteGuard { guard, strong })
}

pub struct ArcWriteGuard<'a, T> {
    // SAFETY: `guard` must be dropped before `strong`.
    guard: RwLockWriteGuard<'a, T>,
    #[allow(dead_code)] // used for drop effect
    strong: Arc<RwLock<T>>,
}

impl<T> std::ops::Deref for ArcWriteGuard<'_, T> {
    type Target = T;

    fn deref(&self) -> &Self::Target {
        &self.guard
    }
}

impl<T> std::ops::DerefMut for ArcWriteGuard<'_, T> {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.guard
    }
}

#[test]
fn test1() {
    let thing = Arc::new(RwLock::new(String::from("hello")));
    let weak = Arc::downgrade(&thing);
    write_from_weak(&weak).unwrap().push_str(" world");

    println!("{}", &*read_from_weak(&weak).unwrap());

    let mut guard = write_from_weak(&weak).unwrap();
    drop(thing);
    guard.push('!');
    drop(guard);
}

[alice]

Your guard is not borrowed, so it is semantically incorrect for it to have a lifetime annotation (though not unsound). I would define the struct like this:

pub struct ArcReadGuard<T> {
    // SAFETY: `guard` must be dropped before `strong`.
    guard: RwLockReadGuard<'static, T>,
    #[allow(dead_code)] // used for drop effect
    strong: Arc<RwLock<T>>,
}

Other than that, it looks fine to me. The Arc ensures that the reference in the inner guard stays valid, and you make sure that they are dropped in the right order.

There is some similar code in the implementation of Tokio's WaitForCancellationFutureOwned.

[kpreid]

@alice

    // SAFETY: `guard` must be dropped before `strong`.
    guard: RwLockReadGuard<'static, T>,

Creating a false 'static reference is exactly what I was trying to avoid. Perhaps it's completely irrelevant, but I wanted to stay as far away as possible from the troubles that self-reference crates have had.

[2e71828]

Does lock_api's arc_lock feature do what you want? (Apologies for linking to the source, but this feature isn't included in docs.rs)

[kpreid]

Technically it would work now, but I want to preserve the option of having some data that is inside the Arc but outside the RwLock, which is not possible if precisely Arc<RwLock< is baked into the other library's code.

[alice]

Okay, so I read the ouroboros soundness issue. I hadn't thought about how stack protectors apply to this kind of thing. I guess that our ArcReadGuard is also unsound by that argument, if the ArcReadGuard is used as the argument to a function and the Arc gets destroyed during that function.

To be clear 'static vs 'a makes no difference here. Both are equal in this matter.

Anyway, wrapping the guard in MaybeUninit should fix the problem.

pub struct ArcReadGuard<T> {
    // SAFETY: `guard` must be dropped before `strong`.
    guard: MaybeDangling<RwLockReadGuard<'static, T>>,
    #[allow(dead_code)] // used for drop effect
    strong: Arc<RwLock<T>>,
}

struct MaybeDangling<T> {
    inner: MaybeUninit<T>,
}

impl<T> Drop for MaybeDangling<T> {
    fn drop(&mut self) {
        drop_in_place(self.inner.as_mut_ptr());
    }
}

[alice]

Technically, our example isn't actually unsound. The reference in an RwLockReadGuard points at data stored in UnsafeCell, so the &UnsafeCell<T> exception applies. (Though this relies on implementation details.)

[kpreid]

Perhaps I should also ask the meta question: my choice is between

• using the above code, thus introducing new unsafe in my own crate
• using self_cell which is relatively more complex, and I don't fully comprehend why it should be sound where ouroboros isn't

I'm generally an advocate for “don't write new unsafe code if a library can solve it”, but self-reference libraries are infamously troublesome. The upside of not using a library is that the code can be less generic and thus have fewer properties it needs to enforce on the generic borrower. But if I use a library, I benefit from the community's attention on it.

Given @alice's point that I also need a MaybeUninit, I'm leaning towards the try-a-library-again option because my simple thing isn't quite as simple as I thought.

Thoughts?

[SkiFire13]

@kpreid

using self_cell which is relatively more complex, and I don't fully comprehend why it should be sound where ouroboros isn't

From my understanding it heap allocates everything and type erases it to a NonNull<u8> and this avoids automatically exposing any dangling reference.

[kpreid]

On closer examination, self_cell doesn't support type parameters either. I guess it's unsafe code and // TODO: Replace this with a library when there is a good one. for me now.

[alice]

@kpreid

Given @alice's point that I also need a MaybeUninit, I'm leaning towards the try-a-library-again option because my simple thing isn't quite as simple as I thought.

For what it's worth, I'm quite certain that the version with MaybeUninit is sound.

[kpreid]

@alice

I'm quite certain that the version with MaybeUninit is sound.

So should I understand that the RFC 3336 proposed MaybeDangling is not itself additional necessary magic, and yours

@alice

struct MaybeDangling<T> {
    inner: MaybeUninit<T>,
}

is merely a manual, possibly too strong (i.e. claiming to the compiler possible non-initialization and not just possible invalidity) version that will do just fine until there is an official one? Or is there possibly something to worry about here?

Oh, I see, @Yandros wrote a comment on the ouroboros issue saying precisely that.

---

One additional change I'm making: My original code took an &Weak, but given there's no lifetime connection to the output any more, I assume it is sound to take an Arc and use Arc::as_ptr() instead. I don't entirely like it, but since I've given up on the lifetime being meaningful, it shouldn't do any harm. (The Weak is merely the original motivation for this whole thing — if I had started with an Arc then the caller could just borrow it and there wouldn't be any of this self-reference business.)

[kpreid]

I've now finished and merged the code, and added testing under Miri in CI since this now means my project has nontrivial unsafe code.

use std::sync::{Arc, RwLock, RwLockReadGuard, RwLockWriteGuard};
use std::{mem, ptr};

use super::uref::UEntry;

#[cfg(doc)]
use super::URef;

type Lock<T> = RwLock<UEntry<T>>;
type Strong<T> = Arc<Lock<T>>;

// There are two near-identical implementations for read-only and exclusive access.
// For readability, they are presented in parallel; both structs then both impls, etc.

/// Owning wrapper around [`RwLockReadGuard`] for [`URef`].
pub(super) struct UBorrowImpl<T: 'static> {
    // SAFETY: `guard` must be dropped before `strong`,
    // which is accomplished by declaring it first.
    /// Lock guard that points into `strong`.
    guard: MaybeDangling<RwLockReadGuard<'static, UEntry<T>>>,

    /// [`Arc`] that keeps the data pointer alive as long as `guard` is in use.
    /// This field is never read, only dropped to decrement the reference count appropriately.
    #[allow(dead_code)] // used for drop effect
    strong: Strong<T>,
}

/// Owning wrapper around [`RwLockWriteGuard`] for [`URef`].
pub(super) struct UBorrowMutImpl<T: 'static> {
    // SAFETY: `guard` must be dropped before `strong`,
    // which is accomplished by declaring it first.
    /// Lock guard that points into `strong`.
    guard: MaybeDangling<RwLockWriteGuard<'static, UEntry<T>>>,

    /// [`Arc`] that keeps the data pointer alive as long as `guard` is in use.
    /// This field is never read, only dropped to decrement the reference count appropriately.
    #[allow(dead_code)] // used for drop effect
    strong: Strong<T>,
}

impl<T: 'static> UBorrowImpl<T> {
    pub(super) fn new(strong: Strong<T>) -> Result<Self, LockError> {
        let ptr: *const Lock<T> = Arc::as_ptr(&strong);

        // SAFETY:
        // * `ptr` will remain valid until we drop `strong`
        // * we will not drop `strong` until after we drop `reference`
        // * we will not let `reference` escape to be copied and potentially outlive `strong`
        // * we will store the owner of the `reference` in `MaybeDangling` (see below)
        //   to avoid asserting its validity too long.
        let reference: &'static Lock<T> = unsafe { &*ptr };

        let guard = MaybeDangling::new(reference.try_read()?);

        Ok(UBorrowImpl { guard, strong })
    }
}

impl<T: 'static> UBorrowMutImpl<T> {
    pub(super) fn new(strong: Strong<T>) -> Result<Self, LockError> {
        let ptr: *const Lock<T> = Arc::as_ptr(&strong);

        // SAFETY:
        // * `ptr` will remain valid until we drop `strong`
        // * we will not drop `strong` until after we drop `reference`
        // * we will not let `reference` escape to be copied and potentially outlive `strong`
        // * we will store the owner of the `reference` in `MaybeDangling` (see below)
        //   to avoid asserting its validity too long.
        let reference: &'static Lock<T> = unsafe { &*ptr };

        let guard = MaybeDangling::new(reference.try_write()?);

        Ok(UBorrowMutImpl { guard, strong })
    }
}

impl<T: 'static> std::ops::Deref for UBorrowImpl<T> {
    type Target = UEntry<T>;

    fn deref(&self) -> &Self::Target {
        &self.guard
    }
}
impl<T: 'static> std::ops::Deref for UBorrowMutImpl<T> {
    type Target = UEntry<T>;

    fn deref(&self) -> &Self::Target {
        &self.guard
    }
}
impl<T: 'static> std::ops::DerefMut for UBorrowMutImpl<T> {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.guard
    }
}

#[derive(Copy, Clone)]
pub(super) struct LockError;

impl<T> From<std::sync::TryLockError<T>> for LockError {
    fn from(_: std::sync::TryLockError<T>) -> Self {
        // TODO: Distinguish PoisonErrors (without granting access) for debugging purposes
        Self
    }
}

/// Wrapper type that avoids implicitly asserting the contents are currently valid.
/// Therefore it is safe to contain a dangling reference, as long as it is not used.
///
/// Context on this code:
/// <https://users.rust-lang.org/t/unsafe-code-review-semi-owning-weak-rwlock-t-guard/95706/6>
///
/// TODO: Replace this with an official definitely-valid version when that is implemented.
/// <https://github.com/rust-lang/rfcs/pull/3336>
struct MaybeDangling<T> {
    inner: mem::MaybeUninit<T>,
}

impl<T> MaybeDangling<T> {
    fn new(value: T) -> Self {
        Self {
            inner: mem::MaybeUninit::new(value),
        }
    }
}

impl<T> std::ops::Deref for MaybeDangling<T> {
    type Target = T;

    fn deref(&self) -> &Self::Target {
        // SAFETY:
        // The `MaybeUninit` is never actually uninitialized. It might be dangling, but
        // in that case it is the caller's responsibility not to use `self`. We are only
        // using `MaybeUninit` to tell the _compiler_ to assume less.
        unsafe { self.inner.assume_init_ref() }
    }
}

impl<T> std::ops::DerefMut for MaybeDangling<T> {
    fn deref_mut(&mut self) -> &mut Self::Target {
        // SAFETY:
        // The `MaybeUninit` is never actually uninitialized. It might be dangling, but
        // in that case it is the caller's responsibility not to use `self`. We are only
        // using `MaybeUninit` to tell the _compiler_ to assume less.
        unsafe { self.inner.assume_init_mut() }
    }
}

impl<T> Drop for MaybeDangling<T> {
    fn drop(&mut self) {
        let p = self.inner.as_mut_ptr();
        // SAFETY:
        // The `MaybeUninit` is never actually uninitialized, and it is the caller's
        // responsibility to drop it before it is invalid just like any normal type.
        unsafe {
            ptr::drop_in_place(p);
        }
    }
}

(UEntry<T> is just a struct containing, currently, Option<T>; it will always be 'static.)

I'd appreciate hearing of any further considerations or newly introduced bugs — but in any case, thanks to everyone who commented so far.

[alice]

My version of MaybeDangling disables niche optimization, which the RRC would let you avoid.

[Yandros]

ManuallyDrop vs. MaybeDangling

I just wanted to clarify that ManuallyDrop should have MaybeDangling semantics, since the ManuallyDrop::{drop,take} APIs are very much designed to "consume" the inner value, wherein any references inside it may be dangling after that, whilst still being a part of the ManuallyDrop.

• More info over Zulip, including examples illustrating the issue

So the fact it currently does not do so is a bug in the language spec semantics. I said so in the issue, but in a rather ill-phrased manner, which seemed to blame miri, the UB-detector tool, for reporting the UB. RalfJ then pointed out that this was not a bug of miri, since it follows the current language spec semantics –even if they're "wrong", may I add–, hence the RFC to offer MaybeDangling (and fix ManuallyDrop).

So until then, we are forced to use MaybeUninit instead, which does require some extra unsafe and whatnot because now we need to pinky-promise our value is not uninit. This is a bit finicky, so it's a good place for a dedicated abstraction. Speaking of which:

Centralizing the MaybeDangling abstraction.

The "lack of MaybeDangling" situation is extremely similar, ecosystem-wise, to the "lack of offset_of! macro" we used to have:

1. theoretically, the code we have right now is "susceptible to UB in the future", albeit not exploited by current compilers yet;

2. a proper implementation is underway, but not yet available (so there is no perfect fit yet), but at least it's guaranteed to kick in before the compiler exploits that "future-wise UB";

3. we have to make do with some workaround, but would like to make the swap to the proper solution the moment it becomes available.

These are the perfect ingredients then for one thing: a dedicated helper crate that centralizes the workaround. In the case of offset_of!, this has been ::memoffset; and now I am thinking we need one such for MaybeDangling:

• by dedicating a crate to it, the issue is acknowledged:

  • for those unaware of all these "protector shenanigans", much like in the past, many people were unaware of &(*dangling_pointer).field being UB, they will have a place for documentation;

  • the RFC/implementation of the proper solution will have something to refer to, and will get extra motivation to get it rolling;

• for libraries that were hand-rolling their workaround until now:

  • they won't have to be worried about doing it wrong (although in this case using MaybeUninit is rather simple-ish…);

  • the moment the language offers nicer solutions (e.g., fixing the fact ManuallyDrop doesn't have MaybeDangling semantics yet; or actually featuring MaybeDangling), this centralized crate will be the only place needing to be updated to take advantage of it, in a semver-compatible manner (by design of the crate).

    This thus lets downstream users take advantage of the fix without "you", the middle-layer library author, having to lift a finger.

Which is why I'll very soon be releasing a ::maybe-dangling crate for that purpose, which will be featuring:

• a fixed ManuallyDrop (that is, a MaybeUninit that can assume_init at any point; at least until the language/::core::mem::ManuallyDrop is fixed and then it will just reëxport it);

• a tentative MaybeDangling (i.e., ManuallyDrop but with Drop back). Alas, in stable Rust, this will require forgoing the <#[may_dangle] T> Drop semantics (the irony is rich).

  • I do have another crate in the works that should offer a stable-Rust polyfill of <#[may_dangle] 'lt> Drop, which will be too cumbersome to be useful i general, but which ought to be a perfect fix for patterns such as ouroboros (wherein the self-referential lifetime needs to have #[may_dangle] semantics). So that may be offered as well, eventually, alongside ManuallyDrop and MaybeDangling

In order to become a trustworthy crate in the ecosystem, it needs to be well-maintained, at least until we get proper MaybeDangling and whatnot in stable Rust. And to be realistic, that can rarely be achieved by a single maintainer (bus factor, etc.), so I'd like to have more owners joining in on this effort: anyone interested?

[alice]

I opened tokio#5808 to add MaybeUninit to a custom self-referential struct in tokio-util.

@Yandros

In order to become a trustworthy crate in the ecosystem, it needs to be well-maintained, at least until we get proper MaybeDangling and whatnot in stable Rust. And to be realistic, that can rarely be achieved by a single maintainer (bus factor, etc.), so I'd like to have more owners joining in on this effort: anyone interested?

You're welcome to send me an invite.

[quinedot]

@Yandros

So the fact it currently does not do so is a bug in the language spec. I said so in the issue, but in a rather ill-phrased manner, which seemed to blame miri, the UB-detector tool, for reporting the UB. RalfJ then pointed out that this was not a bug of miri, since it follows the current language spec –even if it's "wrong", may I add–, hence the RFC to offer MaybeDangling (and fix ManuallyDrop).

When you talk of an existing language spec, what are you referring to?

[Yandros]

Heh, you caught me red-handed I should have mentioned language semantics instead; with those having probably figured out off compiler code? Feel free to go to that Zulip thread and ask RalfJ about it