fix(rwlock): remove downgrade from mapped write guards

CHANGELOG.md

@@ -23,6 +23,10 @@ All notable changes to this project will be documented in this file.
 
 ## Unreleased
 
+### Breaking changes
+
+* Remove `MappedRwLockWriteGuard::downgrade` and `OwnedMappedRwLockWriteGuard::downgrade`, which could cause undefined behavior when the mapped component is reachable by other readers through interior mutability. To keep shared access to a component, downgrade the original write guard first and then map it with `RwLockReadGuard::map` or `OwnedRwLockReadGuard::map`.
+
 ### Bug fixes
 
 * Prevent `OnceMap` from keeping an empty entry when a computation fails, panics, or is cancelled while a concurrent `get` looks up the same key.

asyncband/src/rwlock/mapped_read_guard.rs

@@ -54,6 +54,9 @@ unsafe impl<T: ?Sized + Sync> Send for MappedRwLockReadGuard<'_, T> {}
 unsafe impl<T: ?Sized + Sync> Sync for MappedRwLockReadGuard<'_, T> {}
 
 impl<'a, T: ?Sized> MappedRwLockReadGuard<'a, T> {
+    /// `d` must come from a shared reference into the locked value, created while `access` was
+    /// held. A pointer derived from `&mut` access is not valid here, because other readers can
+    /// change its target through interior mutability.
     pub(crate) fn new(d: NonNull<T>, access: ReadAccess<&'a Semaphore>) -> Self {
         Self {
             d,
@@ -78,7 +81,9 @@ impl<T: ?Sized + fmt::Display> fmt::Display for MappedRwLockReadGuard<'_, T> {
 impl<T: ?Sized> Deref for MappedRwLockReadGuard<'_, T> {
     type Target = T;
     fn deref(&self) -> &Self::Target {
-        // SAFETY: we hold the read lock and the NonNull pointer is valid for the guard's lifetime
+        // SAFETY: `access` holds a read permit for the guard's lifetime, so no writer can reach the
+        // value. `d` comes from a shared reference into the locked value (see `new`), so its target
+        // changes only inside an `UnsafeCell`, as a shared reference permits.
         unsafe { self.d.as_ref() }
     }
 }

asyncband/src/rwlock/mapped_write_guard.rs

@@ -29,13 +29,17 @@ use std::ops::DerefMut;
 use std::ptr::NonNull;
 
 use crate::internal::semaphore::Semaphore;
-use crate::rwlock::MappedRwLockReadGuard;
 use crate::rwlock::access::WriteAccess;
 
 /// Exclusive access to a projection of a locked value borrowed for the guard lifetime.
 ///
 /// Use [`RwLockWriteGuard::map`](crate::rwlock::RwLockWriteGuard::map) to select a component.
 /// Dropping the guard releases its access.
+///
+/// A mapped write guard cannot be downgraded, because its projection may reach data that other
+/// readers could change through interior mutability. To keep shared access to a component,
+/// downgrade the original write guard first and then use
+/// [`RwLockReadGuard::map`](crate::rwlock::RwLockReadGuard::map).
 #[must_use = "dropping the guard releases its write access immediately"]
 pub struct MappedRwLockWriteGuard<'a, T: ?Sized> {
     d: NonNull<T>,
@@ -123,12 +127,4 @@ impl<'a, T: ?Sized> MappedRwLockWriteGuard<'a, T> {
             None => Err(orig),
         }
     }
-
-    /// Retains shared access to the same projection while releasing exclusive access.
-    ///
-    /// There is no unlocked interval in which another writer can modify the value. Queued
-    /// requests retain their order, so a waiting writer can prevent later readers from joining.
-    pub fn downgrade(self) -> MappedRwLockReadGuard<'a, T> {
-        MappedRwLockReadGuard::new(self.d, self.access.downgrade())
-    }
 }

asyncband/src/rwlock/mod.rs

@@ -60,6 +60,10 @@
 //! can be repeated, and `filter_map` returns the original guard when the component is absent.
 //! A projection closure that panics releases its guard during unwinding.
 //!
