RwLock is UnwindSafe
despite not poisoning on read()
, can cause broken invariants #89832
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.