From 2e2c38e59be7d0308c8a58c0843e7af8d7211ee7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Sun, 10 Oct 2021 21:02:42 +0300 Subject: [PATCH] Mark `Arc::from_inner` / `Rc::from_inner` as unsafe While it's an internal function, it is easy to create invalid Arc/Rcs to a dangling pointer with it. Fixes https://github.com/rust-lang/rust/issues/89740 --- library/alloc/src/rc.rs | 51 ++++++++++++++++++++++++--------------- library/alloc/src/sync.rs | 26 +++++++++++--------- 2 files changed, 45 insertions(+), 32 deletions(-) diff --git a/library/alloc/src/rc.rs b/library/alloc/src/rc.rs index 81e97805a72..eff20efccc8 100644 --- a/library/alloc/src/rc.rs +++ b/library/alloc/src/rc.rs @@ -333,12 +333,12 @@ fn inner(&self) -> &RcBox { unsafe { self.ptr.as_ref() } } - fn from_inner(ptr: NonNull>) -> Self { + unsafe fn from_inner(ptr: NonNull>) -> Self { Self { ptr, phantom: PhantomData } } unsafe fn from_ptr(ptr: *mut RcBox) -> Self { - Self::from_inner(unsafe { NonNull::new_unchecked(ptr) }) + unsafe { Self::from_inner(NonNull::new_unchecked(ptr)) } } } @@ -359,9 +359,11 @@ pub fn new(value: T) -> Rc { // pointers, which ensures that the weak destructor never frees // the allocation while the strong destructor is running, even // if the weak pointer is stored inside the strong one. - Self::from_inner( - Box::leak(box RcBox { strong: Cell::new(1), weak: Cell::new(1), value }).into(), - ) + unsafe { + Self::from_inner( + Box::leak(box RcBox { strong: Cell::new(1), weak: Cell::new(1), value }).into(), + ) + } } /// Constructs a new `Rc` using a weak reference to itself. Attempting @@ -412,16 +414,16 @@ pub fn new_cyclic(data_fn: impl FnOnce(&Weak) -> T) -> Rc { // otherwise. let data = data_fn(&weak); - unsafe { + let strong = unsafe { let inner = init_ptr.as_ptr(); ptr::write(ptr::addr_of_mut!((*inner).value), data); let prev_value = (*inner).strong.get(); debug_assert_eq!(prev_value, 0, "No prior strong references should exist"); (*inner).strong.set(1); - } - let strong = Rc::from_inner(init_ptr); + Rc::from_inner(init_ptr) + }; // Strong references should collectively own a shared weak reference, // so don't run the destructor for our old weak reference. @@ -511,10 +513,12 @@ pub fn try_new(value: T) -> Result, AllocError> { // pointers, which ensures that the weak destructor never frees // the allocation while the strong destructor is running, even // if the weak pointer is stored inside the strong one. - Ok(Self::from_inner( - Box::leak(Box::try_new(RcBox { strong: Cell::new(1), weak: Cell::new(1), value })?) - .into(), - )) + unsafe { + Ok(Self::from_inner( + Box::leak(Box::try_new(RcBox { strong: Cell::new(1), weak: Cell::new(1), value })?) + .into(), + )) + } } /// Constructs a new `Rc` with uninitialized contents, returning an error if the allocation fails @@ -733,7 +737,7 @@ impl Rc> { #[unstable(feature = "new_uninit", issue = "63291")] #[inline] pub unsafe fn assume_init(self) -> Rc { - Rc::from_inner(mem::ManuallyDrop::new(self).ptr.cast()) + unsafe { Rc::from_inner(mem::ManuallyDrop::new(self).ptr.cast()) } } } @@ -1199,9 +1203,11 @@ impl Rc { /// ``` pub fn downcast(self) -> Result, Rc> { if (*self).is::() { - let ptr = self.ptr.cast::>(); - forget(self); - Ok(Rc::from_inner(ptr)) + unsafe { + let ptr = self.ptr.cast::>(); + forget(self); + Ok(Rc::from_inner(ptr)) + } } else { Err(self) } @@ -1474,8 +1480,10 @@ impl Clone for Rc { /// ``` #[inline] fn clone(&self) -> Rc { - self.inner().inc_strong(); - Self::from_inner(self.ptr) + unsafe { + self.inner().inc_strong(); + Self::from_inner(self.ptr) + } } } @@ -2225,11 +2233,14 @@ pub unsafe fn from_raw(ptr: *const T) -> Self { #[stable(feature = "rc_weak", since = "1.4.0")] pub fn upgrade(&self) -> Option> { let inner = self.inner()?; + if inner.strong() == 0 { None } else { - inner.inc_strong(); - Some(Rc::from_inner(self.ptr)) + unsafe { + inner.inc_strong(); + Some(Rc::from_inner(self.ptr)) + } } } diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index 6e8da849e64..258557acdaa 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -252,7 +252,7 @@ impl, U: ?Sized> CoerceUnsized> for Arc {} impl, U: ?Sized> DispatchFromDyn> for Arc {} impl Arc { - fn from_inner(ptr: NonNull>) -> Self { + unsafe fn from_inner(ptr: NonNull>) -> Self { Self { ptr, phantom: PhantomData } } @@ -348,7 +348,7 @@ pub fn new(data: T) -> Arc { weak: atomic::AtomicUsize::new(1), data, }; - Self::from_inner(Box::leak(x).into()) + unsafe { Self::from_inner(Box::leak(x).into()) } } /// Constructs a new `Arc` using a weak reference to itself. Attempting @@ -397,7 +397,7 @@ pub fn new_cyclic(data_fn: impl FnOnce(&Weak) -> T) -> Arc { // Now we can properly initialize the inner value and turn our weak // reference into a strong reference. - unsafe { + let strong = unsafe { let inner = init_ptr.as_ptr(); ptr::write(ptr::addr_of_mut!((*inner).data), data); @@ -415,9 +415,9 @@ pub fn new_cyclic(data_fn: impl FnOnce(&Weak) -> T) -> Arc { // possible with safe code alone. let prev_value = (*inner).strong.fetch_add(1, Release); debug_assert_eq!(prev_value, 0, "No prior strong references should exist"); - } - let strong = Arc::from_inner(init_ptr); + Arc::from_inner(init_ptr) + }; // Strong references should collectively own a shared weak reference, // so don't run the destructor for our old weak reference. @@ -526,7 +526,7 @@ pub fn try_new(data: T) -> Result, AllocError> { weak: atomic::AtomicUsize::new(1), data, })?; - Ok(Self::from_inner(Box::leak(x).into())) + unsafe { Ok(Self::from_inner(Box::leak(x).into())) } } /// Constructs a new `Arc` with uninitialized contents, returning an error @@ -737,7 +737,7 @@ impl Arc> { #[unstable(feature = "new_uninit", issue = "63291")] #[inline] pub unsafe fn assume_init(self) -> Arc { - Arc::from_inner(mem::ManuallyDrop::new(self).ptr.cast()) + unsafe { Arc::from_inner(mem::ManuallyDrop::new(self).ptr.cast()) } } } @@ -1327,7 +1327,7 @@ fn clone(&self) -> Arc { abort(); } - Self::from_inner(self.ptr) + unsafe { Self::from_inner(self.ptr) } } } @@ -1654,9 +1654,11 @@ pub fn downcast(self) -> Result, Self> T: Any + Send + Sync + 'static, { if (*self).is::() { - let ptr = self.ptr.cast::>(); - mem::forget(self); - Ok(Arc::from_inner(ptr)) + unsafe { + let ptr = self.ptr.cast::>(); + mem::forget(self); + Ok(Arc::from_inner(ptr)) + } } else { Err(self) } @@ -1880,7 +1882,7 @@ pub fn upgrade(&self) -> Option> { // value can be initialized after `Weak` references have already been created. In that case, we // expect to observe the fully initialized value. match inner.strong.compare_exchange_weak(n, n + 1, Acquire, Relaxed) { - Ok(_) => return Some(Arc::from_inner(self.ptr)), // null checked above + Ok(_) => return Some(unsafe { Arc::from_inner(self.ptr) }), // null checked above Err(old) => n = old, } }