diff options
| author | Matthias Krüger <matthias.krueger@famsik.de> | 2022-11-20 23:50:26 +0100 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2022-11-20 23:50:26 +0100 |
| commit | b4513ce6f835ed3f5c34a3ddc745d8cd3198cbf2 (patch) | |
| tree | 489b4a96bbf05ccbf66a5d7416bba84405eb472f | |
| parent | a28f3c88e50a77bc2a91889241248c4543854e61 (diff) | |
| parent | 734f7244723f579433b7defcd1fd1c2e66406737 (diff) | |
| download | rust-b4513ce6f835ed3f5c34a3ddc745d8cd3198cbf2.tar.gz rust-b4513ce6f835ed3f5c34a3ddc745d8cd3198cbf2.zip | |
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](https://github.com/rust-lang/rust/issues/63292#issuecomment-568284090) 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).
| -rw-r--r-- | library/alloc/src/rc.rs | 41 | ||||
| -rw-r--r-- | library/alloc/src/sync.rs | 41 |
2 files changed, 74 insertions, 8 deletions
diff --git a/library/alloc/src/rc.rs b/library/alloc/src/rc.rs index 5f307069d80..38e31b1802a 100644 --- a/library/alloc/src/rc.rs +++ b/library/alloc/src/rc.rs @@ -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 /// @@ -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 { diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index b69f6b03112..f7dc4d1094c 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -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 /// @@ -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 { |
