nordic-dev.net/rust
nordic-dev.net/rust
2
0
Fork 0
mirror of https://github.com/rust-lang/rust.git synced 2024-11-01 23:12:02 +00:00
Code Issues Packages Projects Releases Wiki Activity

Auto merge of #97027 - cuviper:yesalias-refcell, r=thomcc

Browse Source 
Use pointers in `cell::{Ref,RefMut}` to avoid `noalias`

When `Ref` and `RefMut` were based on references, they would get LLVM `noalias` attributes that were incorrect, because that alias guarantee is only true until the guard drops. A `&RefCell` on the same value can get a new borrow that aliases the previous guard, possibly leading to miscompilation. Using `NonNull` pointers in `Ref` and `RefCell` avoids `noalias`.

Fixes the library side of #63787, but we still might want to explore language solutions there.
This commit is contained in:
bors 2022-05-20 01:05:53 +00:00
commit 4d6992bc18
4 changed files with 103 additions and 38 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

83
library/core/src/cell.rs
View File

@ -194,10 +194,10 @@
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;
use crate::ptr::{self, NonNull};
/// A mutable memory location.
///
@ -896,7 +896,8 @@ impl<T: ?Sized> RefCell<T> {
// 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,
@ -980,8 +981,9 @@ impl<T: ?Sized> RefCell<T> {
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,
@ -1314,7 +1316,10 @@ 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.
// `NonNull` is also covariant over `T`, just like we would have with `&T`.
value: NonNull<T>,
borrow: BorrowRef<'b>,
}
@ -1324,7 +1329,8 @@ impl<T: ?Sized> 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 +1374,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 +1405,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 +1437,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 +1476,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() }
}
}
@ -1507,13 +1517,12 @@ impl<'b, T: ?Sized> RefMut<'b, T> {
/// ```
#[stable(feature = "cell_map", since = "1.8.0")]
#[inline]
pub fn map<U: ?Sized, F>(orig: RefMut<'b, T>, f: F) -> RefMut<'b, U>
pub fn map<U: ?Sized, F>(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
@ -1548,23 +1557,19 @@ impl<'b, T: ?Sized> RefMut<'b, T> {
/// ```
#[unstable(feature = "cell_filter_map", reason = "recently added", issue = "81061")]
#[inline]
pub fn filter_map<U: ?Sized, F>(orig: RefMut<'b, T>, f: F) -> Result<RefMut<'b, U>, Self>
pub fn filter_map<U: ?Sized, F>(mut orig: RefMut<'b, T>, f: F) -> Result<RefMut<'b, U>, 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),
}
}
@ -1596,15 +1601,18 @@ impl<'b, T: ?Sized> RefMut<'b, T> {
#[stable(feature = "refcell_map_split", since = "1.35.0")]
#[inline]
pub fn map_split<U: ?Sized, V: ?Sized, F>(
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.
@ -1630,14 +1638,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() }
}
}
@ -1692,8 +1701,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<T>,
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")]
@ -1702,7 +1715,8 @@ impl<T: ?Sized> 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() }
}
}
@ -1710,7 +1724,8 @@ impl<T: ?Sized> Deref for RefMut<'_, T> {
impl<T: ?Sized> 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() }
}
}

8
src/etc/natvis/libcore.natvis
View File

@ -7,15 +7,15 @@
</Expand>
</Type>
<Type Name="core::cell::Ref&lt;*&gt;">
<DisplayString>{value}</DisplayString>
<DisplayString>{value.pointer}</DisplayString>
<Expand>
<ExpandedItem>value</ExpandedItem>
<ExpandedItem>value.pointer</ExpandedItem>
</Expand>
</Type>
<Type Name="core::cell::RefMut&lt;*&gt;">
<DisplayString>{value}</DisplayString>
<DisplayString>{value.pointer}</DisplayString>
<Expand>
<ExpandedItem>value</ExpandedItem>
<ExpandedItem>value.pointer</ExpandedItem>
</Expand>
</Type>
<Type Name="core::cell::RefCell&lt;*&gt;">

14
src/test/codegen/noalias-refcell.rs Normal file
View File

@ -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<i32>) {}

36
src/test/ui/issues/issue-63787.rs Normal file
View File

@ -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<i32>, r: impl Deref<Target = i32>) -> 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);
}