RwLock is UnwindSafe despite not poisoning on read(), can cause broken invariants

[lilyball]

RwLock only poisons on panic with write(), it explicitly does not poison on panic with read(). This makes sense for most types, but for types with interior mutability read() can expose that interior mutability and allow broken invariants to be witnessed without using AssertUnwindSafe or spawning threads.

If I understand this correctly, due to the lack of read poisoning, it really should only be UnwindSafe where T: UnwindSafe (and I believe RefUnwindSafe where T: RefUnwindSafe, though I'm not certain as I'm still struggling to properly conceptualize this.).

On a similar note, RwLockReadGuard is UnwindSafe and RefUnwindSafe without any conditions. So is RwLockWriteGuard, but that's the one that does poisoning so it's fine there. RwLockReadGuard should at least require T: RefUnwindSafe for it to be UnwindSafe and RefUnwindSafe. I believe that if RwLock is adjusted then RwLockReadGuard will pick it up automatically due to the auto trait rules, though in this case RwLockWriteGuard will need the manual implementations as it does poisoning.

---

I'm not sure if there's any quick go-to for testing unwind safety, but I wrote a simple type that maintains a logical invariant with interior mutability. Without RwLock I get the expected compiler error trying to pass a reference to it across a catch_unwind barrier. Wrapping it in RwLock gets rid of the compiler error without introducing poisoning, which then allows for observing a broken invariant. Playground link

use std::cell::RefCell;

struct Foo {
    // Invariant: inner is always Some, it's only teporarily None during transformation.
    inner: RefCell<Option<String>>,
}

impl Foo {
    fn new() -> Self {
        Foo {
            inner: RefCell::new(Some("initial".to_owned())),
        }
    }

    fn transform(&self, f: impl FnOnce(String) -> String) {
        let inner = self.inner.borrow_mut().take().unwrap();
        *self.inner.borrow_mut() = Some(f(inner));
    }

    fn inner(&self) -> std::cell::Ref<str> {
        std::cell::Ref::map(self.inner.borrow(), |inner| {
            inner.as_deref().expect("broken invariant")
        })
    }
}

fn main() {
    let foo = Foo::new();
    
    // Comment out the next two lines to get an unwind safety error
    let foo = std::sync::RwLock::new(foo);
    let foo = foo.read().unwrap();
    
    dbg!(foo.inner());
    let result = std::panic::catch_unwind(|| {
        foo.transform(|_| panic!());
    });
    let _ = dbg!(result);
    dbg!(foo.inner());
}

In this code I'm passing the RwLockReadGuard across the catch_unwind barrier, but passing the &RwLock across and calling .read().unwrap() on each access produces the same results (meaning this can't just be fixed by changing RwLockReadGuard).

Meta

Playground rust version: Stable channel, 1.55.0

Additional context

I discovered this when I was trying to figure out how to make a type with interior mutability safe to pass to something that requires RefUnwindSafe. I thought "it has interior mutability, maybe I should just use a RwLock and only take reads on it", at which point I discovered that this doesn't add poisoning despite being RefUnwindSafe. In retrospect, reading can't add poisoning (given that there can be concurrent reads, and you can't poison an extant guard) so I really do need a mutex, but the fact that RwLock makes the compiler happy here is a problem.

[RalfJung]

This makes sense. Fixing this is a breaking change however. Cc @rust-lang/libs-api

However, note that your Foo type is not Sync, so you could not actually use an RwLock<Foo> from multiple threads.

[lilyball]

@RalfJung Sure, and there's a whole discussion somewhere else about whether RefUnwindSafe should be implemented for all T: Sync, where the dissenting argument was that the two are orthogonal and should not be linked, even though it's hard to imagine how a type could be safe to share across multiple threads simultaneously and yet not safe to share across a panic boundary.

In any case, I could certainly imagine a generic type that uses RwLock internally, and ends up being UnwindSafe even though it's not Sync (due to the type parameter). As this is an internal choice the user of the type may not even care about threading and may inadvertently pass it across a catch_unwind boundary without realizing the danger.