Skip to content

Commit b4513ce

Browse files
authored
Rollup merge of #101310 - zachs18:rc_get_unchecked_mut_docs_soundness, r=Mark-Simulacrum
Clarify and restrict when `{Arc,Rc}::get_unchecked_mut` is allowed. (Tracking issue for `{Arc,Rc}::get_unchecked_mut`: #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](#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://github.com/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).
2 parents a28f3c8 + 734f724 commit b4513ce

File tree

2 files changed

+74
-8
lines changed

2 files changed

+74
-8
lines changed

library/alloc/src/rc.rs

+37-4
Original file line numberDiff line numberDiff line change
@@ -1091,10 +1091,11 @@ impl<T: ?Sized> Rc<T> {
10911091
///
10921092
/// # Safety
10931093
///
1094-
/// Any other `Rc` or [`Weak`] pointers to the same allocation must not be dereferenced
1095-
/// for the duration of the returned borrow.
1096-
/// This is trivially the case if no such pointers exist,
1097-
/// for example immediately after `Rc::new`.
1094+
/// If any other `Rc` or [`Weak`] pointers to the same allocation exist, then
1095+
/// they must be must not be dereferenced or have active borrows for the duration
1096+
/// of the returned borrow, and their inner type must be exactly the same as the
1097+
/// inner type of this Rc (including lifetimes). This is trivially the case if no
1098+
/// such pointers exist, for example immediately after `Rc::new`.
10981099
///
10991100
/// # Examples
11001101
///
@@ -1109,6 +1110,38 @@ impl<T: ?Sized> Rc<T> {
11091110
/// }
11101111
/// assert_eq!(*x, "foo");
11111112
/// ```
1113+
/// Other `Rc` pointers to the same allocation must be to the same type.
1114+
/// ```no_run
1115+
/// #![feature(get_mut_unchecked)]
1116+
///
1117+
/// use std::rc::Rc;
1118+
///
1119+
/// let x: Rc<str> = Rc::from("Hello, world!");
1120+
/// let mut y: Rc<[u8]> = x.clone().into();
1121+
/// unsafe {
1122+
/// // this is Undefined Behavior, because x's inner type is str, not [u8]
1123+
/// Rc::get_mut_unchecked(&mut y).fill(0xff); // 0xff is invalid in UTF-8
1124+
/// }
1125+
/// println!("{}", &*x); // Invalid UTF-8 in a str
1126+
/// ```
1127+
/// Other `Rc` pointers to the same allocation must be to the exact same type, including lifetimes.
1128+
/// ```no_run
1129+
/// #![feature(get_mut_unchecked)]
1130+
///
1131+
/// use std::rc::Rc;
1132+
///
1133+
/// let x: Rc<&str> = Rc::new("Hello, world!");
1134+
/// {
1135+
/// let s = String::from("Oh, no!");
1136+
/// let mut y: Rc<&str> = x.clone().into();
1137+
/// unsafe {
1138+
/// // this is Undefined Behavior, because x's inner type
1139+
/// // is &'long str, not &'short str
1140+
/// *Rc::get_mut_unchecked(&mut y) = &s;
1141+
/// }
1142+
/// }
1143+
/// println!("{}", &*x); // Use-after-free
1144+
/// ```
11121145
#[inline]
11131146
#[unstable(feature = "get_mut_unchecked", issue = "63292")]
11141147
pub unsafe fn get_mut_unchecked(this: &mut Self) -> &mut T {

library/alloc/src/sync.rs

+37-4
Original file line numberDiff line numberDiff line change
@@ -1587,10 +1587,11 @@ impl<T: ?Sized> Arc<T> {
15871587
///
15881588
/// # Safety
15891589
///
1590-
/// Any other `Arc` or [`Weak`] pointers to the same allocation must not be dereferenced
1591-
/// for the duration of the returned borrow.
1592-
/// This is trivially the case if no such pointers exist,
1593-
/// for example immediately after `Arc::new`.
1590+
/// If any other `Arc` or [`Weak`] pointers to the same allocation exist, then
1591+
/// they must be must not be dereferenced or have active borrows for the duration
1592+
/// of the returned borrow, and their inner type must be exactly the same as the
1593+
/// inner type of this Rc (including lifetimes). This is trivially the case if no
1594+
/// such pointers exist, for example immediately after `Arc::new`.
15941595
///
15951596
/// # Examples
15961597
///
@@ -1605,6 +1606,38 @@ impl<T: ?Sized> Arc<T> {
16051606
/// }
16061607
/// assert_eq!(*x, "foo");
16071608
/// ```
1609+
/// Other `Arc` pointers to the same allocation must be to the same type.
1610+
/// ```no_run
1611+
/// #![feature(get_mut_unchecked)]
1612+
///
1613+
/// use std::sync::Arc;
1614+
///
1615+
/// let x: Arc<str> = Arc::from("Hello, world!");
1616+
/// let mut y: Arc<[u8]> = x.clone().into();
1617+
/// unsafe {
1618+
/// // this is Undefined Behavior, because x's inner type is str, not [u8]
1619+
/// Arc::get_mut_unchecked(&mut y).fill(0xff); // 0xff is invalid in UTF-8
1620+
/// }
1621+
/// println!("{}", &*x); // Invalid UTF-8 in a str
1622+
/// ```
1623+
/// Other `Arc` pointers to the same allocation must be to the exact same type, including lifetimes.
1624+
/// ```no_run
1625+
/// #![feature(get_mut_unchecked)]
1626+
///
1627+
/// use std::sync::Arc;
1628+
///
1629+
/// let x: Arc<&str> = Arc::new("Hello, world!");
1630+
/// {
1631+
/// let s = String::from("Oh, no!");
1632+
/// let mut y: Arc<&str> = x.clone().into();
1633+
/// unsafe {
1634+
/// // this is Undefined Behavior, because x's inner type
1635+
/// // is &'long str, not &'short str
1636+
/// *Arc::get_mut_unchecked(&mut y) = &s;
1637+
/// }
1638+
/// }
1639+
/// println!("{}", &*x); // Use-after-free
1640+
/// ```
16081641
#[inline]
16091642
#[unstable(feature = "get_mut_unchecked", issue = "63292")]
16101643
pub unsafe fn get_mut_unchecked(this: &mut Self) -> &mut T {

0 commit comments

Comments
 (0)