+//! Mapped write guards cannot be downgraded. To keep shared access to a component after an
+//! update, downgrade the write guard first and then map the read guard, as in
+//! `RwLockReadGuard::map(guard.downgrade(), |items| &items[0])`.
+//!
 //! ```
 //! use asyncband::rwlock::MappedRwLockWriteGuard;
 //! use asyncband::rwlock::RwLock;
@@ -71,7 +75,6 @@
 //! let slot = RwLockWriteGuard::map(queue.write().await, |items| &mut items[0]);
 //! let mut message = MappedRwLockWriteGuard::filter_map(slot, Option::as_mut).unwrap();
 //! message.push_str(" review");
-//! let message = message.downgrade();
 //! assert_eq!(&*message, "pending review");
 //! # }
 //! ```
@@ -250,4 +253,28 @@ mod compile_fail_tests {
     /// }
     /// ```
     struct OwnedMappedRwLockWriteGuardIsInvariant;
+
+    /// ```compile_fail
+    /// use asyncband::rwlock::RwLock;
+    /// use asyncband::rwlock::RwLockWriteGuard;
+    ///
+    /// let lock = RwLock::new((0, 0));
+    /// let guard = lock.try_write().unwrap();
+    /// let guard = RwLockWriteGuard::map(guard, |value| &mut value.0);
+    /// let _ = guard.downgrade();
+    /// ```
+    struct MappedRwLockWriteGuardCannotDowngrade;
+
+    /// ```compile_fail
+    /// use std::sync::Arc;
+    ///
+    /// use asyncband::rwlock::OwnedRwLockWriteGuard;
+    /// use asyncband::rwlock::RwLock;
+    ///
+    /// let lock = Arc::new(RwLock::new((0, 0)));
+    /// let guard = lock.try_write_owned().unwrap();
+    /// let guard = OwnedRwLockWriteGuard::map(guard, |value| &mut value.0);
+    /// let _ = guard.downgrade();
+    /// ```
+    struct OwnedMappedRwLockWriteGuardCannotDowngrade;
 }

asyncband/src/rwlock/owned_mapped_read_guard.rs

