From 6c5637ca17701e6d18401ffd259defe41a814c72 Mon Sep 17 00:00:00 2001 From: Imbris Date: Sat, 4 Feb 2023 02:56:15 -0500 Subject: [PATCH 1/4] Add black_box to fetching bench --- benches/bench.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/benches/bench.rs b/benches/bench.rs index 60f193b8..f6855578 100644 --- a/benches/bench.rs +++ b/benches/bench.rs @@ -232,9 +232,9 @@ fn bench_fetching(b: &mut Bencher) { b.iter(|| { for _ in 0..100 { - world.fetch::(); - world.fetch::>(); - world.fetch::>(); + black_box(world.fetch::()); + black_box(world.fetch::>()); + black_box(world.fetch::>()); } }) } From ee5deb57359277620fb8108e66676ca3c20c013a Mon Sep 17 00:00:00 2001 From: Imbris Date: Sat, 4 Feb 2023 01:00:21 -0500 Subject: [PATCH 2/4] Add Miri to CI --- .github/workflows/ci.yml | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d6117444..1e50dd9d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -25,7 +25,7 @@ jobs: timeout-minutes: 10 steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - uses: actions-rs/toolchain@v1 with: profile: minimal @@ -58,3 +58,17 @@ jobs: with: command: test args: --verbose + + miri: + name: "Miri" + runs-on: ubuntu-latest + continue-on-error: true # Needed until all Miri errors are fixed + steps: + - uses: actions/checkout@v3 + - name: Install Miri + run: | + rustup toolchain install nightly --component miri + rustup override set nightly + cargo miri setup + - name: Test with Miri + run: cargo miri test From bcbc2f2ea50ece02595bf71c4877376d7ba4d0b2 Mon Sep 17 00:00:00 2001 From: Imbris Date: Sat, 4 Feb 2023 23:32:01 -0500 Subject: [PATCH 3/4] Also require `T: Send` on `Sync` impl of `TrustCell` since it provides `&mut T` which can be used to move `T` between threads! --- src/cell.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cell.rs b/src/cell.rs index 3345f9f7..7558e410 100644 --- a/src/cell.rs +++ b/src/cell.rs @@ -351,7 +351,7 @@ impl TrustCell { } } -unsafe impl Sync for TrustCell where T: Sync {} +unsafe impl Sync for TrustCell where T: Sync + Send {} impl Default for TrustCell where From fcbf43e9b47d0a074d815536a8dc6c22ab65a470 Mon Sep 17 00:00:00 2001 From: Imbris Date: Sun, 5 Feb 2023 00:06:23 -0500 Subject: [PATCH 4/4] Replace use of references in `Ref` and `RefMut` with `NonNull` to address stacked borrows UB detected by Miri. This is similar to https://github.com/bholley/atomic_refcell/pull/18 --- src/cell.rs | 103 +++++++++++++++++++++++++++++++++++----------------- 1 file changed, 70 insertions(+), 33 deletions(-) diff --git a/src/cell.rs b/src/cell.rs index 7558e410..f0c93c10 100644 --- a/src/cell.rs +++ b/src/cell.rs @@ -1,5 +1,7 @@ //! Helper module for some internals, most users don't need to interact with it. +use core::marker::PhantomData; +use core::ptr::NonNull; use std::{ cell::UnsafeCell, error::Error, @@ -42,7 +44,7 @@ impl Error for InvalidBorrow { #[derive(Debug)] pub struct Ref<'a, T: ?Sized + 'a> { flag: &'a AtomicUsize, - value: &'a T, + value: NonNull, } impl<'a, T: ?Sized> Ref<'a, T> { @@ -95,7 +97,7 @@ impl<'a, T: ?Sized> Ref<'a, T> { { let val = Ref { flag: self.flag, - value: f(self.value), + value: NonNull::from(f(&*self)), }; std::mem::forget(self); @@ -108,7 +110,8 @@ impl<'a, T: ?Sized> Deref for Ref<'a, T> { type Target = T; fn deref(&self) -> &T { - self.value + // SAFETY: `Ref` holds a shared borrow of the value. + unsafe { self.value.as_ref() } } } @@ -134,7 +137,10 @@ impl<'a, T: ?Sized> Clone for Ref<'a, T> { #[derive(Debug)] pub struct RefMut<'a, T: ?Sized + 'a> { flag: &'a AtomicUsize, - value: &'a mut T, + value: NonNull, + // `NonNull` is covariant over `T` but we use it in the place of a mutable reference so we need to be + // invariant over `T`. + marker: PhantomData<&'a mut T>, } impl<'a, T: ?Sized> RefMut<'a, T> { @@ -182,7 +188,7 @@ impl<'a, T: ?Sized> RefMut<'a, T> { /// let b2: RefMut<'_, u32> = RefMut::map(b1, |t| &mut t.0); /// assert_eq!(*b2, 5); /// ``` - pub fn map(self, f: F) -> RefMut<'a, U> + pub fn map(mut self, f: F) -> RefMut<'a, U> where F: FnOnce(&mut T) -> &mut U, U: ?Sized, @@ -192,7 +198,7 @@ impl<'a, T: ?Sized> RefMut<'a, T> { // the given `RefMut`, the lifetime we created through turning the // pointer into a ref is valid. let flag = self.flag; - let value = unsafe { &mut *(self.value as *mut _) }; + let value = NonNull::from(f(&mut *self)); // We have to forget self so that we do not run `Drop`. Further it's safe // because we are creating a new `RefMut`, with the same flag, which @@ -201,7 +207,8 @@ impl<'a, T: ?Sized> RefMut<'a, T> { RefMut { flag, - value: f(value), + value, + marker: PhantomData, } } } @@ -210,13 +217,17 @@ impl<'a, T: ?Sized> Deref for RefMut<'a, T> { type Target = T; fn deref(&self) -> &T { - self.value + // SAFETY: `RefMut` holds an exclusive borrow of the value and we have a shared borrow of + // `RefMut` here. + unsafe { self.value.as_ref() } } } impl<'a, T: ?Sized> DerefMut for RefMut<'a, T> { fn deref_mut(&mut self) -> &mut T { - self.value + // SAFETY: `RefMut` holds an exclusive borrow of the value and we have an exclusive borrow + // of `RefMut` here. + unsafe { self.value.as_mut() } } } @@ -261,7 +272,7 @@ impl TrustCell { Ref { flag: &self.flag, - value: unsafe { &*self.inner.get() }, + value: unsafe { NonNull::new_unchecked(self.inner.get()) }, } } @@ -274,7 +285,7 @@ impl TrustCell { Ok(Ref { flag: &self.flag, - value: unsafe { &*self.inner.get() }, + value: unsafe { NonNull::new_unchecked(self.inner.get()) }, }) } @@ -292,7 +303,8 @@ impl TrustCell { RefMut { flag: &self.flag, - value: unsafe { &mut *self.inner.get() }, + value: unsafe { NonNull::new_unchecked(self.inner.get()) }, + marker: PhantomData, } } @@ -305,7 +317,8 @@ impl TrustCell { Ok(RefMut { flag: &self.flag, - value: unsafe { &mut *self.inner.get() }, + value: unsafe { NonNull::new_unchecked(self.inner.get()) }, + marker: PhantomData, }) } @@ -471,22 +484,27 @@ mod tests { assert!(cell.try_borrow_mut().is_err()); } + fn make_ref<'a, T: ?Sized>(flag: &'a AtomicUsize, value: &'a T) -> Ref<'a, T> { + Ref { + flag, + value: NonNull::from(value), + } + } + #[test] fn ref_with_non_sized() { - let r: Ref<'_, [i32]> = Ref { - flag: &AtomicUsize::new(1), - value: &[2, 3, 4, 5][..], - }; + let value = &[2, 3, 4, 5][..]; + let flag = AtomicUsize::new(1); + let r: Ref<'_, [i32]> = make_ref(&flag, value); assert_eq!(&*r, &[2, 3, 4, 5][..]); } #[test] fn ref_with_non_sized_clone() { - let r: Ref<'_, [i32]> = Ref { - flag: &AtomicUsize::new(1), - value: &[2, 3, 4, 5][..], - }; + let value = &[2, 3, 4, 5][..]; + let flag = AtomicUsize::new(1); + let r: Ref<'_, [i32]> = make_ref(&flag, value); let rr = r.clone(); assert_eq!(&*r, &[2, 3, 4, 5][..]); @@ -498,30 +516,35 @@ mod tests { #[test] fn ref_with_trait_obj() { - let ra: Ref<'_, dyn std::any::Any> = Ref { - flag: &AtomicUsize::new(1), - value: &2i32, - }; + let value = &2i32; + let flag = AtomicUsize::new(1); + let ra: Ref<'_, dyn std::any::Any> = make_ref(&flag, value); assert_eq!(ra.downcast_ref::().unwrap(), &2i32); } + fn make_ref_mut<'a, T: ?Sized>(flag: &'a AtomicUsize, value: &'a mut T) -> RefMut<'a, T> { + RefMut { + flag, + value: NonNull::from(value), + marker: PhantomData, + } + } + #[test] fn ref_mut_with_non_sized() { - let mut r: RefMut<'_, [i32]> = RefMut { - flag: &AtomicUsize::new(1), - value: &mut [2, 3, 4, 5][..], - }; + let value: &mut [i32] = &mut [2, 3, 4, 5][..]; + let flag = AtomicUsize::new(1); + let mut r: RefMut<'_, [i32]> = make_ref_mut(&flag, value); assert_eq!(&mut *r, &mut [2, 3, 4, 5][..]); } #[test] fn ref_mut_with_trait_obj() { - let mut ra: RefMut<'_, dyn std::any::Any> = RefMut { - flag: &AtomicUsize::new(1), - value: &mut 2i32, - }; + let value = &mut 2i32; + let flag = AtomicUsize::new(1); + let mut ra: RefMut<'_, dyn std::any::Any> = make_ref_mut(&flag, value); assert_eq!(ra.downcast_mut::().unwrap(), &mut 2i32); } @@ -613,4 +636,18 @@ mod tests { drop(r); assert_eq!(cell.flag.load(Ordering::SeqCst), 0); } + + // Test intended to allow Miri to catch bugs like this scenario: + // https://github.com/rust-lang/rust/issues/63787 + #[test] + fn drop_and_borrow_in_fn_call() { + fn drop_and_borrow(cell: &TrustCell, borrow: Ref<'_, u8>) { + drop(borrow); + *cell.borrow_mut() = 7u8; + } + + let a = TrustCell::new(4u8); + let borrow = a.borrow(); + drop_and_borrow(&a, borrow); + } }