Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make Cell::swap panic if the Cells partially overlap #114795

Merged
merged 3 commits into from
Aug 29, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions library/core/src/cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@

use crate::cmp::Ordering;
use crate::fmt::{self, Debug, Display};
use crate::intrinsics::is_nonoverlapping;
use crate::marker::{PhantomData, Unsize};
use crate::mem;
use crate::ops::{CoerceUnsized, Deref, DerefMut, DispatchFromDyn};
Expand Down Expand Up @@ -415,6 +416,12 @@ impl<T> Cell<T> {
/// Swaps the values of two `Cell`s.
/// Difference with `std::mem::swap` is that this function doesn't require `&mut` reference.
///
/// # Panics
///
/// This function will panic if `self` and `other` are different `Cell`s that partially overlap.
/// (Using just standard library methods, it is impossible to create such partially overlapping `Cell`s.
/// However, unsafe code is allowed to e.g. create two `&Cell<[i32; 2]>` that partially overlap.)
///
/// # Examples
///
/// ```
Expand All @@ -430,14 +437,20 @@ impl<T> Cell<T> {
#[stable(feature = "move_cell", since = "1.17.0")]
pub fn swap(&self, other: &Self) {
if ptr::eq(self, other) {
// Swapping wouldn't change anything.
return;
}
if !is_nonoverlapping(self, other, 1) {
// See <https://github.com/rust-lang/rust/issues/80778> for why we need to stop here.
panic!("`Cell::swap` on overlapping non-identical `Cell`s");
}
// SAFETY: This can be risky if called from separate threads, but `Cell`
// is `!Sync` so this won't happen. This also won't invalidate any
// pointers since `Cell` makes sure nothing else will be pointing into
// either of these `Cell`s.
// either of these `Cell`s. We also excluded shenanigans like partially overlapping `Cell`s,
// so `swap` will just properly copy two full values of type `T` back and forth.
unsafe {
ptr::swap(self.value.get(), other.value.get());
ptr::swap_nonoverlapping(self.value.get(), other.value.get(), 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pondering: would it be reasonable for this to mem::swap(&mut*self.value.get(), &mut*other.value.get()) now that they're non-overlapping? Or is it important that it stays in pointers here?

r=me either way from a code perspective.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use mem::swap, yeah. What would be the advantage of that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably doesn't make a substantial difference. I think of it as simpler since it doesn't need the loop (usually), but I suppose it'll still instantiate the ptr::swap_nonoverlapping code anyway.

}
}

Expand Down
2 changes: 1 addition & 1 deletion library/core/src/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2568,7 +2568,7 @@ pub(crate) fn is_nonoverlapping<T>(src: *const T, dst: *const T, count: usize) -
let size = mem::size_of::<T>()
.checked_mul(count)
.expect("is_nonoverlapping: `size_of::<T>() * count` overflows a usize");
let diff = if src_usize > dst_usize { src_usize - dst_usize } else { dst_usize - src_usize };
let diff = src_usize.abs_diff(dst_usize);
// If the absolute distance between the ptrs is at least as big as the size of the buffer,
// they do not overlap.
diff >= size
Expand Down
Loading