fix(rwlock): remove downgrade from mapped write guards

[orthur2]

Summary

Closes #362.

Remove MappedRwLockWriteGuard::downgrade and OwnedMappedRwLockWriteGuard::downgrade, which are unsound. They were added in #65, first shipped in mea 0.4.2, and are still in 0.7.3. To keep shared access to a component, downgrade the original write guard first and then map it with RwLockReadGuard::map or OwnedRwLockReadGuard::map. The docs and the changelog describe this migration.

Design Notes

The projection of a mapped write guard can go through interior mutability using exclusive access, as Mutex::get_mut does. After a downgrade, another reader can lock the same mutex and change that data while the guard still hands out &U. With a RefCell<Box<_>> it is even a use-after-free in single-threaded safe code. No bound on the types rules this out, and keeping the write permits would make the method pointless, so this follows tokio and lock_api in not offering it. A downgrade_map taking FnOnce(&T) -> &U would only shorten the migration above, so it can wait until someone needs it. The reproducers and discussion are in #358 and #362.

Two compile-fail doctests keep the removed methods from coming back. Two Miri tests in unsafe_paths_test take the migration path while a second reader locks a mutex inside the projection. No other Miri test keeps a projected read guard alive while another reader uses the lock, so these would catch a read projection derived through &mut.

[tisonkun]

I'm considering to release a 0.7.4 recently to include fixes like #361

Then we will merge this PR that includes breaking changes.

@QwQBiG do you happen to have some time the following week to handle this?

[orthur2]

Sounds good. @QwQBiG, the release skill covers the whole process, from the tracking issue through the ATR vote to publication. Feel free to ask me if anything is unclear. I went through it for 0.7.3.

[jeffoodchain]

sorry I actually don't know much related to the releasing process. But I think it would be better to add a flag like #[deprecated(note = "unsound, fixed in #365")]? (just to avoid the bug from releasing)

[QwQBiG]

Sure, I can handle the 0.7.4 release. I'll confirm the release scope and follow the release skill.

Thanks @orthur2 for offering to help!

[orthur2]

Good idea. A deprecation warning in 0.7.4 would point users at the migration before the methods go away. I’d put the migration in the note instead of a PR number, since that’s what users can act on, e.g. “downgrade the original write guard, then map the read guard”.

@QwQBiG @tisonkun should I send a small PR for 0.7.4 that deprecates both methods, or would you rather handle it as part of the release?

[orthur2]

@jeffoodchain @Soumya2004-java could you take a look when you have time?

GitHub only lets me request reviews from collaborators, so I couldn’t add you as reviewers, but you can still leave a review on the PR directly.

[jeffoodchain]

yes ofc, would be happy to review. I will do this tomorrow. 🙏

[Soumya2004-java]

I reviewed the fix and it looks good to me. Removing both methods seems to address the issue, and the migration approach makes sense.

I saw the discussion about releasing v0.7.4 before merging this breaking change. That sounds reasonable.

I couldn't run the full test suite locally because Cargo couldn't resolve the crates.io download host, so I haven't independently verified the new tests under Miri.

[jeffoodchain]

@orthur2 I ran the reproducers from #358 and #362 under Miri on 5ddd9ad and on this PR (cf6a23f). On 5ddd9ad they report UB; on this PR they fail to compile with E0599.

One documentation point. The bug is in downgrade, but the UB happens in the unsafe block of MappedRwLockReadGuard::deref, which relies on a rule that is not written down: d must come from a shared reference. I suggest documenting it on new and in the deref SAFETY comment, for both MappedRwLockReadGuard and OwnedMappedRwLockReadGuard. For example, on new:

/// `d` must come from a shared reference into the locked value, created while `access` was
/// held. A pointer derived from `&mut` access is invalid here: other readers can change its
/// target through interior mutability (#362).
pub(crate) fn new(...)

and for deref:

// SAFETY: `access` 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.

The compile-fail tests only stop these two method names from coming back. #169 would add more access transitions (upgrade, and downgrade between upgradable and read access), and each one is a new way to build a read guard from an &mut-derived pointer.

LGTM for the code.

[QwQBiG]

+1 LGTM

[orthur2]

@jeffoodchain Updated the new docs and both deref SAFETY comments as suggested.

[jeffoodchain]

LGTM :D