From 6e2c49ff0e61e13aa3381eefba7923672a3c085f Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Fri, 29 Jun 2018 15:43:34 +0200 Subject: [PATCH 1/5] Use an aligned dangling pointer in Weak::new, rather than address 1 --- src/liballoc/sync.rs | 50 +++++++++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/src/liballoc/sync.rs b/src/liballoc/sync.rs index 4244b09b18f9c..abc0befeb947b 100644 --- a/src/liballoc/sync.rs +++ b/src/liballoc/sync.rs @@ -43,9 +43,6 @@ use vec::Vec; /// necessarily) at _exactly_ `MAX_REFCOUNT + 1` references. const MAX_REFCOUNT: usize = (isize::MAX) as usize; -/// A sentinel value that is used for the pointer of `Weak::new()`. -const WEAK_EMPTY: usize = 1; - /// A thread-safe reference-counting pointer. 'Arc' stands for 'Atomically /// Reference Counted'. /// @@ -239,9 +236,9 @@ impl, U: ?Sized> CoerceUnsized> for Arc {} #[stable(feature = "arc_weak", since = "1.4.0")] pub struct Weak { // This is a `NonNull` to allow optimizing the size of this type in enums, - // but it is actually not truly "non-null". A `Weak::new()` will set this - // to a sentinel value, instead of needing to allocate some space in the - // heap. + // but it is not necessarily a valid pointer. + // `Weak::new` sets this to a dangling pointer so that it doesn’t need + // to allocate space on the heap. ptr: NonNull>, } @@ -1035,14 +1032,18 @@ impl Weak { /// ``` #[stable(feature = "downgraded_weak", since = "1.10.0")] pub fn new() -> Weak { - unsafe { - Weak { - ptr: NonNull::new_unchecked(WEAK_EMPTY as *mut _), - } + Weak { + ptr: NonNull::dangling(), } } } +fn is_dangling(ptr: NonNull) -> bool { + let address = ptr.as_ptr() as *mut () as usize; + let align = align_of_val(unsafe { ptr.as_ref() }); + address == align +} + impl Weak { /// Attempts to upgrade the `Weak` pointer to an [`Arc`], extending /// the lifetime of the value if successful. @@ -1074,11 +1075,7 @@ impl Weak { pub fn upgrade(&self) -> Option> { // We use a CAS loop to increment the strong count instead of a // fetch_add because once the count hits 0 it must never be above 0. - let inner = if self.ptr.as_ptr() as *const u8 as usize == WEAK_EMPTY { - return None; - } else { - unsafe { self.ptr.as_ref() } - }; + let inner = self.inner()?; // Relaxed load because any write of 0 that we can observe // leaves the field in a permanently zero state (so a @@ -1109,6 +1106,17 @@ impl Weak { } } } + + /// Return `None` when the pointer is dangling and there is no allocated `ArcInner`, + /// i.e. this `Weak` was created by `Weak::new` + #[inline] + fn inner(&self) -> Option<&ArcInner> { + if is_dangling(self.ptr) { + None + } else { + Some(unsafe { self.ptr.as_ref() }) + } + } } #[stable(feature = "arc_weak", since = "1.4.0")] @@ -1126,10 +1134,10 @@ impl Clone for Weak { /// ``` #[inline] fn clone(&self) -> Weak { - let inner = if self.ptr.as_ptr() as *const u8 as usize == WEAK_EMPTY { - return Weak { ptr: self.ptr }; + let inner = if let Some(inner) = self.inner() { + inner } else { - unsafe { self.ptr.as_ref() } + return Weak { ptr: self.ptr }; }; // See comments in Arc::clone() for why this is relaxed. This can use a // fetch_add (ignoring the lock) because the weak count is only locked @@ -1204,10 +1212,10 @@ impl Drop for Weak { // weak count can only be locked if there was precisely one weak ref, // meaning that drop could only subsequently run ON that remaining weak // ref, which can only happen after the lock is released. - let inner = if self.ptr.as_ptr() as *const u8 as usize == WEAK_EMPTY { - return; + let inner = if let Some(inner) = self.inner() { + inner } else { - unsafe { self.ptr.as_ref() } + return }; if inner.weak.fetch_sub(1, Release) == 1 { From 41730b7e2e2c28a13fe6d08a7ad47d31d68eccea Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 27 Jun 2018 18:09:21 +0200 Subject: [PATCH 2/5] Rc: remove unused allocation from Weak::new() Same as https://github.com/rust-lang/rust/pull/50357 --- src/liballoc/rc.rs | 59 +++++++++++++++++++++++++++----------------- src/liballoc/sync.rs | 2 +- 2 files changed, 37 insertions(+), 24 deletions(-) diff --git a/src/liballoc/rc.rs b/src/liballoc/rc.rs index 32d624e8fbc79..3f32abe1ea959 100644 --- a/src/liballoc/rc.rs +++ b/src/liballoc/rc.rs @@ -253,7 +253,7 @@ use core::hash::{Hash, Hasher}; use core::intrinsics::abort; use core::marker; use core::marker::{Unsize, PhantomData}; -use core::mem::{self, align_of_val, forget, size_of_val, uninitialized}; +use core::mem::{self, align_of_val, forget, size_of_val}; use core::ops::Deref; use core::ops::CoerceUnsized; use core::ptr::{self, NonNull}; @@ -261,6 +261,7 @@ use core::convert::From; use alloc::{Global, Alloc, Layout, box_free, handle_alloc_error}; use string::String; +use sync::is_dangling; use vec::Vec; struct RcBox { @@ -1153,6 +1154,10 @@ impl From> for Rc<[T]> { /// [`None`]: ../../std/option/enum.Option.html#variant.None #[stable(feature = "rc_weak", since = "1.4.0")] pub struct Weak { + // This is a `NonNull` to allow optimizing the size of this type in enums, + // but it is not necessarily a valid pointer. + // `Weak::new` sets this to a dangling pointer so that it doesn’t need + // to allocate space on the heap. ptr: NonNull>, } @@ -1165,8 +1170,8 @@ impl !marker::Sync for Weak {} impl, U: ?Sized> CoerceUnsized> for Weak {} impl Weak { - /// Constructs a new `Weak`, allocating memory for `T` without initializing - /// it. Calling [`upgrade`] on the return value always gives [`None`]. + /// Constructs a new `Weak`, without allocating any memory. + /// Calling [`upgrade`] on the return value always gives [`None`]. /// /// [`upgrade`]: struct.Weak.html#method.upgrade /// [`None`]: ../../std/option/enum.Option.html @@ -1181,14 +1186,8 @@ impl Weak { /// ``` #[stable(feature = "downgraded_weak", since = "1.10.0")] pub fn new() -> Weak { - unsafe { - Weak { - ptr: Box::into_raw_non_null(box RcBox { - strong: Cell::new(0), - weak: Cell::new(1), - value: uninitialized(), - }), - } + Weak { + ptr: NonNull::dangling(), } } } @@ -1222,13 +1221,25 @@ impl Weak { /// ``` #[stable(feature = "rc_weak", since = "1.4.0")] pub fn upgrade(&self) -> Option> { - if self.strong() == 0 { + let inner = self.inner()?; + if inner.strong() == 0 { None } else { - self.inc_strong(); + inner.inc_strong(); Some(Rc { ptr: self.ptr, phantom: PhantomData }) } } + + /// Return `None` when the pointer is dangling and there is no allocated `RcBox`, + /// i.e. 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() }) + } + } } #[stable(feature = "rc_weak", since = "1.4.0")] @@ -1258,12 +1269,14 @@ impl Drop for Weak { /// assert!(other_weak_foo.upgrade().is_none()); /// ``` fn drop(&mut self) { - unsafe { - self.dec_weak(); + 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 self.weak() == 0 { - Global.dealloc(self.ptr.cast(), Layout::for_value(self.ptr.as_ref())); + if inner.weak() == 0 { + unsafe { + Global.dealloc(self.ptr.cast(), Layout::for_value(self.ptr.as_ref())); + } } } } @@ -1284,7 +1297,9 @@ impl Clone for Weak { /// ``` #[inline] fn clone(&self) -> Weak { - self.inc_weak(); + if let Some(inner) = self.inner() { + inner.inc_weak() + } Weak { ptr: self.ptr } } } @@ -1317,7 +1332,7 @@ impl Default for Weak { } } -// NOTE: We checked_add here to deal with mem::forget safety. In particular +// NOTE: We checked_add here to deal with mem::forget safely. In particular // if you mem::forget Rcs (or Weaks), the ref-count can overflow, and then // you can free the allocation while outstanding Rcs (or Weaks) exist. // We abort because this is such a degenerate scenario that we don't care about @@ -1370,12 +1385,10 @@ impl RcBoxPtr for Rc { } } -impl RcBoxPtr for Weak { +impl RcBoxPtr for RcBox { #[inline(always)] fn inner(&self) -> &RcBox { - unsafe { - self.ptr.as_ref() - } + self } } diff --git a/src/liballoc/sync.rs b/src/liballoc/sync.rs index abc0befeb947b..ea8616bf1d05c 100644 --- a/src/liballoc/sync.rs +++ b/src/liballoc/sync.rs @@ -1038,7 +1038,7 @@ impl Weak { } } -fn is_dangling(ptr: NonNull) -> bool { +pub(crate) fn is_dangling(ptr: NonNull) -> bool { let address = ptr.as_ptr() as *mut () as usize; let align = align_of_val(unsafe { ptr.as_ref() }); address == align From 21526c5403d7a43144de897d0187c395fb92bacc Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 27 Jun 2018 21:36:29 +0200 Subject: [PATCH 3/5] Add a test for Weak::new() not crashing for uninhabited types --- .../run-pass/weak-new-uninhabited-issue-48493.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 src/test/run-pass/weak-new-uninhabited-issue-48493.rs diff --git a/src/test/run-pass/weak-new-uninhabited-issue-48493.rs b/src/test/run-pass/weak-new-uninhabited-issue-48493.rs new file mode 100644 index 0000000000000..eb57c12ed12c6 --- /dev/null +++ b/src/test/run-pass/weak-new-uninhabited-issue-48493.rs @@ -0,0 +1,15 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +fn main() { + enum Void {} + std::rc::Weak::::new(); + std::sync::Weak::::new(); +} From 5717d99d1b3b76ec7814c95dfcc0399ab4ddaa83 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Fri, 6 Jul 2018 19:30:09 +0200 Subject: [PATCH 4/5] Add some unit tests for dangling Weak references --- src/liballoc/tests/arc.rs | 55 +++++++++++++++++++++++++++++++++++++++ src/liballoc/tests/lib.rs | 2 ++ src/liballoc/tests/rc.rs | 55 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+) create mode 100644 src/liballoc/tests/arc.rs create mode 100644 src/liballoc/tests/rc.rs diff --git a/src/liballoc/tests/arc.rs b/src/liballoc/tests/arc.rs new file mode 100644 index 0000000000000..753873dd294ce --- /dev/null +++ b/src/liballoc/tests/arc.rs @@ -0,0 +1,55 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use std::any::Any; +use std::sync::{Arc, Weak}; + +#[test] +fn uninhabited() { + enum Void {} + let mut a = Weak::::new(); + a = a.clone(); + assert!(a.upgrade().is_none()); + + let mut a: Weak = a; // Unsizing + a = a.clone(); + assert!(a.upgrade().is_none()); +} + +#[test] +fn slice() { + let a: Arc<[u32; 3]> = Arc::new([3, 2, 1]); + let a: Arc<[u32]> = a; // Unsizing + let b: Arc<[u32]> = Arc::from(&[3, 2, 1][..]); // Conversion + assert_eq!(a, b); + + // Exercise is_dangling() with a DST + let mut a = Arc::downgrade(&a); + a = a.clone(); + assert!(a.upgrade().is_some()); +} + +#[test] +fn trait_object() { + let a: Arc = Arc::new(4); + let a: Arc = a; // Unsizing + + // Exercise is_dangling() with a DST + let mut a = Arc::downgrade(&a); + a = a.clone(); + assert!(a.upgrade().is_some()); + + let mut b = Weak::::new(); + b = b.clone(); + assert!(b.upgrade().is_none()); + let mut b: Weak = b; // Unsizing + b = b.clone(); + assert!(b.upgrade().is_none()); +} diff --git a/src/liballoc/tests/lib.rs b/src/liballoc/tests/lib.rs index dbac2c0bb18a6..2c361598e8928 100644 --- a/src/liballoc/tests/lib.rs +++ b/src/liballoc/tests/lib.rs @@ -32,12 +32,14 @@ extern crate rand; use std::hash::{Hash, Hasher}; use std::collections::hash_map::DefaultHasher; +mod arc; mod binary_heap; mod btree; mod cow_str; mod fmt; mod heap; mod linked_list; +mod rc; mod slice; mod str; mod string; diff --git a/src/liballoc/tests/rc.rs b/src/liballoc/tests/rc.rs new file mode 100644 index 0000000000000..baa0406acfc3d --- /dev/null +++ b/src/liballoc/tests/rc.rs @@ -0,0 +1,55 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use std::any::Any; +use std::rc::{Rc, Weak}; + +#[test] +fn uninhabited() { + enum Void {} + let mut a = Weak::::new(); + a = a.clone(); + assert!(a.upgrade().is_none()); + + let mut a: Weak = a; // Unsizing + a = a.clone(); + assert!(a.upgrade().is_none()); +} + +#[test] +fn slice() { + let a: Rc<[u32; 3]> = Rc::new([3, 2, 1]); + let a: Rc<[u32]> = a; // Unsizing + let b: Rc<[u32]> = Rc::from(&[3, 2, 1][..]); // Conversion + assert_eq!(a, b); + + // Exercise is_dangling() with a DST + let mut a = Rc::downgrade(&a); + a = a.clone(); + assert!(a.upgrade().is_some()); +} + +#[test] +fn trait_object() { + let a: Rc = Rc::new(4); + let a: Rc = a; // Unsizing + + // Exercise is_dangling() with a DST + let mut a = Rc::downgrade(&a); + a = a.clone(); + assert!(a.upgrade().is_some()); + + let mut b = Weak::::new(); + b = b.clone(); + assert!(b.upgrade().is_none()); + let mut b: Weak = b; // Unsizing + b = b.clone(); + assert!(b.upgrade().is_none()); +} From 67202b8b68d93c90e89ca001945865f13bc97154 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Sat, 7 Jul 2018 01:44:57 +0200 Subject: [PATCH 5/5] =?UTF-8?q?Fix=20is=5Fdangling=20import=20when=20Arc?= =?UTF-8?q?=20is=20#[cfg]=E2=80=99ed=20out?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/liballoc/rc.rs | 7 ++++++- src/liballoc/sync.rs | 7 +------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/liballoc/rc.rs b/src/liballoc/rc.rs index 3f32abe1ea959..5b71d0b85a03e 100644 --- a/src/liballoc/rc.rs +++ b/src/liballoc/rc.rs @@ -261,7 +261,6 @@ use core::convert::From; use alloc::{Global, Alloc, Layout, box_free, handle_alloc_error}; use string::String; -use sync::is_dangling; use vec::Vec; struct RcBox { @@ -1192,6 +1191,12 @@ impl Weak { } } +pub(crate) fn is_dangling(ptr: NonNull) -> bool { + let address = ptr.as_ptr() as *mut () as usize; + let align = align_of_val(unsafe { ptr.as_ref() }); + address == align +} + impl Weak { /// Attempts to upgrade the `Weak` pointer to an [`Rc`], extending /// the lifetime of the value if successful. diff --git a/src/liballoc/sync.rs b/src/liballoc/sync.rs index ea8616bf1d05c..6710878b31d94 100644 --- a/src/liballoc/sync.rs +++ b/src/liballoc/sync.rs @@ -34,6 +34,7 @@ use core::convert::From; use alloc::{Global, Alloc, Layout, box_free, handle_alloc_error}; use boxed::Box; +use rc::is_dangling; use string::String; use vec::Vec; @@ -1038,12 +1039,6 @@ impl Weak { } } -pub(crate) fn is_dangling(ptr: NonNull) -> bool { - let address = ptr.as_ptr() as *mut () as usize; - let align = align_of_val(unsafe { ptr.as_ref() }); - address == align -} - impl Weak { /// Attempts to upgrade the `Weak` pointer to an [`Arc`], extending /// the lifetime of the value if successful.