Skip to content

Deny unsafe ops in unsafe fns in libcore #73622

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 7 commits into from
Jul 2, 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
22 changes: 16 additions & 6 deletions src/libcore/alloc/global.rs
Original file line number Diff line number Diff line change
@@ -127,9 +127,12 @@ pub unsafe trait GlobalAlloc {
#[stable(feature = "global_alloc", since = "1.28.0")]
unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
let size = layout.size();
let ptr = self.alloc(layout);
// SAFETY: the safety contract for `alloc` must be upheld by the caller.
let ptr = unsafe { self.alloc(layout) };
if !ptr.is_null() {
ptr::write_bytes(ptr, 0, size);
// SAFETY: as allocation succeeded, the region from `ptr`
// of size `size` is guaranteed to be valid for writes.
unsafe { ptr::write_bytes(ptr, 0, size) };
Copy link
Member

Choose a reason for hiding this comment

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

This is actually a great example where we do still do local unsafety reasoning inside an unsafe fn, instead of just forwarding requirements to the caller.

}
ptr
}
@@ -187,11 +190,18 @@ pub unsafe trait GlobalAlloc {
/// [`handle_alloc_error`]: ../../alloc/alloc/fn.handle_alloc_error.html
#[stable(feature = "global_alloc", since = "1.28.0")]
unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 {
let new_layout = Layout::from_size_align_unchecked(new_size, layout.align());
let new_ptr = self.alloc(new_layout);
// SAFETY: the caller must ensure that the `new_size` does not overflow.
// `layout.align()` comes from a `Layout` and is thus guaranteed to be valid.
let new_layout = unsafe { Layout::from_size_align_unchecked(new_size, layout.align()) };
// SAFETY: the caller must ensure that `new_layout` is greater than zero.
let new_ptr = unsafe { self.alloc(new_layout) };
if !new_ptr.is_null() {
ptr::copy_nonoverlapping(ptr, new_ptr, cmp::min(layout.size(), new_size));
self.dealloc(ptr, layout);
// SAFETY: the previously allocated block cannot overlap the newly allocated block.
// The safety contract for `dealloc` must be upheld by the caller.
unsafe {
ptr::copy_nonoverlapping(ptr, new_ptr, cmp::min(layout.size(), new_size));
self.dealloc(ptr, layout);
}
}
new_ptr
}
3 changes: 2 additions & 1 deletion src/libcore/alloc/layout.rs
Original file line number Diff line number Diff line change
@@ -90,7 +90,8 @@ impl Layout {
#[rustc_const_stable(feature = "alloc_layout", since = "1.28.0")]
#[inline]
pub const unsafe fn from_size_align_unchecked(size: usize, align: usize) -> Self {
Layout { size_: size, align_: NonZeroUsize::new_unchecked(align) }
// SAFETY: the caller must ensure that `align` is greater than zero.
Layout { size_: size, align_: unsafe { NonZeroUsize::new_unchecked(align) } }
}

/// The minimum size in bytes for a memory block of this layout.
58 changes: 45 additions & 13 deletions src/libcore/alloc/mod.rs
Original file line number Diff line number Diff line change
@@ -54,7 +54,9 @@ impl AllocInit {
#[inline]
#[unstable(feature = "allocator_api", issue = "32838")]
pub unsafe fn init(self, memory: MemoryBlock) {
self.init_offset(memory, 0)
// SAFETY: the safety contract for `init_offset` must be
// upheld by the caller.
unsafe { self.init_offset(memory, 0) }
}

/// Initialize the memory block like specified by `init` at the specified `offset`.
@@ -78,7 +80,10 @@ impl AllocInit {
match self {
AllocInit::Uninitialized => (),
AllocInit::Zeroed => {
memory.ptr.as_ptr().add(offset).write_bytes(0, memory.size - offset)
// SAFETY: the caller must guarantee that `offset` is smaller than or equal to `memory.size`,
// so the memory from `memory.ptr + offset` of length `memory.size - offset`
// is guaranteed to be contaned in `memory` and thus valid for writes.
unsafe { memory.ptr.as_ptr().add(offset).write_bytes(0, memory.size - offset) }
}
}
}
@@ -281,11 +286,23 @@ pub unsafe trait AllocRef {
return Ok(MemoryBlock { ptr, size });
}

let new_layout = Layout::from_size_align_unchecked(new_size, layout.align());
let new_layout =
// SAFETY: the caller must ensure that the `new_size` does not overflow.
// `layout.align()` comes from a `Layout` and is thus guaranteed to be valid for a Layout.
// The caller must ensure that `new_size` is greater than zero.
unsafe { Layout::from_size_align_unchecked(new_size, layout.align()) };
let new_memory = self.alloc(new_layout, init)?;
ptr::copy_nonoverlapping(ptr.as_ptr(), new_memory.ptr.as_ptr(), size);
self.dealloc(ptr, layout);
Ok(new_memory)

// SAFETY: because `new_size` must be greater than or equal to `size`, both the old and new
// memory allocation are valid for reads and writes for `size` bytes. Also, because the old
// allocation wasn't yet deallocated, it cannot overlap `new_memory`. Thus, the call to
// `copy_nonoverlapping` is safe.
// The safety contract for `dealloc` must be upheld by the caller.
unsafe {
ptr::copy_nonoverlapping(ptr.as_ptr(), new_memory.ptr.as_ptr(), size);
self.dealloc(ptr, layout);
Ok(new_memory)
}
}
}
}
@@ -356,11 +373,23 @@ pub unsafe trait AllocRef {
return Ok(MemoryBlock { ptr, size });
}