@@ -53,6 +53,9 @@ unsafe impl<T: ?Sized + Send + Sync, U: ?Sized + Sync> Send for OwnedMappedRwLoc
 unsafe impl<T: ?Sized + Send + Sync, U: ?Sized + Sync> Sync for OwnedMappedRwLockReadGuard<T, U> {}
 
 impl<T: ?Sized, U: ?Sized> OwnedMappedRwLockReadGuard<T, U> {
+    /// `d` must come from a shared reference into the locked value, created while `access` was
+    /// held. A pointer derived from `&mut` access is not valid here, because other readers can
+    /// change its target through interior mutability.
     pub(crate) fn new(d: NonNull<U>, access: ReadAccess<Arc<RwLock<T>>>) -> Self {
         Self {
             d,
@@ -77,7 +80,9 @@ impl<T: ?Sized, U: ?Sized + fmt::Display> fmt::Display for OwnedMappedRwLockRead
 impl<T: ?Sized, U: ?Sized> Deref for OwnedMappedRwLockReadGuard<T, U> {
     type Target = U;
     fn deref(&self) -> &Self::Target {
-        // SAFETY: we hold the read lock and the NonNull pointer is valid for the guard's lifetime
+        // SAFETY: `access` keeps the lock alive and holds a read permit, so no writer can reach the
+        // value. `d` comes from a shared reference into the locked value (see `new`), so its target
+        // changes only inside an `UnsafeCell`, as a shared reference permits.
         unsafe { self.d.as_ref() }
     }
 }

asyncband/src/rwlock/owned_mapped_write_guard.rs

@@ -29,14 +29,18 @@ use std::ops::DerefMut;
 use std::ptr::NonNull;
 use std::sync::Arc;
 
-use crate::rwlock::OwnedMappedRwLockReadGuard;
 use crate::rwlock::RwLock;
 use crate::rwlock::access::WriteAccess;
 
 /// Exclusive access to a projection of a locked value kept alive by an `Arc`.
 ///
 /// Use [`OwnedRwLockWriteGuard::map`](crate::rwlock::OwnedRwLockWriteGuard::map) to select a
 /// component. Dropping the guard releases its access.
+///
+/// A mapped write guard cannot be downgraded, because its projection may reach data that other
+/// readers could change through interior mutability. To keep shared access to a component,
+/// downgrade the original write guard first and then use
+/// [`OwnedRwLockReadGuard::map`](crate::rwlock::OwnedRwLockReadGuard::map).
 #[must_use = "dropping the guard releases its write access immediately"]
 pub struct OwnedMappedRwLockWriteGuard<T: ?Sized, U: ?Sized> {
     d: NonNull<U>,
@@ -125,12 +129,4 @@ impl<T: ?Sized, U: ?Sized> OwnedMappedRwLockWriteGuard<T, U> {
             None => Err(orig),
         }
     }
-
-    /// Retains shared access to the same projection while releasing exclusive access.
-    ///
-    /// There is no unlocked interval in which another writer can modify the value. Queued
-    /// requests retain their order, so a waiting writer can prevent later readers from joining.
-    pub fn downgrade(self) -> OwnedMappedRwLockReadGuard<T, U> {
-        OwnedMappedRwLockReadGuard::new(self.d, self.access.downgrade())
-    }
 }

asyncband/src/rwlock/owned_write_guard.rs

@@ -138,6 +138,9 @@ impl<T: ?Sized> OwnedRwLockWriteGuard<T> {
     ///
     /// There is no unlocked interval in which another writer can modify the value. Queued
     /// requests retain their order, so a waiting writer can prevent later readers from joining.
+    ///
+    /// To keep shared access to a component, call this before [`OwnedRwLockReadGuard::map`] or
+    /// [`OwnedRwLockReadGuard::filter_map`]. Mapped write guards cannot be downgraded.
     pub fn downgrade(self) -> OwnedRwLockReadGuard<T> {
         OwnedRwLockReadGuard {
             access: self.access.downgrade(),

asyncband/src/rwlock/write_guard.rs

@@ -133,6 +133,9 @@ impl<'a, T: ?Sized> RwLockWriteGuard<'a, T> {
     ///
     /// There is no unlocked interval in which another writer can modify the value. Queued
     /// requests retain their order, so a waiting writer can prevent later readers from joining.
+    ///
+    /// To keep shared access to a component, call this before [`RwLockReadGuard::map`] or
+    /// [`RwLockReadGuard::filter_map`]. Mapped write guards cannot be downgraded.
     pub fn downgrade(self) -> RwLockReadGuard<'a, T> {
         RwLockReadGuard {
             access: self.access.downgrade(),

tests-integration/tests/rwlock_test.rs

@@ -144,7 +144,6 @@ async fn owned_write_mappings_preserve_lock_ownership() {
     let mut value =
         OwnedMappedRwLockWriteGuard::filter_map(value, |value| value.first_mut()).unwrap();
     *value = 100;
-    let value = value.downgrade();
 
     drop(rwlock);
     assert!(weak.upgrade().is_some());
@@ -184,7 +183,7 @@ async fn owned_read_mappings_preserve_lock_ownership() {
 }
 
 #[tokio::test]
-async fn every_write_guard_type_can_downgrade() {
+async fn write_guards_downgrade_before_mapping() {
     let rwlock = Arc::new(RwLock::new((42, "test".to_string())));
 
     {
@@ -213,23 +212,22 @@ async fn every_write_guard_type_can_downgrade() {
     }
 
     {
-        let write_guard = rwlock.write().await;
-        let mut mapped_write = RwLockWriteGuard::map(write_guard, |data| &mut data.0);
-        *mapped_write = 200;
+        let mut write_guard = rwlock.write().await;
+        write_guard.0 = 200;
 
-        let mapped_read = mapped_write.downgrade();
+        let mapped_read = RwLockReadGuard::map(write_guard.downgrade(), |data| &data.0);
         assert_eq!(*mapped_read, 200);
 
         assert!(rwlock.try_write().is_none());
+        assert_eq!(rwlock.try_read().unwrap().0, 200);
         drop(mapped_read);
     }
 
     {
-        let owned_write = rwlock.clone().write_owned().await;
-        let mut owned_mapped = OwnedRwLockWriteGuard::map(owned_write, |data| &mut data.1);
-        *owned_mapped = "final".to_string();
+        let mut owned_write = rwlock.clone().write_owned().await;
+        owned_write.1 = "final".to_string();
 
-        let owned_mapped_read = owned_mapped.downgrade();
+        let owned_mapped_read = OwnedRwLockReadGuard::map(owned_write.downgrade(), |data| &data.1);
         assert_eq!(*owned_mapped_read, "final");
 
         let moved_task = tokio::spawn(async move {

tests-integration/tests/unsafe_paths_test.rs

@@ -21,6 +21,7 @@ use std::marker::PhantomPinned;
 use std::pin::Pin;
 use std::pin::pin;
 use std::ptr;
+use std::sync;
 use std::sync::Arc;
 use std::task::Context;
 use std::task::Poll;
@@ -91,8 +92,8 @@ fn mapped_rwlock_guards_preserve_lock_ownership() {
     let mapped = RwLockWriteGuard::map(write, |data| &mut data.values);
     let mut mapped = MappedRwLockWriteGuard::map(mapped, |values| &mut values[1]);
     *mapped = 3;
-    let mapped = mapped.downgrade();
     assert_eq!(*mapped, 3);
+    assert!(lock.try_read().is_none());
     assert!(lock.try_write().is_none());
     drop(mapped);
 
@@ -113,8 +114,8 @@ fn mapped_rwlock_guards_preserve_lock_ownership() {
     let mut mapped =
         OwnedMappedRwLockWriteGuard::filter_map(mapped, |values| values.first_mut()).unwrap();
     *mapped = 6;
-    let mapped = mapped.downgrade();
     assert_eq!(*mapped, 6);
+    assert!(lock.try_read().is_none());
     drop(mapped);
 
     let read = lock.clone().try_read_owned().unwrap();
@@ -128,6 +129,46 @@ fn mapped_rwlock_guards_preserve_lock_ownership() {
     assert!(weak.upgrade().is_none());
 }
 
+#[test]
+fn downgraded_shared_projection_preserves_interior_mutability() {
+    let lock = RwLock::new(Some(sync::Mutex::new(0u8)));
+    let mut write = lock.try_write().unwrap();
+    *write.as_mut().unwrap().get_mut().unwrap() = 1;
+    let read = RwLockReadGuard::map(write.downgrade(), |value| value.as_ref().unwrap());
+
+    // The shared projection retains the mutex, so access to its contents remains synchronized.
+    let shared = &*read;
+    let outer = lock.try_read().unwrap();
+    *outer.as_ref().unwrap().lock().unwrap() = 2;
+    assert_eq!(*shared.lock().unwrap(), 2);
+    assert!(lock.try_write().is_none());
+
+    drop(outer);
+    drop(read);
+    assert!(lock.try_write().is_some());
+}
+
+#[test]
+fn owned_downgraded_shared_projection_preserves_interior_mutability() {
+    let lock = Arc::new(RwLock::new(Some(sync::Mutex::new(0u8))));
+    let weak = Arc::downgrade(&lock);
+    let mut write = lock.clone().try_write_owned().unwrap();
+    *write.as_mut().unwrap().get_mut().unwrap() = 1;
+    let read = OwnedRwLockReadGuard::filter_map(write.downgrade(), Option::as_ref).unwrap();
+
+    let shared = &*read;
+    let outer = lock.try_read().unwrap();
+    *outer.as_ref().unwrap().lock().unwrap() = 2;
+    assert_eq!(*shared.lock().unwrap(), 2);
+    assert!(lock.try_write().is_none());
+
+    drop(outer);
+    drop(lock);
+    assert_eq!(*shared.lock().unwrap(), 2);
+    drop(read);
+    assert!(weak.upgrade().is_none());
+}
+
 struct AddressSensitiveFuture {
     address: Cell<*const Self>,
     _pin: PhantomPinned,