Skip to content

Deny unsafe ops in unsafe fns in std::sys_common #73928

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

Closed
Closed
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
21 changes: 16 additions & 5 deletions library/std/src/sys_common/alloc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,25 @@ pub unsafe fn realloc_fallback(
old_layout: Layout,
new_size: usize,
) -> *mut u8 {
// Docs for GlobalAlloc::realloc require this to be valid:
let new_layout = Layout::from_size_align_unchecked(new_size, old_layout.align());
// SAFETY: as stated in docs for GlobalAlloc::realloc, the caller
// must guarantee that `new_size` is valid for a `Layout`.
Copy link
Member

Choose a reason for hiding this comment

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

Please put any guarantees the caller must make in a documentation comment on the function.

// The `old_layout.align()` is guaranteed to be valid as it comes
// from a `Layout`.
let new_layout = unsafe { Layout::from_size_align_unchecked(new_size, old_layout.align()) };

let new_ptr = GlobalAlloc::alloc(alloc, new_layout);
// SAFETY: as stated in docs for GlobalAlloc::realloc, the caller
// must guarantee that `new_size` is greater than zero.
let new_ptr = unsafe { GlobalAlloc::alloc(alloc, new_layout) };
if !new_ptr.is_null() {
let size = cmp::min(old_layout.size(), new_size);
ptr::copy_nonoverlapping(ptr, new_ptr, size);
GlobalAlloc::dealloc(alloc, ptr, old_layout);
// SAFETY: the newly allocated memory cannot overlap the previously
// allocated memory. Also, the call to `dealloc` is safe since
// the caller must guarantee that `ptr` is allocated via this allocator
// and layout is the same layout that was used to allocate `ptr`.
unsafe {
ptr::copy_nonoverlapping(ptr, new_ptr, size);
GlobalAlloc::dealloc(alloc, ptr, old_layout);
}
}
new_ptr
}
10 changes: 7 additions & 3 deletions library/std/src/sys_common/at_exit_imp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,14 @@ const DONE: *mut Queue = 1_usize as *mut _;
const ITERS: usize = 10;

unsafe fn init() -> bool {
if QUEUE.is_null() {
// SAFETY: the caller must ensure that `QUEUE` is not in use anywhere else,
// which `push` below does by locking `LOCK`.
if unsafe { QUEUE.is_null() } {
let state: Box<Queue> = box Vec::new();
QUEUE = Box::into_raw(state);
} else if QUEUE == DONE {
unsafe {
QUEUE = Box::into_raw(state);
}
} else if unsafe { QUEUE == DONE } {
// can't re-init after a cleanup
return false;
}
Expand Down
64 changes: 33 additions & 31 deletions library/std/src/sys_common/backtrace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,44 +76,46 @@ unsafe fn _print_fmt(fmt: &mut fmt::Formatter<'_>, print_fmt: PrintFmt) -> fmt::
let mut res = Ok(());
// Start immediately if we're not using a short backtrace.
let mut start = print_fmt != PrintFmt::Short;
backtrace_rs::trace_unsynchronized(|frame| {
if print_fmt == PrintFmt::Short && idx > MAX_NB_FRAMES {
return false;
}
unsafe {
backtrace_rs::trace_unsynchronized(|frame| {
if print_fmt == PrintFmt::Short && idx > MAX_NB_FRAMES {
return false;
}

let mut hit = false;
let mut stop = false;
backtrace_rs::resolve_frame_unsynchronized(frame, |symbol| {
hit = true;
if print_fmt == PrintFmt::Short {
if let Some(sym) = symbol.name().and_then(|s| s.as_str()) {
if sym.contains("__rust_begin_short_backtrace") {
stop = true;
return;
}
if sym.contains("__rust_end_short_backtrace") {
start = true;
return;
let mut hit = false;
let mut stop = false;
backtrace_rs::resolve_frame_unsynchronized(frame, |symbol| {
hit = true;
if print_fmt == PrintFmt::Short {
if let Some(sym) = symbol.name().and_then(|s| s.as_str()) {
if sym.contains("__rust_begin_short_backtrace") {
stop = true;
return;
}
if sym.contains("__rust_end_short_backtrace") {
start = true;
return;
}
}
}
}

if start {
res = bt_fmt.frame().symbol(frame, symbol);
if start {
res = bt_fmt.frame().symbol(frame, symbol);
}
});
if stop {
return false;
}
});
if stop {
return false;
}
if !hit {
if start {
res = bt_fmt.frame().print_raw(frame.ip(), None, None, None);
if !hit {
if start {
res = bt_fmt.frame().print_raw(frame.ip(), None, None, None);
}
}
}

idx += 1;
res.is_ok()
});
idx += 1;
res.is_ok()
});
}
res?;
bt_fmt.finish()?;
if print_fmt == PrintFmt::Short {
Expand Down
18 changes: 12 additions & 6 deletions library/std/src/sys_common/condvar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,22 @@ impl Condvar {
/// address.
#[inline]
pub unsafe fn init(&mut self) {
self.0.init()
// SAFETY: the caller must uphold the safety contract for `init`.
unsafe { self.0.init() }
}

/// Signals one waiter on this condition variable to wake up.
#[inline]
pub unsafe fn notify_one(&self) {
self.0.notify_one()
// SAFETY: the caller must uphold the safety contract for `notify_one`.
unsafe { self.0.notify_one() }
}

/// Awakens all current waiters on this condition variable.
#[inline]
pub unsafe fn notify_all(&self) {
self.0.notify_all()
// SAFETY: the caller must uphold the safety contract for `notify_all`.
unsafe { self.0.notify_all() }
}

/// Waits for a signal on the specified mutex.
Expand All @@ -47,7 +50,8 @@ impl Condvar {
/// on this condition variable.
#[inline]
pub unsafe fn wait(&self, mutex: &Mutex) {
self.0.wait(mutex::raw(mutex))
// SAFETY: the caller must uphold the safety contract for `wait`.
unsafe { self.0.wait(mutex::raw(mutex)) }
}

/// Waits for a signal on the specified mutex with a timeout duration
Expand All @@ -58,7 +62,8 @@ impl Condvar {
/// on this condition variable.
#[inline]
pub unsafe fn wait_timeout(&self, mutex: &Mutex, dur: Duration) -> bool {
self.0.wait_timeout(mutex::raw(mutex), dur)
// SAFETY: the caller must uphold the safety contract for `wait_timeout`.
unsafe { self.0.wait_timeout(mutex::raw(mutex), dur) }
}

/// Deallocates all resources associated with this condition variable.
Expand All @@ -67,6 +72,7 @@ impl Condvar {
/// this condition variable.
#[inline]
pub unsafe fn destroy(&self) {
self.0.destroy()
// SAFETY: the caller must uphold the safety contract for `destroy`.
unsafe { self.0.destroy() }
}
}
1 change: 1 addition & 0 deletions library/std/src/sys_common/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

#![allow(missing_docs)]
#![allow(missing_debug_implementations)]
#![deny(unsafe_op_in_unsafe_fn)]

#[cfg(test)]
mod tests;
Expand Down
20 changes: 14 additions & 6 deletions library/std/src/sys_common/mutex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ impl Mutex {
/// `init()`) is undefined behavior.
#[inline]
pub unsafe fn init(&mut self) {
self.0.init()
// SAFETY: the caller must uphold the safety contract for `init`.
unsafe { self.0.init() }
}

/// Locks the mutex blocking the current thread until it is available.
Expand All @@ -39,14 +40,18 @@ impl Mutex {
/// previous function call.
#[inline]
pub unsafe fn raw_lock(&self) {
self.0.lock()
// SAFETY: the caller must uphold the safety contract for `lock`.
unsafe { self.0.lock() }
}

/// Calls raw_lock() and then returns an RAII guard to guarantee the mutex
/// will be unlocked.
#[inline]
pub unsafe fn lock(&self) -> MutexGuard<'_> {
self.raw_lock();
// SAFETY: the caller must uphold the safety contract for `raw_lock`.
unsafe {
self.raw_lock();
}
MutexGuard(&self.0)
}

Expand All @@ -57,7 +62,8 @@ impl Mutex {
/// previous function call.
#[inline]
pub unsafe fn try_lock(&self) -> bool {
self.0.try_lock()
// SAFETY: the caller must uphold the safety contract for `try_lock`.
unsafe { self.0.try_lock() }
}

/// Unlocks the mutex.
Expand All @@ -69,7 +75,8 @@ impl Mutex {
/// lock() whenever possible.
#[inline]
pub unsafe fn raw_unlock(&self) {
self.0.unlock()
// SAFETY: the caller must uphold the safety contract for `unlock`.
unsafe { self.0.unlock() }
}

/// Deallocates all resources associated with this mutex.
Expand All @@ -78,7 +85,8 @@ impl Mutex {
/// this mutex.
#[inline]
pub unsafe fn destroy(&self) {
self.0.destroy()
// SAFETY: the caller must uphold the safety contract for `destroy`.
unsafe { self.0.destroy() }
}
}

Expand Down
11 changes: 9 additions & 2 deletions library/std/src/sys_common/remutex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,12 @@ impl<T> ReentrantMutex<T> {
/// once this mutex is in its final resting place, and only then are the
/// lock/unlock methods safe.
pub const unsafe fn new(t: T) -> ReentrantMutex<T> {
ReentrantMutex { inner: sys::ReentrantMutex::uninitialized(), data: t }
// `ReentrantMutex::uninitialized()` is not unsafe on all platforms
#[allow(unused_unsafe)]
// SAFETY: the caller must guarantee that `init` is called.
unsafe {
ReentrantMutex { inner: sys::ReentrantMutex::uninitialized(), data: t }
}
}

/// Initializes this mutex so it's ready for use.
Expand All @@ -63,7 +68,8 @@ impl<T> ReentrantMutex<T> {
/// Unsafe to call more than once, and must be called after this will no
/// longer move in memory.
pub unsafe fn init(&self) {
self.inner.init();
// SAFETY: the caller must uphold the safety contract for `init`.
unsafe { self.inner.init() };
}

/// Acquires a mutex, blocking the current thread until it is able to do so.
Expand All @@ -79,6 +85,7 @@ impl<T> ReentrantMutex<T> {
/// this call will return failure if the mutex would otherwise be
/// acquired.
pub fn lock(&self) -> ReentrantMutexGuard<'_, T> {
// SAFETY: the caller must uphold the safety contract for `lock`.
unsafe { self.inner.lock() }
ReentrantMutexGuard::new(&self)
}
Expand Down
21 changes: 14 additions & 7 deletions library/std/src/sys_common/rwlock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ impl RWLock {
/// previous method call.
#[inline]
pub unsafe fn read(&self) {
self.0.read()
// SAFETY: the caller must uphold the safety contract for `read`.
unsafe { self.0.read() }
}

/// Attempts to acquire shared access to this lock, returning whether it
Expand All @@ -35,7 +36,8 @@ impl RWLock {
/// previous method call.
#[inline]
pub unsafe fn try_read(&self) -> bool {
self.0.try_read()
// SAFETY: the caller must uphold the safety contract for `try_read`.
unsafe { self.0.try_read() }
}

/// Acquires write access to the underlying lock, blocking the current thread
Expand All @@ -45,7 +47,8 @@ impl RWLock {
/// previous method call.
#[inline]
pub unsafe fn write(&self) {
self.0.write()
// SAFETY: the caller must uphold the safety contract for `write`.
unsafe { self.0.write() }
}

/// Attempts to acquire exclusive access to this lock, returning whether it
Expand All @@ -57,15 +60,17 @@ impl RWLock {
/// previous method call.
#[inline]
pub unsafe fn try_write(&self) -> bool {
self.0.try_write()
// SAFETY: the caller must uphold the safety contract for `try_write`.
unsafe { self.0.try_write() }
}

/// Unlocks previously acquired shared access to this lock.
///
/// Behavior is undefined if the current thread does not have shared access.
#[inline]
pub unsafe fn read_unlock(&self) {
self.0.read_unlock()
// SAFETY: the caller must uphold the safety contract for `read_unlock`.
unsafe { self.0.read_unlock() }
}

/// Unlocks previously acquired exclusive access to this lock.
Expand All @@ -74,7 +79,8 @@ impl RWLock {
/// exclusive access.
#[inline]
pub unsafe fn write_unlock(&self) {
self.0.write_unlock()
// SAFETY: the caller must uphold the safety contract for `write_unlock`.
unsafe { self.0.write_unlock() }
}

/// Destroys OS-related resources with this RWLock.
Expand All @@ -83,6 +89,7 @@ impl RWLock {
/// lock.
#[inline]
pub unsafe fn destroy(&self) {
self.0.destroy()
// SAFETY: the caller must uphold the safety contract for `destroy`.
unsafe { self.0.destroy() }
}
}
6 changes: 5 additions & 1 deletion library/std/src/sys_common/thread_info.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
#![allow(dead_code)] // stack_guard isn't used right now on all platforms
// stack_guard isn't used right now on all platforms
#![allow(dead_code)]
// FIXME: this should be removed once `thread_local!` denies unsafe ops in unsafe
// fns. This cannot be done yet because the macro is also used in libproc_macro.
#![allow(unsafe_op_in_unsafe_fn)]

use crate::cell::RefCell;
use crate::sys::thread::guard::Guard;
Expand Down
18 changes: 11 additions & 7 deletions library/std/src/sys_common/thread_local_dtor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,21 +29,25 @@ pub unsafe fn register_dtor_fallback(t: *mut u8, dtor: unsafe extern "C" fn(*mut

static DTORS: StaticKey = StaticKey::new(Some(run_dtors));
type List = Vec<(*mut u8, unsafe extern "C" fn(*mut u8))>;
if DTORS.get().is_null() {
if unsafe { DTORS.get().is_null() } {
let v: Box<List> = box Vec::new();
DTORS.set(Box::into_raw(v) as *mut u8);
unsafe { DTORS.set(Box::into_raw(v) as *mut u8) }
}
let list: &mut List = &mut *(DTORS.get() as *mut List);
let list: &mut List = unsafe { &mut *(DTORS.get() as *mut List) };
list.push((t, dtor));

unsafe extern "C" fn run_dtors(mut ptr: *mut u8) {
while !ptr.is_null() {
let list: Box<List> = Box::from_raw(ptr as *mut List);
let list: Box<List> = unsafe { Box::from_raw(ptr as *mut List) };
for (ptr, dtor) in list.into_iter() {
dtor(ptr);
unsafe {
dtor(ptr);
}
}
unsafe {
ptr = DTORS.get();
DTORS.set(ptr::null_mut());
}
ptr = DTORS.get();
DTORS.set(ptr::null_mut());
}
}
}
Loading