From d369045aed63ac8b9de1ed71679fac9bb4b0340a Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Fri, 13 May 2022 10:12:32 -0700 Subject: [PATCH 1/6] Use a pointer in cell::Ref so it's not noalias --- library/core/src/cell.rs | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/library/core/src/cell.rs b/library/core/src/cell.rs index 2a49017de3cc8..04bbef4844b6a 100644 --- a/library/core/src/cell.rs +++ b/library/core/src/cell.rs @@ -197,7 +197,7 @@ use crate::fmt::{self, Debug, Display}; use crate::marker::Unsize; use crate::mem; use crate::ops::{CoerceUnsized, Deref, DerefMut}; -use crate::ptr; +use crate::ptr::{self, NonNull}; /// A mutable memory location. /// @@ -896,7 +896,8 @@ impl RefCell { // SAFETY: `BorrowRef` ensures that there is only immutable access // to the value while borrowed. - Ok(Ref { value: unsafe { &*self.value.get() }, borrow: b }) + let value = unsafe { NonNull::new_unchecked(self.value.get()) }; + Ok(Ref { value, borrow: b }) } None => Err(BorrowError { // If a borrow occurred, then we must already have an outstanding borrow, @@ -1314,7 +1315,9 @@ impl Clone for BorrowRef<'_> { #[stable(feature = "rust1", since = "1.0.0")] #[must_not_suspend = "holding a Ref across suspend points can cause BorrowErrors"] pub struct Ref<'b, T: ?Sized + 'b> { - value: &'b T, + // NB: we use a pointer instead of `&'b T` to avoid `noalias` violations, because a + // `Ref` argument doesn't hold immutability for its whole scope, only until it drops. + value: NonNull, borrow: BorrowRef<'b>, } @@ -1324,7 +1327,8 @@ impl Deref for Ref<'_, T> { #[inline] fn deref(&self) -> &T { - self.value + // SAFETY: the value is accessible as long as we hold our borrow. + unsafe { self.value.as_ref() } } } @@ -1368,7 +1372,7 @@ impl<'b, T: ?Sized> Ref<'b, T> { where F: FnOnce(&T) -> &U, { - Ref { value: f(orig.value), borrow: orig.borrow } + Ref { value: NonNull::from(f(&*orig)), borrow: orig.borrow } } /// Makes a new `Ref` for an optional component of the borrowed data. The @@ -1399,8 +1403,8 @@ impl<'b, T: ?Sized> Ref<'b, T> { where F: FnOnce(&T) -> Option<&U>, { - match f(orig.value) { - Some(value) => Ok(Ref { value, borrow: orig.borrow }), + match f(&*orig) { + Some(value) => Ok(Ref { value: NonNull::from(value), borrow: orig.borrow }), None => Err(orig), } } @@ -1431,9 +1435,12 @@ impl<'b, T: ?Sized> Ref<'b, T> { where F: FnOnce(&T) -> (&U, &V), { - let (a, b) = f(orig.value); + let (a, b) = f(&*orig); let borrow = orig.borrow.clone(); - (Ref { value: a, borrow }, Ref { value: b, borrow: orig.borrow }) + ( + Ref { value: NonNull::from(a), borrow }, + Ref { value: NonNull::from(b), borrow: orig.borrow }, + ) } /// Convert into a reference to the underlying data. @@ -1467,7 +1474,8 @@ impl<'b, T: ?Sized> Ref<'b, T> { // unique reference to the borrowed RefCell. No further mutable references can be created // from the original cell. mem::forget(orig.borrow); - orig.value + // SAFETY: after forgetting, we can form a reference for the rest of lifetime `'b`. + unsafe { orig.value.as_ref() } } } From 2b8041f5746bdbd7c9f6ccf077544e1c77e927c0 Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Fri, 13 May 2022 10:46:00 -0700 Subject: [PATCH 2/6] Use a pointer in cell::RefMut so it's not noalias --- library/core/src/cell.rs | 52 +++++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/library/core/src/cell.rs b/library/core/src/cell.rs index 04bbef4844b6a..fa0206c349af6 100644 --- a/library/core/src/cell.rs +++ b/library/core/src/cell.rs @@ -194,7 +194,7 @@ use crate::cmp::Ordering; use crate::fmt::{self, Debug, Display}; -use crate::marker::Unsize; +use crate::marker::{PhantomData, Unsize}; use crate::mem; use crate::ops::{CoerceUnsized, Deref, DerefMut}; use crate::ptr::{self, NonNull}; @@ -981,8 +981,9 @@ impl RefCell { self.borrowed_at.set(Some(crate::panic::Location::caller())); } - // SAFETY: `BorrowRef` guarantees unique access. - Ok(RefMut { value: unsafe { &mut *self.value.get() }, borrow: b }) + // SAFETY: `BorrowRefMut` guarantees unique access. + let value = unsafe { NonNull::new_unchecked(self.value.get()) }; + Ok(RefMut { value, borrow: b, marker: PhantomData }) } None => Err(BorrowMutError { // If a borrow occurred, then we must already have an outstanding borrow, @@ -1515,13 +1516,13 @@ impl<'b, T: ?Sized> RefMut<'b, T> { /// ``` #[stable(feature = "cell_map", since = "1.8.0")] #[inline] - pub fn map(orig: RefMut<'b, T>, f: F) -> RefMut<'b, U> + pub fn map(mut orig: RefMut<'b, T>, f: F) -> RefMut<'b, U> where F: FnOnce(&mut T) -> &mut U, { // FIXME(nll-rfc#40): fix borrow-check - let RefMut { value, borrow } = orig; - RefMut { value: f(value), borrow } + let value = NonNull::from(f(&mut *orig)); + RefMut { value, borrow: orig.borrow, marker: PhantomData } } /// Makes a new `RefMut` for an optional component of the borrowed data. The @@ -1556,23 +1557,20 @@ impl<'b, T: ?Sized> RefMut<'b, T> { /// ``` #[unstable(feature = "cell_filter_map", reason = "recently added", issue = "81061")] #[inline] - pub fn filter_map(orig: RefMut<'b, T>, f: F) -> Result, Self> + pub fn filter_map(mut orig: RefMut<'b, T>, f: F) -> Result, Self> where F: FnOnce(&mut T) -> Option<&mut U>, { // FIXME(nll-rfc#40): fix borrow-check - let RefMut { value, borrow } = orig; - let value = value as *mut T; // SAFETY: function holds onto an exclusive reference for the duration // of its call through `orig`, and the pointer is only de-referenced // inside of the function call never allowing the exclusive reference to // escape. - match f(unsafe { &mut *value }) { - Some(value) => Ok(RefMut { value, borrow }), - None => { - // SAFETY: same as above. - Err(RefMut { value: unsafe { &mut *value }, borrow }) + match f(&mut *orig) { + Some(value) => { + Ok(RefMut { value: NonNull::from(value), borrow: orig.borrow, marker: PhantomData }) } + None => Err(orig), } } @@ -1604,15 +1602,18 @@ impl<'b, T: ?Sized> RefMut<'b, T> { #[stable(feature = "refcell_map_split", since = "1.35.0")] #[inline] pub fn map_split( - orig: RefMut<'b, T>, + mut orig: RefMut<'b, T>, f: F, ) -> (RefMut<'b, U>, RefMut<'b, V>) where F: FnOnce(&mut T) -> (&mut U, &mut V), { - let (a, b) = f(orig.value); let borrow = orig.borrow.clone(); - (RefMut { value: a, borrow }, RefMut { value: b, borrow: orig.borrow }) + let (a, b) = f(&mut *orig); + ( + RefMut { value: NonNull::from(a), borrow, marker: PhantomData }, + RefMut { value: NonNull::from(b), borrow: orig.borrow, marker: PhantomData }, + ) } /// Convert into a mutable reference to the underlying data. @@ -1638,14 +1639,15 @@ impl<'b, T: ?Sized> RefMut<'b, T> { /// assert!(cell.try_borrow_mut().is_err()); /// ``` #[unstable(feature = "cell_leak", issue = "69099")] - pub fn leak(orig: RefMut<'b, T>) -> &'b mut T { + pub fn leak(mut orig: RefMut<'b, T>) -> &'b mut T { // By forgetting this BorrowRefMut we ensure that the borrow counter in the RefCell can't // go back to UNUSED within the lifetime `'b`. Resetting the reference tracking state would // require a unique reference to the borrowed RefCell. No further references can be created // from the original cell within that lifetime, making the current borrow the only // reference for the remaining lifetime. mem::forget(orig.borrow); - orig.value + // SAFETY: after forgetting, we can form a reference for the rest of lifetime `'b`. + unsafe { orig.value.as_mut() } } } @@ -1700,8 +1702,12 @@ impl<'b> BorrowRefMut<'b> { #[stable(feature = "rust1", since = "1.0.0")] #[must_not_suspend = "holding a RefMut across suspend points can cause BorrowErrors"] pub struct RefMut<'b, T: ?Sized + 'b> { - value: &'b mut T, + // NB: we use a pointer instead of `&'b mut T` to avoid `noalias` violations, because a + // `RefMut` argument doesn't hold exclusivity for its whole scope, only until it drops. + value: NonNull, borrow: BorrowRefMut<'b>, + // NonNull is covariant over T, so we need to reintroduce invariance. + marker: PhantomData<&'b mut T>, } #[stable(feature = "rust1", since = "1.0.0")] @@ -1710,7 +1716,8 @@ impl Deref for RefMut<'_, T> { #[inline] fn deref(&self) -> &T { - self.value + // SAFETY: the value is accessible as long as we hold our borrow. + unsafe { self.value.as_ref() } } } @@ -1718,7 +1725,8 @@ impl Deref for RefMut<'_, T> { impl DerefMut for RefMut<'_, T> { #[inline] fn deref_mut(&mut self) -> &mut T { - self.value + // SAFETY: the value is accessible as long as we hold our borrow. + unsafe { self.value.as_mut() } } } From 15d8c008203208402bd234aec66b07dbe2824ec7 Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Fri, 13 May 2022 11:25:51 -0700 Subject: [PATCH 3/6] Test RefCell aliasing --- src/test/codegen/noalias-refcell.rs | 14 +++++++++++ src/test/ui/issues/issue-63787.rs | 36 +++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) create mode 100644 src/test/codegen/noalias-refcell.rs create mode 100644 src/test/ui/issues/issue-63787.rs diff --git a/src/test/codegen/noalias-refcell.rs b/src/test/codegen/noalias-refcell.rs new file mode 100644 index 0000000000000..dba73937abf17 --- /dev/null +++ b/src/test/codegen/noalias-refcell.rs @@ -0,0 +1,14 @@ +// compile-flags: -O -C no-prepopulate-passes -Z mutable-noalias=yes + +#![crate_type = "lib"] + +use std::cell::{Ref, RefCell, RefMut}; + +// Make sure that none of the arguments get a `noalias` attribute, because +// the `RefCell` might alias writes after either `Ref`/`RefMut` is dropped. + +// CHECK-LABEL: @maybe_aliased( +// CHECK-NOT: noalias +// CHECK-SAME: %_refcell +#[no_mangle] +pub unsafe fn maybe_aliased(_: Ref<'_, i32>, _: RefMut<'_, i32>, _refcell: &RefCell) {} diff --git a/src/test/ui/issues/issue-63787.rs b/src/test/ui/issues/issue-63787.rs new file mode 100644 index 0000000000000..cba079b231522 --- /dev/null +++ b/src/test/ui/issues/issue-63787.rs @@ -0,0 +1,36 @@ +// run-pass +// compile-flags: -O + +// Make sure that `Ref` and `RefMut` do not make false promises about aliasing, +// because once they drop, their reference/pointer can alias other writes. + +// Adapted from comex's proof of concept: +// https://github.com/rust-lang/rust/issues/63787#issuecomment-523588164 + +use std::cell::RefCell; +use std::ops::Deref; + +pub fn break_if_r_is_noalias(rc: &RefCell, r: impl Deref) -> i32 { + let ptr1 = &*r as *const i32; + let a = *r; + drop(r); + *rc.borrow_mut() = 2; + let r2 = rc.borrow(); + let ptr2 = &*r2 as *const i32; + if ptr2 != ptr1 { + panic!(); + } + // If LLVM knows the pointers are the same, and if `r` was `noalias`, + // then it may replace this with `a + a`, ignoring the earlier write. + a + *r2 +} + +fn main() { + let mut rc = RefCell::new(1); + let res = break_if_r_is_noalias(&rc, rc.borrow()); + assert_eq!(res, 3); + + *rc.get_mut() = 1; + let res = break_if_r_is_noalias(&rc, rc.borrow_mut()); + assert_eq!(res, 3); +} From 1e53fab55a0435be8bf4bf3ff9810ce2c73d5977 Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Mon, 16 May 2022 17:22:51 -0700 Subject: [PATCH 4/6] Remove outdated references to nll-rfc#40 --- library/core/src/cell.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/library/core/src/cell.rs b/library/core/src/cell.rs index fa0206c349af6..d43be569c5b5f 100644 --- a/library/core/src/cell.rs +++ b/library/core/src/cell.rs @@ -1520,7 +1520,6 @@ impl<'b, T: ?Sized> RefMut<'b, T> { where F: FnOnce(&mut T) -> &mut U, { - // FIXME(nll-rfc#40): fix borrow-check let value = NonNull::from(f(&mut *orig)); RefMut { value, borrow: orig.borrow, marker: PhantomData } } @@ -1561,7 +1560,6 @@ impl<'b, T: ?Sized> RefMut<'b, T> { where F: FnOnce(&mut T) -> Option<&mut U>, { - // FIXME(nll-rfc#40): fix borrow-check // SAFETY: function holds onto an exclusive reference for the duration // of its call through `orig`, and the pointer is only de-referenced // inside of the function call never allowing the exclusive reference to From 1f33c921d1e030c1d293b774df079996bcfb2f68 Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Mon, 16 May 2022 17:24:53 -0700 Subject: [PATCH 5/6] Add a comment for covariant `Ref` --- library/core/src/cell.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/core/src/cell.rs b/library/core/src/cell.rs index d43be569c5b5f..5448ced803a09 100644 --- a/library/core/src/cell.rs +++ b/library/core/src/cell.rs @@ -1318,6 +1318,7 @@ impl Clone for BorrowRef<'_> { pub struct Ref<'b, T: ?Sized + 'b> { // NB: we use a pointer instead of `&'b T` to avoid `noalias` violations, because a // `Ref` argument doesn't hold immutability for its whole scope, only until it drops. + // `NonNull` is also covariant over `T`, just like we would have with `&T`. value: NonNull, borrow: BorrowRef<'b>, } @@ -1704,7 +1705,7 @@ pub struct RefMut<'b, T: ?Sized + 'b> { // `RefMut` argument doesn't hold exclusivity for its whole scope, only until it drops. value: NonNull, borrow: BorrowRefMut<'b>, - // NonNull is covariant over T, so we need to reintroduce invariance. + // `NonNull` is covariant over `T`, so we need to reintroduce invariance. marker: PhantomData<&'b mut T>, } From 1c3921fa43ecc6438a1f4d5365d2f99caad7b847 Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Mon, 16 May 2022 17:39:34 -0700 Subject: [PATCH 6/6] Read the Ref/RefMut pointer in natvis --- src/etc/natvis/libcore.natvis | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/etc/natvis/libcore.natvis b/src/etc/natvis/libcore.natvis index 643590fc97787..a4e8a57e4b16d 100644 --- a/src/etc/natvis/libcore.natvis +++ b/src/etc/natvis/libcore.natvis @@ -7,15 +7,15 @@ - {value} + {value.pointer} - value + value.pointer - {value} + {value.pointer} - value + value.pointer