Skip to content

Document unsafety in core::ptr #71507

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

Merged
merged 2 commits into from
Apr 29, 2020
Merged
Changes from all 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
27 changes: 23 additions & 4 deletions src/libcore/ptr/mod.rs
Original file line number Diff line number Diff line change
@@ -65,8 +65,6 @@
//! [`write_volatile`]: ./fn.write_volatile.html
//! [`NonNull::dangling`]: ./struct.NonNull.html#method.dangling

// ignore-tidy-undocumented-unsafe

#![stable(feature = "rust1", since = "1.0.0")]

use crate::cmp::Ordering;
@@ -248,14 +246,17 @@ pub(crate) struct FatPtr<T> {
///
/// // create a slice pointer when starting out with a pointer to the first element
/// let x = [5, 6, 7];
/// let ptr = x.as_ptr();
/// let slice = ptr::slice_from_raw_parts(ptr, 3);
/// let raw_pointer = x.as_ptr();
/// let slice = ptr::slice_from_raw_parts(raw_pointer, 3);
/// assert_eq!(unsafe { &*slice }[2], 7);
/// ```
#[inline]
#[stable(feature = "slice_from_raw_parts", since = "1.42.0")]
#[rustc_const_unstable(feature = "const_slice_from_raw_parts", issue = "67456")]
pub const fn slice_from_raw_parts<T>(data: *const T, len: usize) -> *const [T] {
// SAFETY: Accessing the value from the `Repr` union is safe since *const [T]
// and FatPtr have the same memory layouts. Only std can make this
// guarantee.
unsafe { Repr { raw: FatPtr { data, len } }.rust }
}

@@ -269,10 +270,28 @@ pub const fn slice_from_raw_parts<T>(data: *const T, len: usize) -> *const [T] {
///
/// [`slice_from_raw_parts`]: fn.slice_from_raw_parts.html
/// [`from_raw_parts_mut`]: ../../std/slice/fn.from_raw_parts_mut.html
///
/// # Examples
///
/// ```rust
/// use std::ptr;
///
/// let x = &mut [5, 6, 7];
/// let raw_pointer = x.as_mut_ptr();
/// let slice = ptr::slice_from_raw_parts_mut(raw_pointer, 3);
///
/// unsafe {
/// (*slice)[2] = 99; // assign a value at an index in the slice
/// };
///
/// assert_eq!(unsafe { &*slice }[2], 99);
/// ```
#[inline]
#[stable(feature = "slice_from_raw_parts", since = "1.42.0")]
#[rustc_const_unstable(feature = "const_slice_from_raw_parts", issue = "67456")]
pub const fn slice_from_raw_parts_mut<T>(data: *mut T, len: usize) -> *mut [T] {
// SAFETY: Accessing the value from the `Repr` union is safe since *mut [T]
// and FatPtr have the same memory layouts
unsafe { Repr { raw: FatPtr { data, len } }.rust_mut }
}

18 changes: 15 additions & 3 deletions src/libcore/ptr/non_null.rs
Original file line number Diff line number Diff line change
@@ -7,8 +7,6 @@ use crate::mem;
use crate::ops::{CoerceUnsized, DispatchFromDyn};
use crate::ptr::Unique;

// ignore-tidy-undocumented-unsafe

/// `*mut T` but non-zero and covariant.
///
/// This is often the correct thing to use when building data structures using
@@ -69,6 +67,9 @@ impl<T: Sized> NonNull<T> {
#[rustc_const_stable(feature = "const_nonnull_dangling", since = "1.32.0")]
#[inline]
pub const fn dangling() -> Self {
// SAFETY: mem::align_of() returns a non-zero usize which is then casted
// to a *mut T. Therefore, `ptr` is not null and the conditions for
// calling new_unchecked() are respected.
unsafe {
let ptr = mem::align_of::<T>() as *mut T;
NonNull::new_unchecked(ptr)
@@ -93,7 +94,12 @@ impl<T: ?Sized> NonNull<T> {
#[stable(feature = "nonnull", since = "1.25.0")]
#[inline]
pub fn new(ptr: *mut T) -> Option<Self> {
if !ptr.is_null() { Some(unsafe { Self::new_unchecked(ptr) }) } else { None }
if !ptr.is_null() {
// SAFETY: The pointer is already checked and is not null
Some(unsafe { Self::new_unchecked(ptr) })
} else {
None
}
}

/// Acquires the underlying `*mut` pointer.
@@ -131,6 +137,7 @@ impl<T: ?Sized> NonNull<T> {
#[rustc_const_stable(feature = "const_nonnull_cast", since = "1.32.0")]
#[inline]
pub const fn cast<U>(self) -> NonNull<U> {
// SAFETY: `self` is a `NonNull` pointer which is necessarily non-null
unsafe { NonNull::new_unchecked(self.as_ptr() as *mut U) }
}
}
@@ -205,6 +212,8 @@ impl<T: ?Sized> hash::Hash for NonNull<T> {
impl<T: ?Sized> From<Unique<T>> for NonNull<T> {
#[inline]
fn from(unique: Unique<T>) -> Self {
// SAFETY: A Unique pointer cannot be null, so the conditions for
// new_unchecked() are respected.
unsafe { NonNull::new_unchecked(unique.as_ptr()) }
}
}
@@ -213,6 +222,7 @@ impl<T: ?Sized> From<Unique<T>> for NonNull<T> {
impl<T: ?Sized> From<&mut T> for NonNull<T> {
#[inline]
fn from(reference: &mut T) -> Self {
// SAFETY: A mutable reference cannot be null.
unsafe { NonNull { pointer: reference as *mut T } }
Copy link
Contributor

Choose a reason for hiding this comment

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

You refer to new_unchecked in the SAFETY comment, but it's not what's called here. We should always be using new_unchecked to create NonNulls, even internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ecstatic-morse Do you think I should add it to this PR ? I feel like changing the code is a bit outside of its scope. I'm changing the SAFETY comment

}
}
@@ -221,6 +231,8 @@ impl<T: ?Sized> From<&mut T> for NonNull<T> {
impl<T: ?Sized> From<&T> for NonNull<T> {
#[inline]
fn from(reference: &T) -> Self {
// SAFETY: A reference cannot be null, so the conditions for
// new_unchecked() are respected.
unsafe { NonNull { pointer: reference as *const T } }
}
}
7 changes: 7 additions & 0 deletions src/libcore/ptr/unique.rs
Original file line number Diff line number Diff line change
@@ -74,6 +74,8 @@ impl<T: Sized> Unique<T> {
// FIXME: rename to dangling() to match NonNull?
#[inline]
pub const fn empty() -> Self {
// SAFETY: mem::align_of() returns a valid, non-null pointer. The
// conditions to call new_unchecked() are thus respected.
unsafe { Unique::new_unchecked(mem::align_of::<T>() as *mut T) }
}
}
@@ -94,6 +96,7 @@ impl<T: ?Sized> Unique<T> {
#[inline]
pub fn new(ptr: *mut T) -> Option<Self> {
if !ptr.is_null() {
// SAFETY: The pointer has already been checked and is not null.
Some(unsafe { Unique { pointer: ptr as _, _marker: PhantomData } })
} else {
None
@@ -129,6 +132,9 @@ impl<T: ?Sized> Unique<T> {
/// Casts to a pointer of another type.
#[inline]
pub const fn cast<U>(self) -> Unique<U> {
// SAFETY: Unique::new_unchecked() creates a new unique and needs
// the given pointer to not be null.
// Since we are passing self as a pointer, it cannot be null.
unsafe { Unique::new_unchecked(self.as_ptr() as *mut U) }
}
}
@@ -168,6 +174,7 @@ impl<T: ?Sized> fmt::Pointer for Unique<T> {
impl<T: ?Sized> From<&mut T> for Unique<T> {
#[inline]
fn from(reference: &mut T) -> Self {
// SAFETY: A mutable reference cannot be null
unsafe { Unique { pointer: reference as *mut T, _marker: PhantomData } }
}
}