Skip to content

Commit

Permalink
Rollup merge of rust-lang#101310 - zachs18:rc_get_unchecked_mut_docs_…
Browse files Browse the repository at this point in the history
…soundness, r=Mark-Simulacrum

Clarify and restrict when `{Arc,Rc}::get_unchecked_mut` is allowed.

(Tracking issue for `{Arc,Rc}::get_unchecked_mut`: rust-lang#63292)

(I'm using `Rc` in this comment, but it applies for `Arc` all the same).

As currently documented, `Rc::get_unchecked_mut` can lead to unsoundness when multiple `Rc`/`Weak` pointers to the same allocation exist. The current documentation only requires that other `Rc`/`Weak` pointers to the same allocation "must not be dereferenced for the duration of the returned borrow". This can lead to unsoundness in (at least) two ways: variance, and `Rc<str>`/`Rc<[u8]>` aliasing. ([playground link](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=d7e2d091c389f463d121630ab0a37320)).

This PR changes the documentation of `Rc::get_unchecked_mut` to restrict usage to when all `Rc<T>`/`Weak<T>` have the exact same `T` (including lifetimes). I believe this is sufficient to prevent unsoundness, while still allowing `get_unchecked_mut` to be called on an aliased `Rc` as long as the safety contract is upheld by the caller.

## Alternatives

* A less strict, but still sound alternative would be to say that the caller must only write values which are valid for all aliased `Rc`/`Weak` inner types. (This was [mentioned](rust-lang#63292 (comment)) in the tracking issue). This may be too complicated to clearly express in the documentation.
* A more strict alternative would be to say that there must not be any aliased `Rc`/`Weak` pointers, i.e. it is required that get_mut would return `Some(_)`. (This was also mentioned in the tracking issue). There is at least one codebase that this would cause to become unsound ([here](https:/kaimast/lsm-rs/blob/be5a164d770d850d905e510e2966ad4b1cc9aa5e/src/memtable.rs#L166), where additional locking is used to ensure unique access to an aliased `Rc<T>`;  I saw this because it was linked on the tracking issue).
  • Loading branch information
matthiaskrgr authored Nov 20, 2022
2 parents a28f3c8 + 734f724 commit b4513ce
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 8 deletions.
41 changes: 37 additions & 4 deletions library/alloc/src/rc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1091,10 +1091,11 @@ impl<T: ?Sized> Rc<T> {
///
/// # Safety
///
/// Any other `Rc` or [`Weak`] pointers to the same allocation must not be dereferenced
/// for the duration of the returned borrow.
/// This is trivially the case if no such pointers exist,
/// for example immediately after `Rc::new`.
/// If any other `Rc` or [`Weak`] pointers to the same allocation exist, then
/// they must be must not be dereferenced or have active borrows for the duration
/// of the returned borrow, and their inner type must be exactly the same as the
/// inner type of this Rc (including lifetimes). This is trivially the case if no
/// such pointers exist, for example immediately after `Rc::new`.
///
/// # Examples
///
Expand All @@ -1109,6 +1110,38 @@ impl<T: ?Sized> Rc<T> {
/// }
/// assert_eq!(*x, "foo");
/// ```
/// Other `Rc` pointers to the same allocation must be to the same type.
/// ```no_run
/// #![feature(get_mut_unchecked)]
///
/// use std::rc::Rc;
///
/// let x: Rc<str> = Rc::from("Hello, world!");
/// let mut y: Rc<[u8]> = x.clone().into();
/// unsafe {
/// // this is Undefined Behavior, because x's inner type is str, not [u8]
/// Rc::get_mut_unchecked(&mut y).fill(0xff); // 0xff is invalid in UTF-8
/// }
/// println!("{}", &*x); // Invalid UTF-8 in a str
/// ```
/// Other `Rc` pointers to the same allocation must be to the exact same type, including lifetimes.
/// ```no_run
/// #![feature(get_mut_unchecked)]
///
/// use std::rc::Rc;
///
/// let x: Rc<&str> = Rc::new("Hello, world!");
/// {
/// let s = String::from("Oh, no!");
/// let mut y: Rc<&str> = x.clone().into();
/// unsafe {
/// // this is Undefined Behavior, because x's inner type
/// // is &'long str, not &'short str
/// *Rc::get_mut_unchecked(&mut y) = &s;
/// }
/// }
/// println!("{}", &*x); // Use-after-free
/// ```
#[inline]
#[unstable(feature = "get_mut_unchecked", issue = "63292")]
pub unsafe fn get_mut_unchecked(this: &mut Self) -> &mut T {
Expand Down
41 changes: 37 additions & 4 deletions library/alloc/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1587,10 +1587,11 @@ impl<T: ?Sized> Arc<T> {
///
/// # Safety
///
/// Any other `Arc` or [`Weak`] pointers to the same allocation must not be dereferenced
/// for the duration of the returned borrow.
/// This is trivially the case if no such pointers exist,
/// for example immediately after `Arc::new`.
/// If any other `Arc` or [`Weak`] pointers to the same allocation exist, then
/// they must be must not be dereferenced or have active borrows for the duration
/// of the returned borrow, and their inner type must be exactly the same as the
/// inner type of this Rc (including lifetimes). This is trivially the case if no
/// such pointers exist, for example immediately after `Arc::new`.
///
/// # Examples
///
Expand All @@ -1605,6 +1606,38 @@ impl<T: ?Sized> Arc<T> {
/// }
/// assert_eq!(*x, "foo");
/// ```
/// Other `Arc` pointers to the same allocation must be to the same type.
/// ```no_run
/// #![feature(get_mut_unchecked)]
///
/// use std::sync::Arc;
///
/// let x: Arc<str> = Arc::from("Hello, world!");
/// let mut y: Arc<[u8]> = x.clone().into();
/// unsafe {
/// // this is Undefined Behavior, because x's inner type is str, not [u8]
/// Arc::get_mut_unchecked(&mut y).fill(0xff); // 0xff is invalid in UTF-8
/// }
/// println!("{}", &*x); // Invalid UTF-8 in a str
/// ```
/// Other `Arc` pointers to the same allocation must be to the exact same type, including lifetimes.
/// ```no_run
/// #![feature(get_mut_unchecked)]
///
/// use std::sync::Arc;
///
/// let x: Arc<&str> = Arc::new("Hello, world!");
/// {
/// let s = String::from("Oh, no!");
/// let mut y: Arc<&str> = x.clone().into();
/// unsafe {
/// // this is Undefined Behavior, because x's inner type
/// // is &'long str, not &'short str
/// *Arc::get_mut_unchecked(&mut y) = &s;
/// }
/// }
/// println!("{}", &*x); // Use-after-free
/// ```
#[inline]
#[unstable(feature = "get_mut_unchecked", issue = "63292")]
pub unsafe fn get_mut_unchecked(this: &mut Self) -> &mut T {
Expand Down

0 comments on commit b4513ce

Please sign in to comment.