From 493c037699603388a00010d96339d84e84e361c6 Mon Sep 17 00:00:00 2001 From: carbotaniuman <41451839+carbotaniuman@users.noreply.github.com> Date: Wed, 9 Sep 2020 12:11:44 -0500 Subject: [PATCH 1/4] Eliminate mut reference UB in Drop impl for Rc This changes `self.ptr.as_mut()` with `get_mut_unchecked` which does not use an intermediate reference. Arc already handled this case properly. --- library/alloc/src/rc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/alloc/src/rc.rs b/library/alloc/src/rc.rs index a9b293856e57b..fe429e6e9464c 100644 --- a/library/alloc/src/rc.rs +++ b/library/alloc/src/rc.rs @@ -1195,7 +1195,7 @@ unsafe impl<#[may_dangle] T: ?Sized> Drop for Rc { self.dec_strong(); if self.strong() == 0 { // destroy the contained object - ptr::drop_in_place(self.ptr.as_mut()); + ptr::drop_in_place(Self::get_mut_unchecked(self)); // remove the implicit "strong weak" pointer now that we've // destroyed the contents. From 8f43fa09893b664e4ceb4cc8c7815fa5ab20c10e Mon Sep 17 00:00:00 2001 From: carbotaniuman <41451839+carbotaniuman@users.noreply.github.com> Date: Wed, 9 Sep 2020 13:39:48 -0500 Subject: [PATCH 2/4] Add WeakInner<'_> and have Weak::inner() return it This avoids overlapping a reference covering the data field, which may be changed due in concurrent conditions. This fully fixed the UB mainfested with `new_cyclic`. --- library/alloc/src/rc.rs | 109 ++++++++++++++++++++++++++-------------- 1 file changed, 70 insertions(+), 39 deletions(-) diff --git a/library/alloc/src/rc.rs b/library/alloc/src/rc.rs index fe429e6e9464c..43cf3fe6fb6ac 100644 --- a/library/alloc/src/rc.rs +++ b/library/alloc/src/rc.rs @@ -469,7 +469,7 @@ impl Rc { // the strong count, and then remove the implicit "strong weak" // pointer while also handling drop logic by just crafting a // fake Weak. - this.dec_strong(); + this.inner().dec_strong(); let _weak = Weak { ptr: this.ptr }; forget(this); Ok(val) @@ -735,7 +735,7 @@ impl Rc { /// ``` #[stable(feature = "rc_weak", since = "1.4.0")] pub fn downgrade(this: &Self) -> Weak { - this.inc_weak(); + this.inner().inc_weak(); // Make sure we do not create a dangling Weak debug_assert!(!is_dangling(this.ptr)); Weak { ptr: this.ptr } @@ -756,7 +756,7 @@ impl Rc { #[inline] #[stable(feature = "rc_counts", since = "1.15.0")] pub fn weak_count(this: &Self) -> usize { - this.weak() - 1 + this.inner().weak() - 1 } /// Gets the number of strong (`Rc`) pointers to this allocation. @@ -774,7 +774,7 @@ impl Rc { #[inline] #[stable(feature = "rc_counts", since = "1.15.0")] pub fn strong_count(this: &Self) -> usize { - this.strong() + this.inner().strong() } /// Returns `true` if there are no other `Rc` or [`Weak`] pointers to @@ -844,7 +844,16 @@ impl Rc { #[inline] #[unstable(feature = "get_mut_unchecked", issue = "63292")] pub unsafe fn get_mut_unchecked(this: &mut Self) -> &mut T { - unsafe { &mut this.ptr.as_mut().value } + // We are careful to *not* create a reference covering the "count" fields, as + // this would alias with reenterant access to the reference counts (e.g. by `Weak`). + unsafe { &mut (*this.ptr.as_ptr()).value } + } + + #[inline] + fn inner(&self) -> &RcBox { + // This unsafety is ok because while this Rc is alive we're guaranteed + // that the inner pointer is valid. + unsafe { self.ptr.as_ref() } } #[inline] @@ -931,10 +940,10 @@ impl Rc { unsafe { let mut swap = Rc::new(ptr::read(&this.ptr.as_ref().value)); mem::swap(this, &mut swap); - swap.dec_strong(); + swap.inner().dec_strong(); // Remove implicit strong-weak ref (no need to craft a fake // Weak here -- we know other Weaks can clean up for us) - swap.dec_weak(); + swap.inner().dec_weak(); forget(swap); } } @@ -1192,16 +1201,16 @@ unsafe impl<#[may_dangle] T: ?Sized> Drop for Rc { /// ``` fn drop(&mut self) { unsafe { - self.dec_strong(); - if self.strong() == 0 { + self.inner().dec_strong(); + if self.inner().strong() == 0 { // destroy the contained object ptr::drop_in_place(Self::get_mut_unchecked(self)); // remove the implicit "strong weak" pointer now that we've // destroyed the contents. - self.dec_weak(); + self.inner().dec_weak(); - if self.weak() == 0 { + if self.inner().weak() == 0 { Global.dealloc(self.ptr.cast(), Layout::for_value(self.ptr.as_ref())); } } @@ -1227,7 +1236,7 @@ impl Clone for Rc { /// ``` #[inline] fn clone(&self) -> Rc { - self.inc_strong(); + self.inner().inc_strong(); Self::from_inner(self.ptr) } } @@ -1851,6 +1860,13 @@ pub(crate) fn is_dangling(ptr: NonNull) -> bool { address == usize::MAX } +/// Helper type to allow accessing the reference counts without +/// making any assertions about the data field. +struct WeakInner<'a> { + weak: &'a Cell, + strong: &'a Cell, +} + impl Weak { /// Attempts to upgrade the `Weak` pointer to an [`Rc`], delaying /// dropping of the inner value if successful. @@ -1910,11 +1926,21 @@ impl Weak { .unwrap_or(0) } - /// Returns `None` when the pointer is dangling and there is no allocated `RcBox` + /// Returns `None` when the pointer is dangling and there is no allocated `RcBox`, /// (i.e., when this `Weak` was created by `Weak::new`). #[inline] - fn inner(&self) -> Option<&RcBox> { - if is_dangling(self.ptr) { None } else { Some(unsafe { self.ptr.as_ref() }) } + fn inner(&self) -> Option> { + if is_dangling(self.ptr) { + None + } else { + // We are careful to *not* create a reference covering the "data" field, as + // the field may be mutated concurrently (for example, if the last `Rc` + // is dropped, the data field will be dropped in-place). + Some(unsafe { + let ptr = self.ptr.as_ptr(); + WeakInner { strong: &(*ptr).strong, weak: &(*ptr).weak } + }) + } } /// Returns `true` if the two `Weak`s point to the same allocation (similar to @@ -1992,14 +2018,14 @@ impl Drop for Weak { /// assert!(other_weak_foo.upgrade().is_none()); /// ``` fn drop(&mut self) { - if let Some(inner) = self.inner() { - inner.dec_weak(); - // the weak count starts at 1, and will only go to zero if all - // the strong pointers have disappeared. - if inner.weak() == 0 { - unsafe { - Global.dealloc(self.ptr.cast(), Layout::for_value(self.ptr.as_ref())); - } + let inner = if let Some(inner) = self.inner() { inner } else { return }; + + inner.dec_weak(); + // the weak count starts at 1, and will only go to zero if all + // the strong pointers have disappeared. + if inner.weak() == 0 { + unsafe { + Global.dealloc(self.ptr.cast(), Layout::for_value(self.ptr.as_ref())); } } } @@ -2065,12 +2091,13 @@ impl Default for Weak { // clone these much in Rust thanks to ownership and move-semantics. #[doc(hidden)] -trait RcBoxPtr { - fn inner(&self) -> &RcBox; +trait RcInnerPtr { + fn weak_ref(&self) -> &Cell; + fn strong_ref(&self) -> &Cell; #[inline] fn strong(&self) -> usize { - self.inner().strong.get() + self.strong_ref().get() } #[inline] @@ -2084,17 +2111,17 @@ trait RcBoxPtr { if strong == 0 || strong == usize::MAX { abort(); } - self.inner().strong.set(strong + 1); + self.strong_ref().set(strong + 1); } #[inline] fn dec_strong(&self) { - self.inner().strong.set(self.strong() - 1); + self.strong_ref().set(self.strong() - 1); } #[inline] fn weak(&self) -> usize { - self.inner().weak.get() + self.weak_ref().get() } #[inline] @@ -2108,26 +2135,30 @@ trait RcBoxPtr { if weak == 0 || weak == usize::MAX { abort(); } - self.inner().weak.set(weak + 1); + self.weak_ref().set(weak + 1); } #[inline] fn dec_weak(&self) { - self.inner().weak.set(self.weak() - 1); + self.weak_ref().set(self.weak() - 1); } } -impl RcBoxPtr for Rc { - #[inline(always)] - fn inner(&self) -> &RcBox { - unsafe { self.ptr.as_ref() } +impl RcInnerPtr for RcBox { + fn weak_ref(&self) -> &Cell { + &self.weak + } + fn strong_ref(&self) -> &Cell { + &self.strong } } -impl RcBoxPtr for RcBox { - #[inline(always)] - fn inner(&self) -> &RcBox { - self +impl<'a> RcInnerPtr for WeakInner<'a> { + fn weak_ref(&self) -> &Cell { + self.weak + } + fn strong_ref(&self) -> &Cell { + self.strong } } From bb57c9f91c920a8b1533fb92aa50be342e11f675 Mon Sep 17 00:00:00 2001 From: carbotaniuman <41451839+carbotaniuman@users.noreply.github.com> Date: Wed, 9 Sep 2020 13:44:22 -0500 Subject: [PATCH 3/4] Format --- library/alloc/src/rc.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/alloc/src/rc.rs b/library/alloc/src/rc.rs index 43cf3fe6fb6ac..4821c8f567626 100644 --- a/library/alloc/src/rc.rs +++ b/library/alloc/src/rc.rs @@ -845,7 +845,7 @@ impl Rc { #[unstable(feature = "get_mut_unchecked", issue = "63292")] pub unsafe fn get_mut_unchecked(this: &mut Self) -> &mut T { // We are careful to *not* create a reference covering the "count" fields, as - // this would alias with reenterant access to the reference counts (e.g. by `Weak`). + // this would alias with concurrent access to the reference counts (e.g. by `Weak`). unsafe { &mut (*this.ptr.as_ptr()).value } } @@ -2019,7 +2019,7 @@ impl Drop for Weak { /// ``` fn drop(&mut self) { let inner = if let Some(inner) = self.inner() { inner } else { return }; - + inner.dec_weak(); // the weak count starts at 1, and will only go to zero if all // the strong pointers have disappeared. @@ -2144,7 +2144,7 @@ trait RcInnerPtr { } } -impl RcInnerPtr for RcBox { +impl RcInnerPtr for RcBox { fn weak_ref(&self) -> &Cell { &self.weak } From b729368d4e6a7e6a85dd4189ca16d49622a1582a Mon Sep 17 00:00:00 2001 From: carbotaniuman <41451839+carbotaniuman@users.noreply.github.com> Date: Fri, 11 Sep 2020 07:25:28 -0500 Subject: [PATCH 4/4] Address review comments --- library/alloc/src/rc.rs | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/library/alloc/src/rc.rs b/library/alloc/src/rc.rs index 4821c8f567626..f998e49dcfcde 100644 --- a/library/alloc/src/rc.rs +++ b/library/alloc/src/rc.rs @@ -295,6 +295,13 @@ impl, U: ?Sized> CoerceUnsized> for Rc {} impl, U: ?Sized> DispatchFromDyn> for Rc {} impl Rc { + #[inline(always)] + fn inner(&self) -> &RcBox { + // This unsafety is ok because while this Rc is alive we're guaranteed + // that the inner pointer is valid. + unsafe { self.ptr.as_ref() } + } + fn from_inner(ptr: NonNull>) -> Self { Self { ptr, phantom: PhantomData } } @@ -845,17 +852,10 @@ impl Rc { #[unstable(feature = "get_mut_unchecked", issue = "63292")] pub unsafe fn get_mut_unchecked(this: &mut Self) -> &mut T { // We are careful to *not* create a reference covering the "count" fields, as - // this would alias with concurrent access to the reference counts (e.g. by `Weak`). + // this would conflict with accesses to the reference counts (e.g. by `Weak`). unsafe { &mut (*this.ptr.as_ptr()).value } } - #[inline] - fn inner(&self) -> &RcBox { - // This unsafety is ok because while this Rc is alive we're guaranteed - // that the inner pointer is valid. - unsafe { self.ptr.as_ref() } - } - #[inline] #[stable(feature = "ptr_eq", since = "1.17.0")] /// Returns `true` if the two `Rc`s point to the same allocation @@ -2145,18 +2145,24 @@ trait RcInnerPtr { } impl RcInnerPtr for RcBox { + #[inline(always)] fn weak_ref(&self) -> &Cell { &self.weak } + + #[inline(always)] fn strong_ref(&self) -> &Cell { &self.strong } } impl<'a> RcInnerPtr for WeakInner<'a> { + #[inline(always)] fn weak_ref(&self) -> &Cell { self.weak } + + #[inline(always)] fn strong_ref(&self) -> &Cell { self.strong }