let new_layout = Layout::from_size_align_unchecked(new_size, layout.align());
let new_layout =
// SAFETY: the caller must ensure that the `new_size` does not overflow.
// `layout.align()` comes from a `Layout` and is thus guaranteed to be valid for a Layout.
// The caller must ensure that `new_size` is greater than zero.
unsafe { Layout::from_size_align_unchecked(new_size, layout.align()) };
let new_memory = self.alloc(new_layout, AllocInit::Uninitialized)?;
ptr::copy_nonoverlapping(ptr.as_ptr(), new_memory.ptr.as_ptr(), new_size);
self.dealloc(ptr, layout);
Ok(new_memory)

// SAFETY: because `new_size` must be lower than or equal to `size`, both the old and new
// memory allocation are valid for reads and writes for `new_size` bytes. Also, because the
// old allocation wasn't yet deallocated, it cannot overlap `new_memory`. Thus, the call to
// `copy_nonoverlapping` is safe.
// The safety contract for `dealloc` must be upheld by the caller.
unsafe {
ptr::copy_nonoverlapping(ptr.as_ptr(), new_memory.ptr.as_ptr(), new_size);
self.dealloc(ptr, layout);
Ok(new_memory)
}
}
}
}
@@ -386,7 +415,8 @@ where

#[inline]
unsafe fn dealloc(&mut self, ptr: NonNull<u8>, layout: Layout) {
(**self).dealloc(ptr, layout)
// SAFETY: the safety contract must be upheld by the caller
unsafe { (**self).dealloc(ptr, layout) }
}

#[inline]
@@ -398,7 +428,8 @@ where
placement: ReallocPlacement,
init: AllocInit,
) -> Result<MemoryBlock, AllocErr> {
(**self).grow(ptr, layout, new_size, placement, init)
// SAFETY: the safety contract must be upheld by the caller
unsafe { (**self).grow(ptr, layout, new_size, placement, init) }
}

#[inline]
@@ -409,6 +440,7 @@ where
new_size: usize,
placement: ReallocPlacement,
) -> Result<MemoryBlock, AllocErr> {
(**self).shrink(ptr, layout, new_size, placement)
// SAFETY: the safety contract must be upheld by the caller
unsafe { (**self).shrink(ptr, layout, new_size, placement) }
}
}
7 changes: 6 additions & 1 deletion src/libcore/cell.rs
Original file line number Diff line number Diff line change
@@ -1005,7 +1005,12 @@ impl<T: ?Sized> RefCell<T> {
#[inline]
pub unsafe fn try_borrow_unguarded(&self) -> Result<&T, BorrowError> {
if !is_writing(self.borrow.get()) {
Ok(&*self.value.get())
// SAFETY: We check that nobody is actively writing now, but it is
// the caller's responsibility to ensure that nobody writes until
// the returned reference is no longer in use.
// Also, `self.value.get()` refers to the value owned by `self`
// and is thus guaranteed to be valid for the lifetime of `self`.
Ok(unsafe { &*self.value.get() })
} else {
Err(BorrowError { _private: () })
}
3 changes: 2 additions & 1 deletion src/libcore/char/convert.rs
Original file line number Diff line number Diff line change
@@ -99,7 +99,8 @@ pub fn from_u32(i: u32) -> Option<char> {
#[inline]
#[stable(feature = "char_from_unchecked", since = "1.5.0")]
pub unsafe fn from_u32_unchecked(i: u32) -> char {
if cfg!(debug_assertions) { char::from_u32(i).unwrap() } else { transmute(i) }
// SAFETY: the caller must guarantee that `i` is a valid char value.
if cfg!(debug_assertions) { char::from_u32(i).unwrap() } else { unsafe { transmute(i) } }
}

#[stable(feature = "char_convert", since = "1.13.0")]
3 changes: 2 additions & 1 deletion src/libcore/char/methods.rs
Original file line number Diff line number Diff line change
@@ -183,7 +183,8 @@ impl char {
#[unstable(feature = "assoc_char_funcs", reason = "recently added", issue = "71763")]
#[inline]
pub unsafe fn from_u32_unchecked(i: u32) -> char {
super::convert::from_u32_unchecked(i)
// SAFETY: the safety contract must be upheld by the caller.
unsafe { super::convert::from_u32_unchecked(i) }
}

/// Converts a digit in the given radix to a `char`.
3 changes: 2 additions & 1 deletion src/libcore/convert/num.rs
Original file line number Diff line number Diff line change
@@ -28,7 +28,8 @@ macro_rules! impl_float_to_int {
#[doc(hidden)]
#[inline]
unsafe fn to_int_unchecked(self) -> $Int {
crate::intrinsics::float_to_int_unchecked(self)
// SAFETY: the safety contract must be upheld by the caller.
unsafe { crate::intrinsics::float_to_int_unchecked(self) }
}
}
)+
8 changes: 6 additions & 2 deletions src/libcore/ffi.rs
Original file line number Diff line number Diff line change
@@ -333,7 +333,8 @@ impl<'f> VaListImpl<'f> {
/// Advance to the next arg.
#[inline]
pub unsafe fn arg<T: sealed_trait::VaArgSafe>(&mut self) -> T {
va_arg(self)
// SAFETY: the caller must uphold the safety contract for `va_arg`.
unsafe { va_arg(self) }
}

/// Copies the `va_list` at the current location.
@@ -343,7 +344,10 @@ impl<'f> VaListImpl<'f> {
{
let mut ap = self.clone();
let ret = f(ap.as_va_list());
va_end(&mut ap);
// SAFETY: the caller must uphold the safety contract for `va_end`.
unsafe {
va_end(&mut ap);
}
ret
}
}
4 changes: 3 additions & 1 deletion src/libcore/future/mod.rs
Original file line number Diff line number Diff line change
@@ -85,5 +85,7 @@ where
#[unstable(feature = "gen_future", issue = "50547")]
#[inline]
pub unsafe fn get_context<'a, 'b>(cx: ResumeTy) -> &'a mut Context<'b> {
&mut *cx.0.as_ptr().cast()
// SAFETY: the caller must guarantee that `cx.0` is a valid pointer
// that fulfills all the requirements for a mutable reference.
unsafe { &mut *cx.0.as_ptr().cast() }
}
10 changes: 7 additions & 3 deletions src/libcore/hash/sip.rs
Original file line number Diff line number Diff line change
@@ -130,15 +130,19 @@ unsafe fn u8to64_le(buf: &[u8], start: usize, len: usize) -> u64 {
let mut i = 0; // current byte index (from LSB) in the output u64
let mut out = 0;
if i + 3 < len {
out = load_int_le!(buf, start + i, u32) as u64;
// SAFETY: `i` cannot be greater than `len`, and the caller must guarantee
// that the index start..start+len is in bounds.
out = unsafe { load_int_le!(buf, start + i, u32) } as u64;
i += 4;
}
if i + 1 < len {
out |= (load_int_le!(buf, start + i, u16) as u64) << (i * 8);
// SAFETY: same as above.
out |= (unsafe { load_int_le!(buf, start + i, u16) } as u64) << (i * 8);
i += 2
}
if i < len {
out |= (*buf.get_unchecked(start + i) as u64) << (i * 8);
// SAFETY: same as above.
out |= (unsafe { *buf.get_unchecked(start + i) } as u64) << (i * 8);
i += 1;
}
debug_assert_eq!(i, len);
4 changes: 3 additions & 1 deletion src/libcore/hint.rs
Original file line number Diff line number Diff line change
@@ -46,7 +46,9 @@ use crate::intrinsics;
#[inline]
#[stable(feature = "unreachable", since = "1.27.0")]
pub unsafe fn unreachable_unchecked() -> ! {
intrinsics::unreachable()
// SAFETY: the safety contract for `intrinsics::unreachable` must
// be upheld by the caller.
unsafe { intrinsics::unreachable() }
}

/// Emits a machine instruction hinting to the processor that it is running in busy-wait
13 changes: 10 additions & 3 deletions src/libcore/intrinsics.rs
Original file line number Diff line number Diff line change
@@ -2097,7 +2097,10 @@ pub unsafe fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: usize) {
// Not panicking to keep codegen impact smaller.
abort();
}
copy_nonoverlapping(src, dst, count)

// SAFETY: the safety contract for `copy_nonoverlapping` must be
// upheld by the caller.
unsafe { copy_nonoverlapping(src, dst, count) }
}

/// Copies `count * size_of::<T>()` bytes from `src` to `dst`. The source
@@ -2163,7 +2166,9 @@ pub unsafe fn copy<T>(src: *const T, dst: *mut T, count: usize) {
// Not panicking to keep codegen impact smaller.
abort();
}
copy(src, dst, count)

// SAFETY: the safety contract for `copy` must be upheld by the caller.
unsafe { copy(src, dst, count) }
}

/// Sets `count * size_of::<T>()` bytes of memory starting at `dst` to
@@ -2246,5 +2251,7 @@ pub unsafe fn write_bytes<T>(dst: *mut T, val: u8, count: usize) {
}

debug_assert!(is_aligned_and_not_null(dst), "attempt to write to unaligned or null pointer");
write_bytes(dst, val, count)

// SAFETY: the safety contract for `write_bytes` must be upheld by the caller.
unsafe { write_bytes(dst, val, count) }
}
5 changes: 3 additions & 2 deletions src/libcore/iter/adapters/fuse.rs
Original file line number Diff line number Diff line change
@@ -178,9 +178,10 @@ where
{
unsafe fn get_unchecked(&mut self, i: usize) -> I::Item {
match self.iter {
Some(ref mut iter) => iter.get_unchecked(i),
// SAFETY: the caller must uphold the contract for `TrustedRandomAccess::get_unchecked`.
Some(ref mut iter) => unsafe { iter.get_unchecked(i) },
// SAFETY: the caller asserts there is an item at `i`, so we're not exhausted.
None => intrinsics::unreachable(),
None => unsafe { intrinsics::unreachable() },
}
}

15 changes: 10 additions & 5 deletions src/libcore/iter/adapters/mod.rs
Original file line number Diff line number Diff line change
@@ -272,7 +272,8 @@ where
T: Copy,
{
unsafe fn get_unchecked(&mut self, i: usize) -> Self::Item {
*self.it.get_unchecked(i)
// SAFETY: the caller must uphold the contract for `TrustedRandomAccess::get_unchecked`.
unsafe { *self.it.get_unchecked(i) }
}

#[inline]
@@ -402,7 +403,8 @@ where
T: Clone,
{
default unsafe fn get_unchecked(&mut self, i: usize) -> Self::Item {
self.it.get_unchecked(i).clone()
// SAFETY: the caller must uphold the contract for `TrustedRandomAccess::get_unchecked`.
unsafe { self.it.get_unchecked(i) }.clone()
}

#[inline]
@@ -418,7 +420,8 @@ where
T: Copy,
{
unsafe fn get_unchecked(&mut self, i: usize) -> Self::Item {
*self.it.get_unchecked(i)
// SAFETY: the caller must uphold the contract for `TrustedRandomAccess::get_unchecked`.
unsafe { *self.it.get_unchecked(i) }
}

#[inline]
@@ -930,7 +933,8 @@ where
F: FnMut(I::Item) -> B,
{
unsafe fn get_unchecked(&mut self, i: usize) -> Self::Item {
(self.f)(self.iter.get_unchecked(i))
// SAFETY: the caller must uphold the contract for `TrustedRandomAccess::get_unchecked`.
(self.f)(unsafe { self.iter.get_unchecked(i) })
}
#[inline]
fn may_have_side_effect() -> bool {
@@ -1392,7 +1396,8 @@ where
I: TrustedRandomAccess,
{
unsafe fn get_unchecked(&mut self, i: usize) -> (usize, I::Item) {
(self.count + i, self.iter.get_unchecked(i))
// SAFETY: the caller must uphold the contract for `TrustedRandomAccess::get_unchecked`.
(self.count + i, unsafe { self.iter.get_unchecked(i) })
}

fn may_have_side_effect() -> bool {
3 changes: 2 additions & 1 deletion src/libcore/iter/adapters/zip.rs
Original file line number Diff line number Diff line change
@@ -271,7 +271,8 @@ where
B: TrustedRandomAccess,
{
unsafe fn get_unchecked(&mut self, i: usize) -> (A::Item, B::Item) {
(self.a.get_unchecked(i), self.b.get_unchecked(i))
// SAFETY: the caller must uphold the contract for `TrustedRandomAccess::get_unchecked`.
unsafe { (self.a.get_unchecked(i), self.b.get_unchecked(i)) }
}

fn may_have_side_effect() -> bool {
Loading