From fd9b2ed48ff71511aac0d232e1448baa536b15f9 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Thu, 27 Sep 2018 09:01:37 +1000 Subject: [PATCH 1/9] fix a soundness hole allowing multiple mutable borrows --- src/heap_lazy.rs | 22 +++++++++++----------- src/inline_lazy.rs | 15 +++++++++------ 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/src/heap_lazy.rs b/src/heap_lazy.rs index c67a301..4a42763 100644 --- a/src/heap_lazy.rs +++ b/src/heap_lazy.rs @@ -8,27 +8,27 @@ extern crate std; use self::std::prelude::v1::*; +use self::std::cell::Cell; use self::std::sync::Once; pub use self::std::sync::ONCE_INIT; -pub struct Lazy(*const T, Once); +pub struct Lazy(Cell<*const T>, Once); impl Lazy { - pub const INIT: Self = Lazy(0 as *const T, ONCE_INIT); + pub const INIT: Self = Lazy(Cell::new(0 as *const T), ONCE_INIT); #[inline(always)] - pub fn get(&'static mut self, f: F) -> &T + pub fn get(&'static self, f: F) -> &T where F: FnOnce() -> T, { - unsafe { - let r = &mut self.0; - self.1.call_once(|| { - *r = Box::into_raw(Box::new(f())); - }); - - &*self.0 - } + self.1.call_once(|| { + self.0.set(Box::into_raw(Box::new(f()))); + }); + + // `self.0` is guaranteed to have a value by this point + // The `Once` will catch and propegate panics + unsafe { &*self.0.get() } } } diff --git a/src/inline_lazy.rs b/src/inline_lazy.rs index 201a3c0..63cca8b 100644 --- a/src/inline_lazy.rs +++ b/src/inline_lazy.rs @@ -9,27 +9,30 @@ extern crate core; extern crate std; use self::std::prelude::v1::*; +use self::std::cell::Cell; use self::std::sync::Once; pub use self::std::sync::ONCE_INIT; -pub struct Lazy(Option, Once); +pub struct Lazy(Cell>, Once); impl Lazy { - pub const INIT: Self = Lazy(None, ONCE_INIT); + pub const INIT: Self = Lazy(Cell::new(None), ONCE_INIT); #[inline(always)] - pub fn get(&'static mut self, f: F) -> &T + pub fn get(&'static self, f: F) -> &T where F: FnOnce() -> T, { { - let r = &mut self.0; self.1.call_once(|| { - *r = Some(f()); + self.0.set(Some(f())); }); } + + // `self.0` is guaranteed to be `Some` by this point + // The `Once` will catch and propegate panics unsafe { - match self.0 { + match *self.0.as_ptr() { Some(ref x) => x, None => unreachable_unchecked(), } From e33326e920dadd9c541ca7d6ba6f133a72b45710 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Thu, 27 Sep 2018 09:07:34 +1000 Subject: [PATCH 2/9] remove redundant braces --- src/inline_lazy.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/inline_lazy.rs b/src/inline_lazy.rs index 63cca8b..203d6c7 100644 --- a/src/inline_lazy.rs +++ b/src/inline_lazy.rs @@ -23,11 +23,9 @@ impl Lazy { where F: FnOnce() -> T, { - { - self.1.call_once(|| { - self.0.set(Some(f())); - }); - } + self.1.call_once(|| { + self.0.set(Some(f())); + }); // `self.0` is guaranteed to be `Some` by this point // The `Once` will catch and propegate panics From 70611a92ef78538af22efb5f1bdd5d623a6260a6 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Thu, 27 Sep 2018 09:43:02 +1000 Subject: [PATCH 3/9] make heap lazy build without const fns This means manually setting the value in an unsafe way, because we can't use `Cell` with the `const INIT` value. --- src/heap_lazy.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/heap_lazy.rs b/src/heap_lazy.rs index 4a42763..8a5a481 100644 --- a/src/heap_lazy.rs +++ b/src/heap_lazy.rs @@ -8,14 +8,14 @@ extern crate std; use self::std::prelude::v1::*; -use self::std::cell::Cell; +use self::std::ptr; use self::std::sync::Once; pub use self::std::sync::ONCE_INIT; -pub struct Lazy(Cell<*const T>, Once); +pub struct Lazy(*const T, Once); impl Lazy { - pub const INIT: Self = Lazy(Cell::new(0 as *const T), ONCE_INIT); + pub const INIT: Self = Lazy(0 as *const T, ONCE_INIT); #[inline(always)] pub fn get(&'static self, f: F) -> &T @@ -23,12 +23,19 @@ impl Lazy { F: FnOnce() -> T, { self.1.call_once(|| { - self.0.set(Box::into_raw(Box::new(f()))); + let r = Box::into_raw(Box::new(f())); + + // This is safe because the `Once` guarantees we + // have exclusive access to the value field + // The value hasn't been previously initialized + unsafe { + ptr::write(&self.0 as *const _ as *mut _, r); + } }); // `self.0` is guaranteed to have a value by this point // The `Once` will catch and propegate panics - unsafe { &*self.0.get() } + unsafe { &*self.0 } } } From 551986e9ff1f7fa388a13fc4d5e77cac77f94c56 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Sat, 29 Sep 2018 09:31:11 +1000 Subject: [PATCH 4/9] Revert "make heap lazy build without const fns" This reverts commit 70611a92ef78538af22efb5f1bdd5d623a6260a6. --- src/heap_lazy.rs | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/heap_lazy.rs b/src/heap_lazy.rs index 8a5a481..4a42763 100644 --- a/src/heap_lazy.rs +++ b/src/heap_lazy.rs @@ -8,14 +8,14 @@ extern crate std; use self::std::prelude::v1::*; -use self::std::ptr; +use self::std::cell::Cell; use self::std::sync::Once; pub use self::std::sync::ONCE_INIT; -pub struct Lazy(*const T, Once); +pub struct Lazy(Cell<*const T>, Once); impl Lazy { - pub const INIT: Self = Lazy(0 as *const T, ONCE_INIT); + pub const INIT: Self = Lazy(Cell::new(0 as *const T), ONCE_INIT); #[inline(always)] pub fn get(&'static self, f: F) -> &T @@ -23,19 +23,12 @@ impl Lazy { F: FnOnce() -> T, { self.1.call_once(|| { - let r = Box::into_raw(Box::new(f())); - - // This is safe because the `Once` guarantees we - // have exclusive access to the value field - // The value hasn't been previously initialized - unsafe { - ptr::write(&self.0 as *const _ as *mut _, r); - } + self.0.set(Box::into_raw(Box::new(f()))); }); // `self.0` is guaranteed to have a value by this point // The `Once` will catch and propegate panics - unsafe { &*self.0 } + unsafe { &*self.0.get() } } } From 0eb1595b4d1f8daa4f57b544044945fb2e430fb7 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Sat, 29 Sep 2018 09:31:54 +1000 Subject: [PATCH 5/9] bump min version to 1.24.1 --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 820eec3..fc1e88e 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,7 +1,7 @@ language: rust matrix: include: - - rust: 1.21.0 + - rust: 1.24.1 - rust: stable - os: osx - rust: beta From c192db550da9a06d5e4223f0cdfdb827c644e699 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Sat, 29 Sep 2018 09:40:12 +1000 Subject: [PATCH 6/9] remove static mut from macros --- src/heap_lazy.rs | 2 +- src/inline_lazy.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/heap_lazy.rs b/src/heap_lazy.rs index 4a42763..9f0c92d 100644 --- a/src/heap_lazy.rs +++ b/src/heap_lazy.rs @@ -38,6 +38,6 @@ unsafe impl Sync for Lazy {} #[doc(hidden)] macro_rules! __lazy_static_create { ($NAME:ident, $T:ty) => { - static mut $NAME: $crate::lazy::Lazy<$T> = $crate::lazy::Lazy::INIT; + static $NAME: $crate::lazy::Lazy<$T> = $crate::lazy::Lazy::INIT; }; } diff --git a/src/inline_lazy.rs b/src/inline_lazy.rs index 203d6c7..4db8cf3 100644 --- a/src/inline_lazy.rs +++ b/src/inline_lazy.rs @@ -44,7 +44,7 @@ unsafe impl Sync for Lazy {} #[doc(hidden)] macro_rules! __lazy_static_create { ($NAME:ident, $T:ty) => { - static mut $NAME: $crate::lazy::Lazy<$T> = $crate::lazy::Lazy::INIT; + static $NAME: $crate::lazy::Lazy<$T> = $crate::lazy::Lazy::INIT; }; } From 9b7bfcfab1d63505dcecaac25cd8debed98f6cf9 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Tue, 30 Oct 2018 11:07:50 +1000 Subject: [PATCH 7/9] leave a debug_assert in the supposedly unreachable block --- src/inline_lazy.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/inline_lazy.rs b/src/inline_lazy.rs index 4db8cf3..e8dc2ef 100644 --- a/src/inline_lazy.rs +++ b/src/inline_lazy.rs @@ -32,7 +32,11 @@ impl Lazy { unsafe { match *self.0.as_ptr() { Some(ref x) => x, - None => unreachable_unchecked(), + None => { + debug_assert!(false, "attempted to derefence an uninitialized lazy static. This is a bug"); + + unreachable_unchecked() + }, } } } From f3a5f4a50b99389d9160ae169c5a41bbefde31e5 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Tue, 30 Oct 2018 15:13:16 +1000 Subject: [PATCH 8/9] remove redundant unsafe block --- src/lib.rs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index a01dd4b..b58e334 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -137,19 +137,16 @@ macro_rules! __lazy_static_internal { (@TAIL, $N:ident : $T:ty = $e:expr) => { impl $crate::__Deref for $N { type Target = $T; - #[allow(unsafe_code)] fn deref(&self) -> &$T { - unsafe { - #[inline(always)] - fn __static_ref_initialize() -> $T { $e } - - #[inline(always)] - unsafe fn __stability() -> &'static $T { - __lazy_static_create!(LAZY, $T); - LAZY.get(__static_ref_initialize) - } - __stability() + #[inline(always)] + fn __static_ref_initialize() -> $T { $e } + + #[inline(always)] + fn __stability() -> &'static $T { + __lazy_static_create!(LAZY, $T); + LAZY.get(__static_ref_initialize) } + __stability() } } impl $crate::LazyStatic for $N { From c039eeb88045bc0b4bfa3bfa9a350f22b75965e2 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Thu, 1 Nov 2018 07:50:44 +1000 Subject: [PATCH 9/9] add a FIXME to replace Option with MaybeInitialized --- src/inline_lazy.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/inline_lazy.rs b/src/inline_lazy.rs index e8dc2ef..268dd45 100644 --- a/src/inline_lazy.rs +++ b/src/inline_lazy.rs @@ -13,6 +13,7 @@ use self::std::cell::Cell; use self::std::sync::Once; pub use self::std::sync::ONCE_INIT; +// FIXME: Replace Option with MaybeInitialized pub struct Lazy(Cell>, Once); impl Lazy {