Skip to content

fix: prevent UB in mapped RwLock downgrades and oneshot reclamation - #358

Closed
tisonkun wants to merge 2 commits into
apache:mainfrom
tisonkun:codex/fix-mapped-rwlock-downgrade
Closed

tisonkun wants to merge 2 commits into
apache:mainfrom
tisonkun:codex/fix-mapped-rwlock-downgrade

Conversation

@tisonkun

@tisonkun tisonkun commented Oct 11, 2026 •

Copy link
Copy Markdown
Member

Summary

Remove downgrade from borrowed and owned mapped write guards: retaining an arbitrary mutable projection while allowing readers of the original value is unsound. Both APIs were introduced by #65 and first shipped in mea v0.4.2 (2025-08-30); they remain present in v0.7.3. Document the breaking migration to downgrading the original write guard before using read-guard map or filter_map.

Add compile-fail regressions for both safe-code reproducers, exercise shared projections with interior mutability under Miri, and update existing projection tests and examples.

Fix the independent oneshot reclamation bug exposed by CI in a separate commit. Its sender finalization helper now accepts the channel pointer instead of retaining an &self parameter while the receiver can free the allocation.

Design Notes

This entirely safe program triggers undefined behavior under default Miri on the base commit:

use asyncband::rwlock::{RwLock, RwLockWriteGuard};

fn main() {
    let lock = RwLock::new(std::sync::Mutex::new(0u8));
    let write = RwLockWriteGuard::map(lock.try_write().unwrap(), |inner| {
        inner.get_mut().unwrap()
    });
    let read = write.downgrade();
    let shared = &*read;
    let outer = lock.try_read().unwrap();
    *outer.lock().unwrap() = 1;
    println!("{shared}");
}

The mutable projection bypasses the inner mutex using exclusive access. Downgrading then lets another reader lock that mutex and mutate the u8 while shared remains live. Miri reports that the later unique retag invalidates its shared read-only tag. The owned variant fails identically. Both Mutex<u8> and u8 are Send + Sync, so adding trait bounds does not fix this. A separate Cell<u8> reproducer also detects a data race after sending a mapped write guard to another thread and downgrading there.

Keeping all write permits would be safe, but would keep blocking readers and change the downgrade contract. A replacement projection from the original &T can be sound; the existing RwLockReadGuard::map(write.downgrade(), |root| &root.field) composition already provides this without an unlocked interval, with an owned equivalent. A convenience API accepting FnOnce(&T) -> &U could express the same safety boundary, but cannot reuse an arbitrary mutable projection. Tokio likewise prohibits mapped write guard downgrading and its write-guard downgrade_map requires a shared projection. Projections that require exclusive access must keep their mapped write guard.

The oneshot failure was reproduced with the CI toolchain (nightly-2026-10-11), Linux target, strict provenance, symbolic alignment checks, and seeds 0..4. In seed 1, concurrent_drop_sender_and_poll_to_completion reclaimed the allocation while finish_sender_awakening(&self, ...) still held a protected argument borrow. Passing NonNull<Channel<T>> allows that borrow to end at terminal publication; the helper never accesses the channel afterward. Existing integration and state-machine tests cover both normal publication and receiver cancellation.

Validation: cargo x test (598 passed, including both compile-fail regressions), cargo x check, and cargo x lint. Oneshot integration tests (19 per seed) and internal state tests (2 per seed) pass under the matching CI Miri toolchain, flags, Linux target, and all four seeds. The borrowed, owned, and cross-thread mapped-downgrade reproducer programs now fail to compile with E0599.

@tisonkun tisonkun changed the title fix(rwlock): remove unsound mapped write guard downgrades fix: prevent UB in mapped RwLock downgrades and oneshot reclamation Oct 11, 2026
@tisonkun tisonkun closed this Oct 11, 2026
@tisonkun

Copy link
Copy Markdown
Member Author

@orthur2 @jeffoodchain Could you help verify the mapped RwLock write guard soundness issue described here and discuss the right solution? I've closed this PR to leave the API decision open for discussion.

The following entirely safe program triggers UB under Miri on the base commit (44fc2d8):

use asyncband::rwlock::{RwLock, RwLockWriteGuard};
use std::sync::Mutex;

fn main() {
    let lock = RwLock::new(Mutex::new(0u8));
    let write = RwLockWriteGuard::map(lock.try_write().unwrap(), |inner| {
        inner.get_mut().unwrap()
    });
    let read = write.downgrade();
    let shared = &*read;
    let outer = lock.try_read().unwrap();
    *outer.lock().unwrap() = 1;
    println!("{shared}");
}

Miri reports that the mutable access through outer.lock() invalidates the shared reference. The owned mapped guard version fails in the same way. My understanding is that &mut T -> &mut U may project through interior mutability while exclusive access is held, but retaining that pointer after allowing other readers of T does not guarantee a valid &U. Both Mutex<u8> and u8 are Send + Sync, so those bounds alone do not eliminate this example.

The mapped downgrade methods were introduced in #65 and first released in mea 0.4.2; #62 and the follow-up discussion in #61 provide the original feature context.

Could you check whether the reproducer and this reasoning establish an unsound API, and which contract we should support? Keeping exclusive permits would block other readers; downgrading the original write guard before a shared projection, or providing an API that projects from the original &T, appears to preserve shared access safely. I'd appreciate your assessment before settling on removal or an alternative API.

@tisonkun
tisonkun deleted the codex/fix-mapped-rwlock-downgrade branch October 11, 2026 09:28
@orthur2

orthur2 commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

Thanks for catching this. I can reproduce it with Miri, and the owned version fails the same way. Bounds won't help, since putting the mutex behind a Box still triggers UB even though Box<Mutex<u8>> is Freeze. It isn't only an aliasing-model problem either, because with the second reader on another thread Miri reports a data race even with Stacked Borrows turned off.

The mistake in #65 was treating downgrade as a pure permit change. A &mut T -> &mut U projection is only valid while nobody else can see T. After downgrading, other readers can reach the same U through Mutex, RefCell, atomics and so on, while the guard still hands out &U.

I'd remove both methods as you proposed. tokio and lock_api don't offer downgrade on mapped write guards either. Keeping the write permits would make the method misleading, and an unsafe version would put a hard-to-check condition on callers. RwLockReadGuard::map(write.downgrade(), |root| &root.field) already covers the use case without an unlocked window, so I don't think we need downgrade_map until someone asks for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants