Skip to content

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

Open
@lilyball

Description

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.

Metadata

Assignees

No one assigned

    Labels

    A-concurrencyArea: ConcurrencyC-bugCategory: This is a bug.T-libs-apiRelevant to the library API team, which will review and decide on the PR/issue.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions