Skip to content

Commit

Permalink
Rollup merge of #127744 - workingjubilee:deny-unsafe-op-in-std, r=jhp…
Browse files Browse the repository at this point in the history
…ratt

std: `#![deny(unsafe_op_in_unsafe_fn)]` in platform-independent code

This applies the `unsafe_op_in_unsafe_fn` lint in all places in std that _do not have platform-specific cfg in their code_. For all such places, the lint remains allowed, because they need further work to address the relevant concerns. This list includes:

- `std::backtrace_rs` (internal-only)
- `std::sys` (internal-only)
- `std::os`

Notably this eliminates all "unwrapped" unsafe operations in `std::io` and `std::sync`, which will make them much more auditable in the future. Such has *also* been left for future work. While I made a few safety comments along the way on interfaces I have grown sufficiently familiar with, in most cases I had no context, nor particular confidence the unsafety was correct.

In the cases where I was able to determine the unsafety was correct without having prior context, it was obviously redundant. For example, an unsafe function calling another unsafe function that has the exact same contract, forwarding its caller's requirements just as it forwards its actual call.
  • Loading branch information
workingjubilee committed Jul 15, 2024
2 parents 8f91841 + e324602 commit a6b4ff6
Show file tree
Hide file tree
Showing 18 changed files with 89 additions and 66 deletions.
2 changes: 1 addition & 1 deletion library/std/src/collections/hash/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1018,7 +1018,7 @@ where
K: Borrow<Q>,
Q: Hash + Eq,
{
self.base.get_many_unchecked_mut(ks)
unsafe { self.base.get_many_unchecked_mut(ks) }
}

/// Returns `true` if the map contains a value for the specified key.
Expand Down
14 changes: 4 additions & 10 deletions library/std/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -366,11 +366,8 @@ impl Error for VarError {
#[rustc_deprecated_safe_2024]
#[stable(feature = "env", since = "1.0.0")]
pub unsafe fn set_var<K: AsRef<OsStr>, V: AsRef<OsStr>>(key: K, value: V) {
_set_var(key.as_ref(), value.as_ref())
}

unsafe fn _set_var(key: &OsStr, value: &OsStr) {
os_imp::setenv(key, value).unwrap_or_else(|e| {
let (key, value) = (key.as_ref(), value.as_ref());
unsafe { os_imp::setenv(key, value) }.unwrap_or_else(|e| {
panic!("failed to set environment variable `{key:?}` to `{value:?}`: {e}")
})
}
Expand Down Expand Up @@ -433,11 +430,8 @@ unsafe fn _set_var(key: &OsStr, value: &OsStr) {
#[rustc_deprecated_safe_2024]
#[stable(feature = "env", since = "1.0.0")]
pub unsafe fn remove_var<K: AsRef<OsStr>>(key: K) {
_remove_var(key.as_ref())
}

unsafe fn _remove_var(key: &OsStr) {
os_imp::unsetenv(key)
let key = key.as_ref();
unsafe { os_imp::unsetenv(key) }
.unwrap_or_else(|e| panic!("failed to remove environment variable `{key:?}`: {e}"))
}

Expand Down
4 changes: 2 additions & 2 deletions library/std/src/ffi/os_str.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ impl OsString {
#[inline]
#[stable(feature = "os_str_bytes", since = "1.74.0")]
pub unsafe fn from_encoded_bytes_unchecked(bytes: Vec<u8>) -> Self {
OsString { inner: Buf::from_encoded_bytes_unchecked(bytes) }
OsString { inner: unsafe { Buf::from_encoded_bytes_unchecked(bytes) } }
}

/// Converts to an [`OsStr`] slice.
Expand Down Expand Up @@ -813,7 +813,7 @@ impl OsStr {
#[inline]
#[stable(feature = "os_str_bytes", since = "1.74.0")]
pub unsafe fn from_encoded_bytes_unchecked(bytes: &[u8]) -> &Self {
Self::from_inner(Slice::from_encoded_bytes_unchecked(bytes))
Self::from_inner(unsafe { Slice::from_encoded_bytes_unchecked(bytes) })
}

#[inline]
Expand Down
8 changes: 5 additions & 3 deletions library/std/src/io/buffered/bufwriter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -433,9 +433,11 @@ impl<W: ?Sized + Write> BufWriter<W> {
let old_len = self.buf.len();
let buf_len = buf.len();
let src = buf.as_ptr();
let dst = self.buf.as_mut_ptr().add(old_len);
ptr::copy_nonoverlapping(src, dst, buf_len);
self.buf.set_len(old_len + buf_len);
unsafe {
let dst = self.buf.as_mut_ptr().add(old_len);
ptr::copy_nonoverlapping(src, dst, buf_len);
self.buf.set_len(old_len + buf_len);
}
}

#[inline]
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/io/cursor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ where
A: Allocator,
{
debug_assert!(vec.capacity() >= pos + buf.len());
vec.as_mut_ptr().add(pos).copy_from(buf.as_ptr(), buf.len());
unsafe { vec.as_mut_ptr().add(pos).copy_from(buf.as_ptr(), buf.len()) };
pos + buf.len()
}

Expand Down
7 changes: 5 additions & 2 deletions library/std/src/io/error/repr_bitpacked.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,11 +267,14 @@ where
// Using this rather than unwrap meaningfully improves the code
// for callers which only care about one variant (usually
// `Custom`)
core::hint::unreachable_unchecked();
unsafe { core::hint::unreachable_unchecked() };
});
ErrorData::Simple(kind)
}
TAG_SIMPLE_MESSAGE => ErrorData::SimpleMessage(&*ptr.cast::<SimpleMessage>().as_ptr()),
TAG_SIMPLE_MESSAGE => {
// SAFETY: per tag
unsafe { ErrorData::SimpleMessage(&*ptr.cast::<SimpleMessage>().as_ptr()) }
}
TAG_CUSTOM => {
// It would be correct for us to use `ptr::byte_sub` here (see the
// comment above the `wrapping_add` call in `new_custom` for why),
Expand Down
4 changes: 2 additions & 2 deletions library/std/src/io/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -382,11 +382,11 @@ pub(crate) unsafe fn append_to_string<F>(buf: &mut String, f: F) -> Result<usize
where
F: FnOnce(&mut Vec<u8>) -> Result<usize>,
{
let mut g = Guard { len: buf.len(), buf: buf.as_mut_vec() };
let mut g = Guard { len: buf.len(), buf: unsafe { buf.as_mut_vec() } };
let ret = f(g.buf);

// SAFETY: the caller promises to only append data to `buf`
let appended = g.buf.get_unchecked(g.len..);
let appended = unsafe { g.buf.get_unchecked(g.len..) };
if str::from_utf8(appended).is_err() {
ret.and_then(|_| Err(Error::INVALID_UTF8))
} else {
Expand Down
3 changes: 2 additions & 1 deletion library/std/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@
#![allow(internal_features)]
#![deny(rustc::existing_doc_keyword)]
#![deny(fuzzy_provenance_casts)]
#![deny(unsafe_op_in_unsafe_fn)]
#![allow(rustdoc::redundant_explicit_links)]
// Ensure that std can be linked against panic_abort despite compiled with `-C panic=unwind`
#![deny(ffi_unwind_calls)]
Expand Down Expand Up @@ -664,7 +665,7 @@ pub mod alloc;
mod panicking;

#[path = "../../backtrace/src/lib.rs"]
#[allow(dead_code, unused_attributes, fuzzy_provenance_casts)]
#[allow(dead_code, unused_attributes, fuzzy_provenance_casts, unsafe_op_in_unsafe_fn)]
mod backtrace_rs;

// Re-export macros defined in core.
Expand Down
1 change: 1 addition & 0 deletions library/std/src/os/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#![stable(feature = "os", since = "1.0.0")]
#![allow(missing_docs, nonstandard_style, missing_debug_implementations)]
#![allow(unsafe_op_in_unsafe_fn)]

pub mod raw;

Expand Down
22 changes: 13 additions & 9 deletions library/std/src/sync/mpmc/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,11 +200,12 @@ impl<T> Channel<T> {
return Err(msg);
}

let slot: &Slot<T> = &*(token.array.slot as *const Slot<T>);

// Write the message into the slot and update the stamp.
slot.msg.get().write(MaybeUninit::new(msg));
slot.stamp.store(token.array.stamp, Ordering::Release);
unsafe {
let slot: &Slot<T> = &*(token.array.slot as *const Slot<T>);
slot.msg.get().write(MaybeUninit::new(msg));
slot.stamp.store(token.array.stamp, Ordering::Release);
}

// Wake a sleeping receiver.
self.receivers.notify();
Expand Down Expand Up @@ -291,11 +292,14 @@ impl<T> Channel<T> {
return Err(());
}

let slot: &Slot<T> = &*(token.array.slot as *const Slot<T>);

// Read the message from the slot and update the stamp.
let msg = slot.msg.get().read().assume_init();
slot.stamp.store(token.array.stamp, Ordering::Release);
let msg = unsafe {
let slot: &Slot<T> = &*(token.array.slot as *const Slot<T>);

let msg = slot.msg.get().read().assume_init();
slot.stamp.store(token.array.stamp, Ordering::Release);
msg
};

// Wake a sleeping sender.
self.senders.notify();
Expand Down Expand Up @@ -471,7 +475,7 @@ impl<T> Channel<T> {
false
};

self.discard_all_messages(tail);
unsafe { self.discard_all_messages(tail) };
disconnected
}

Expand Down
4 changes: 2 additions & 2 deletions library/std/src/sync/mpmc/counter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ impl<C> Sender<C> {
disconnect(&self.counter().chan);

if self.counter().destroy.swap(true, Ordering::AcqRel) {
drop(Box::from_raw(self.counter));
drop(unsafe { Box::from_raw(self.counter) });
}
}
}
Expand Down Expand Up @@ -116,7 +116,7 @@ impl<C> Receiver<C> {
disconnect(&self.counter().chan);

if self.counter().destroy.swap(true, Ordering::AcqRel) {
drop(Box::from_raw(self.counter));
drop(unsafe { Box::from_raw(self.counter) });
}
}
}
Expand Down
38 changes: 21 additions & 17 deletions library/std/src/sync/mpmc/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ impl<T> Block<T> {
// It is not necessary to set the `DESTROY` bit in the last slot because that slot has
// begun destruction of the block.
for i in start..BLOCK_CAP - 1 {
let slot = (*this).slots.get_unchecked(i);
let slot = unsafe { (*this).slots.get_unchecked(i) };

// Mark the `DESTROY` bit if a thread is still using the slot.
if slot.state.load(Ordering::Acquire) & READ == 0
Expand All @@ -103,7 +103,7 @@ impl<T> Block<T> {
}

// No thread is using the block, now it is safe to destroy it.
drop(Box::from_raw(this));
drop(unsafe { Box::from_raw(this) });
}
}

Expand Down Expand Up @@ -265,9 +265,11 @@ impl<T> Channel<T> {
// Write the message into the slot.
let block = token.list.block as *mut Block<T>;
let offset = token.list.offset;
let slot = (*block).slots.get_unchecked(offset);
slot.msg.get().write(MaybeUninit::new(msg));
slot.state.fetch_or(WRITE, Ordering::Release);
unsafe {
let slot = (*block).slots.get_unchecked(offset);
slot.msg.get().write(MaybeUninit::new(msg));
slot.state.fetch_or(WRITE, Ordering::Release);
}

// Wake a sleeping receiver.
self.receivers.notify();
Expand Down Expand Up @@ -369,19 +371,21 @@ impl<T> Channel<T> {
// Read the message.
let block = token.list.block as *mut Block<T>;
let offset = token.list.offset;
let slot = (*block).slots.get_unchecked(offset);
slot.wait_write();
let msg = slot.msg.get().read().assume_init();

// Destroy the block if we've reached the end, or if another thread wanted to destroy but
// couldn't because we were busy reading from the slot.
if offset + 1 == BLOCK_CAP {
Block::destroy(block, 0);
} else if slot.state.fetch_or(READ, Ordering::AcqRel) & DESTROY != 0 {
Block::destroy(block, offset + 1);
}
unsafe {
let slot = (*block).slots.get_unchecked(offset);
slot.wait_write();
let msg = slot.msg.get().read().assume_init();

// Destroy the block if we've reached the end, or if another thread wanted to destroy but
// couldn't because we were busy reading from the slot.
if offset + 1 == BLOCK_CAP {
Block::destroy(block, 0);
} else if slot.state.fetch_or(READ, Ordering::AcqRel) & DESTROY != 0 {
Block::destroy(block, offset + 1);
}

Ok(msg)
Ok(msg)
}
}

/// Attempts to send a message into the channel.
Expand Down
20 changes: 12 additions & 8 deletions library/std/src/sync/mpmc/zero.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,11 @@ impl<T> Channel<T> {
return Err(msg);
}

let packet = &*(token.zero.0 as *const Packet<T>);
packet.msg.get().write(Some(msg));
packet.ready.store(true, Ordering::Release);
unsafe {
let packet = &*(token.zero.0 as *const Packet<T>);
packet.msg.get().write(Some(msg));
packet.ready.store(true, Ordering::Release);
}
Ok(())
}

Expand All @@ -116,22 +118,24 @@ impl<T> Channel<T> {
return Err(());
}

let packet = &*(token.zero.0 as *const Packet<T>);
let packet = unsafe { &*(token.zero.0 as *const Packet<T>) };

if packet.on_stack {
// The message has been in the packet from the beginning, so there is no need to wait
// for it. However, after reading the message, we need to set `ready` to `true` in
// order to signal that the packet can be destroyed.
let msg = packet.msg.get().replace(None).unwrap();
let msg = unsafe { packet.msg.get().replace(None) }.unwrap();
packet.ready.store(true, Ordering::Release);
Ok(msg)
} else {
// Wait until the message becomes available, then read it and destroy the
// heap-allocated packet.
packet.wait_ready();
let msg = packet.msg.get().replace(None).unwrap();
drop(Box::from_raw(token.zero.0 as *mut Packet<T>));
Ok(msg)
unsafe {
let msg = packet.msg.get().replace(None).unwrap();
drop(Box::from_raw(token.zero.0 as *mut Packet<T>));
Ok(msg)
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions library/std/src/sync/once_lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,7 @@ impl<T> OnceLock<T> {
#[inline]
unsafe fn get_unchecked(&self) -> &T {
debug_assert!(self.is_initialized());
(&*self.value.get()).assume_init_ref()
unsafe { (&*self.value.get()).assume_init_ref() }
}

/// # Safety
Expand All @@ -511,7 +511,7 @@ impl<T> OnceLock<T> {
#[inline]
unsafe fn get_unchecked_mut(&mut self) -> &mut T {
debug_assert!(self.is_initialized());
(&mut *self.value.get()).assume_init_mut()
unsafe { (&mut *self.value.get()).assume_init_mut() }
}
}

Expand Down
4 changes: 3 additions & 1 deletion library/std/src/sync/reentrant_lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,9 @@ impl<T: ?Sized> ReentrantLock<T> {
}

unsafe fn increment_lock_count(&self) -> Option<()> {
*self.lock_count.get() = (*self.lock_count.get()).checked_add(1)?;
unsafe {
*self.lock_count.get() = (*self.lock_count.get()).checked_add(1)?;
}
Some(())
}
}
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/sync/rwlock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ impl<'rwlock, T: ?Sized> RwLockReadGuard<'rwlock, T> {
// successfully called from the same thread before instantiating this object.
unsafe fn new(lock: &'rwlock RwLock<T>) -> LockResult<RwLockReadGuard<'rwlock, T>> {
poison::map_result(lock.poison.borrow(), |()| RwLockReadGuard {
data: NonNull::new_unchecked(lock.data.get()),
data: unsafe { NonNull::new_unchecked(lock.data.get()) },
inner_lock: &lock.inner,
})
}
Expand Down
2 changes: 2 additions & 0 deletions library/std/src/sys/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![allow(unsafe_op_in_unsafe_fn)]

/// The PAL (platform abstraction layer) contains platform-specific abstractions
/// for implementing the features in the other submodules, e.g. UNIX file
/// descriptors.
Expand Down
14 changes: 10 additions & 4 deletions library/std/src/sys_common/wtf8.rs
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,8 @@ impl Wtf8 {
/// marked unsafe.
#[inline]
pub unsafe fn from_bytes_unchecked(value: &[u8]) -> &Wtf8 {
mem::transmute(value)
// SAFETY: start with &[u8], end with fancy &[u8]
unsafe { &*(value as *const [u8] as *const Wtf8) }
}

/// Creates a mutable WTF-8 slice from a mutable WTF-8 byte slice.
Expand All @@ -611,7 +612,8 @@ impl Wtf8 {
/// marked unsafe.
#[inline]
unsafe fn from_mut_bytes_unchecked(value: &mut [u8]) -> &mut Wtf8 {
mem::transmute(value)
// SAFETY: start with &mut [u8], end with fancy &mut [u8]
unsafe { &mut *(value as *mut [u8] as *mut Wtf8) }
}

/// Returns the length, in WTF-8 bytes.
Expand Down Expand Up @@ -942,8 +944,12 @@ pub fn check_utf8_boundary(slice: &Wtf8, index: usize) {
/// Copied from core::str::raw::slice_unchecked
#[inline]
pub unsafe fn slice_unchecked(s: &Wtf8, begin: usize, end: usize) -> &Wtf8 {
// memory layout of a &[u8] and &Wtf8 are the same
Wtf8::from_bytes_unchecked(slice::from_raw_parts(s.bytes.as_ptr().add(begin), end - begin))
// SAFETY: memory layout of a &[u8] and &Wtf8 are the same
unsafe {
let len = end - begin;
let start = s.as_bytes().as_ptr().add(begin);
Wtf8::from_bytes_unchecked(slice::from_raw_parts(start, len))
}
}

/// Copied from core::str::raw::slice_error_fail
Expand Down

0 comments on commit a6b4ff6

Please sign in to comment.