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

Deny unsafe ops in unsafe fns in libcore #73622

Merged
merged 7 commits into from
Jul 2, 2020
Merged
Show file tree
Hide file tree
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
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
3 changes: 2 additions & 1 deletion src/libcore/alloc/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
58 changes: 45 additions & 13 deletions src/libcore/alloc/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand All @@ -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) }
}
}
}
Expand Down Expand Up @@ -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)
}
}
}
}
Expand Down Expand Up @@ -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)
}
}
}
}
Expand All @@ -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]
Expand All @@ -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]
Expand All @@ -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
Expand Up @@ -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: () })
}
Expand Down
3 changes: 2 additions & 1 deletion src/libcore/char/convert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down
3 changes: 2 additions & 1 deletion src/libcore/char/methods.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down
3 changes: 2 additions & 1 deletion src/libcore/convert/num.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) }
}
}
)+
Expand Down
8 changes: 6 additions & 2 deletions src/libcore/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
}
}
Expand Down
4 changes: 3 additions & 1 deletion src/libcore/future/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Up @@ -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);
Expand Down
4 changes: 3 additions & 1 deletion src/libcore/hint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 10 additions & 3 deletions src/libcore/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Up @@ -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() },
}
}

Expand Down
15 changes: 10 additions & 5 deletions src/libcore/iter/adapters/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Expand All @@ -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]
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
3 changes: 2 additions & 1 deletion src/libcore/iter/adapters/zip.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading