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

Apply #![deny(unsafe_op_in_unsafe_fn)] to sys/windows #76676

Closed
77 changes: 56 additions & 21 deletions library/std/src/sys/windows/alloc.rs
Original file line number Diff line number Diff line change
@@ -1,61 +1,96 @@
#![deny(unsafe_op_in_unsafe_fn)]

use crate::alloc::{GlobalAlloc, Layout, System};
use crate::ptr;
use crate::sys::c;
use crate::sys_common::alloc::{realloc_fallback, MIN_ALIGN};

#[repr(C)]
struct Header(*mut u8);

unsafe fn get_header<'a>(ptr: *mut u8) -> &'a mut Header {
&mut *(ptr as *mut Header).offset(-1)
/// # Safety
///
/// There must be a `Header` at `ptr.offset(-1)`.
unsafe fn get_header<'a>(ptr: *mut u8) -> *mut Header {
// SAFETY: the safety contract must be upheld by the caller
carbotaniuman marked this conversation as resolved.
Show resolved Hide resolved
unsafe { &mut *(ptr as *mut Header).offset(-1) }
}

/// # Safety
///
/// `ptr`, once aligned, must have space for a Header at `ptr.offset(-1)`.
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 not a sufficient condition. ptr must not only have space, but also be valid for writing. Consider borrowing from https://doc.rust-lang.org/nightly/std/ptr/fn.write.html#safety, for example.

It looks like regardless this function is wrong as it gets &mut access to uninitialized memory -- it would be worth fixing that before trying to document unsafety here, I think, likely in a separate PR. Or at least leaving a FIXME comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I just inline the function and avoid the &mut?

Copy link
Member

Choose a reason for hiding this comment

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

Change get_header to return a *mut Header and change align_ptr to use ptr::write to write the header.

unsafe fn align_ptr(ptr: *mut u8, align: usize) -> *mut u8 {
let aligned = ptr.add(align - (ptr as usize & (align - 1)));
*get_header(aligned) = Header(ptr);
aligned
// SAFETY: the safety contract must be upheld by the caller
carbotaniuman marked this conversation as resolved.
Show resolved Hide resolved
unsafe {
let aligned = ptr.add(align - (ptr as usize & (align - 1)));
ptr::write(get_header(aligned), Header(ptr));
aligned
}
}

#[inline]
unsafe fn allocate_with_flags(layout: Layout, flags: c::DWORD) -> *mut u8 {
if layout.align() <= MIN_ALIGN {
return c::HeapAlloc(c::GetProcessHeap(), flags, layout.size()) as *mut u8;
// SAFETY: `layout.size()` comes from `Layout` and is valid.
return unsafe { c::HeapAlloc(c::GetProcessHeap(), flags, layout.size()) as *mut u8 };
}

let size = layout.size() + layout.align();
let ptr = c::HeapAlloc(c::GetProcessHeap(), flags, size);
if ptr.is_null() { ptr as *mut u8 } else { align_ptr(ptr as *mut u8, layout.align()) }
let ptr = unsafe {
// SAFETY: The caller must ensure that
// `layout.size()` + `layout.align()` does not overflow.
let size = layout.size() + layout.align();
c::HeapAlloc(c::GetProcessHeap(), flags, size)
};

if ptr.is_null() {
ptr as *mut u8
} else {
// SAFETY: `ptr` is a valid pointer
// with enough allocated space to store the header.
unsafe { align_ptr(ptr as *mut u8, layout.align()) }
}
}

// SAFETY: All methods implemented follow the contract rules defined
// in `GlobalAlloc`.
#[stable(feature = "alloc_system_type", since = "1.28.0")]
unsafe impl GlobalAlloc for System {
#[inline]
unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
allocate_with_flags(layout, 0)
// SAFETY: the safety contract for `allocate_with_flags` must be upheld by the caller.
unsafe { allocate_with_flags(layout, 0) }
}

#[inline]
unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
allocate_with_flags(layout, c::HEAP_ZERO_MEMORY)
// SAFETY: the safety contract for `allocate_with_flags must be upheld by the caller.
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 likely not sufficient. See e.g. #74477 for necessary commentary here; these comments should show that the function being called has preconditions equivalent to the trait.

There are several more cases in this PR where you assert that the preconditions/safety contract of X must be upheld by the caller, please take a look at those and try to adjust them too.

unsafe { allocate_with_flags(layout, c::HEAP_ZERO_MEMORY) }
}

#[inline]
unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
if layout.align() <= MIN_ALIGN {
let err = c::HeapFree(c::GetProcessHeap(), 0, ptr as c::LPVOID);
debug_assert!(err != 0, "Failed to free heap memory: {}", c::GetLastError());
} else {
let header = get_header(ptr);
let err = c::HeapFree(c::GetProcessHeap(), 0, header.0 as c::LPVOID);
debug_assert!(err != 0, "Failed to free heap memory: {}", c::GetLastError());
}
// SAFETY: HeapFree is safe if ptr was allocated by this allocator
let err = unsafe {
if layout.align() <= MIN_ALIGN {
c::HeapFree(c::GetProcessHeap(), 0, ptr as c::LPVOID)
} else {
let header = get_header(ptr);
c::HeapFree(c::GetProcessHeap(), 0, (*header).0 as c::LPVOID)
}
};
// SAFETY: `c::GetLastError()` cannot fail
debug_assert!(err != 0, "Failed to free heap memory: {}", unsafe { c::GetLastError() });
carbotaniuman marked this conversation as resolved.
Show resolved Hide resolved
}

#[inline]
unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 {
// SAFETY: HeapReAlloc/realloc_fallback is safe if ptr was allocated by this allocator
// and new_size is not 0.
debug_assert_ne!(new_size, 0);
if layout.align() <= MIN_ALIGN {
c::HeapReAlloc(c::GetProcessHeap(), 0, ptr as c::LPVOID, new_size) as *mut u8
unsafe { c::HeapReAlloc(c::GetProcessHeap(), 0, ptr as c::LPVOID, new_size) as *mut u8 }
} else {
realloc_fallback(self, ptr, layout, new_size)
unsafe { realloc_fallback(self, ptr, layout, new_size) }
}
}
}
14 changes: 9 additions & 5 deletions library/std/src/sys/windows/args.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#![allow(dead_code)] // runtime init functions not used during testing
#![deny(unsafe_op_in_unsafe_fn)]

#[cfg(test)]
mod tests;
Expand Down Expand Up @@ -52,11 +53,14 @@ unsafe fn parse_lp_cmd_line<F: Fn() -> OsString>(
const TAB: u16 = '\t' as u16;
const SPACE: u16 = ' ' as u16;
let mut ret_val = Vec::new();
if lp_cmd_line.is_null() || *lp_cmd_line == 0 {
ret_val.push(exe_name());
return ret_val;
}
let mut cmd_line = {

// SAFETY: the caller must supply a pointer that is valid to dereference
let mut cmd_line = unsafe {
if lp_cmd_line.is_null() || *lp_cmd_line == 0 {
ret_val.push(exe_name());
return ret_val;
}

let mut end = 0;
while *lp_cmd_line.offset(end) != 0 {
end += 1;
Expand Down
1 change: 1 addition & 0 deletions library/std/src/sys/windows/c.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! C definitions used by libnative that don't belong in liblibc

#![allow(nonstandard_style)]
#![deny(unsafe_op_in_unsafe_fn)]
#![cfg_attr(test, allow(dead_code))]
#![unstable(issue = "none", feature = "windows_c")]

Expand Down
1 change: 1 addition & 0 deletions library/std/src/sys/windows/cmath.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#![deny(unsafe_op_in_unsafe_fn)]
#![cfg(not(test))]

use libc::{c_double, c_float};
Expand Down
13 changes: 11 additions & 2 deletions library/std/src/sys/windows/compat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
//! manner we pay a semi-large one-time cost up front for detecting whether a
//! function is available but afterwards it's just a load and a jump.

#![deny(unsafe_op_in_unsafe_fn)]

use crate::ffi::CString;
use crate::sys::c;

Expand Down Expand Up @@ -43,7 +45,12 @@ macro_rules! compat_fn {
static PTR: AtomicUsize = AtomicUsize::new(0);

#[allow(unused_variables)]
unsafe extern "system" fn fallback($($argname: $argtype),*) -> $rettype $body
unsafe extern "system" fn fallback($($argname: $argtype),*) -> $rettype {
#[allow(unused_unsafe)]
unsafe {
$body
}
}

/// This address is stored in `PTR` to incidate an unavailable API.
///
Expand Down Expand Up @@ -86,7 +93,9 @@ macro_rules! compat_fn {
}

pub unsafe fn call($($argname: $argtype),*) -> $rettype {
mem::transmute::<usize, F>(addr())($($argname),*)
unsafe {
mem::transmute::<usize, F>(addr())($($argname),*)
}
}
}

Expand Down
28 changes: 19 additions & 9 deletions library/std/src/sys/windows/condvar.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![deny(unsafe_op_in_unsafe_fn)]

use crate::cell::UnsafeCell;
use crate::sys::c;
use crate::sys::mutex::{self, Mutex};
Expand All @@ -23,17 +25,23 @@ impl Condvar {

#[inline]
pub unsafe fn wait(&self, mutex: &Mutex) {
let r = c::SleepConditionVariableSRW(self.inner.get(), mutex::raw(mutex), c::INFINITE, 0);
// SAFETY: The caller must ensure that the condvar is not moved or copied
let r = unsafe {
c::SleepConditionVariableSRW(self.inner.get(), mutex::raw(mutex), c::INFINITE, 0)
};
debug_assert!(r != 0);
}

pub unsafe fn wait_timeout(&self, mutex: &Mutex, dur: Duration) -> bool {
let r = c::SleepConditionVariableSRW(
self.inner.get(),
mutex::raw(mutex),
super::dur2timeout(dur),
0,
);
// SAFETY: The caller must ensure that the condvar is not moved or copied
let r = unsafe {
c::SleepConditionVariableSRW(
self.inner.get(),
mutex::raw(mutex),
super::dur2timeout(dur),
0,
)
};
if r == 0 {
debug_assert_eq!(os::errno() as usize, c::ERROR_TIMEOUT as usize);
false
Expand All @@ -44,12 +52,14 @@ impl Condvar {

#[inline]
pub unsafe fn notify_one(&self) {
c::WakeConditionVariable(self.inner.get())
// SAFETY: The caller must ensure that the condvar is not moved or copied
unsafe { c::WakeConditionVariable(self.inner.get()) }
}

#[inline]
pub unsafe fn notify_all(&self) {
c::WakeAllConditionVariable(self.inner.get())
// SAFETY: The caller must ensure that the condvar is not moved or copied
unsafe { c::WakeAllConditionVariable(self.inner.get()) }
}

pub unsafe fn destroy(&self) {
Expand Down
2 changes: 2 additions & 0 deletions library/std/src/sys/windows/env.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![deny(unsafe_op_in_unsafe_fn)]

pub mod os {
pub const FAMILY: &str = "windows";
pub const OS: &str = "windows";
Expand Down
1 change: 1 addition & 0 deletions library/std/src/sys/windows/ext/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
//! [`collect`]: crate::iter::Iterator::collect
//! [U+FFFD]: crate::char::REPLACEMENT_CHARACTER

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

use crate::ffi::{OsStr, OsString};
Expand Down
1 change: 1 addition & 0 deletions library/std/src/sys/windows/ext/fs.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Windows-specific extensions for the primitives in the `std::fs` module.

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

use crate::fs::{self, Metadata, OpenOptions};
Expand Down
1 change: 1 addition & 0 deletions library/std/src/sys/windows/ext/io.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Windows-specific extensions to general I/O primitives.

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

use crate::fs;
Expand Down
1 change: 1 addition & 0 deletions library/std/src/sys/windows/ext/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
//! `std` types and idioms with Windows in a way that the normal
//! platform-agnostic idioms would not normally support.

#![deny(unsafe_op_in_unsafe_fn)]
#![stable(feature = "rust1", since = "1.0.0")]
#![doc(cfg(windows))]
#![allow(missing_docs)]
Expand Down
1 change: 1 addition & 0 deletions library/std/src/sys/windows/ext/process.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Extensions to `std::process` for Windows.

#![deny(unsafe_op_in_unsafe_fn)]
#![stable(feature = "process_extensions", since = "1.2.0")]

use crate::os::windows::io::{AsRawHandle, FromRawHandle, IntoRawHandle, RawHandle};
Expand Down
1 change: 1 addition & 0 deletions library/std/src/sys/windows/ext/raw.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Windows-specific primitives.

#![deny(unsafe_op_in_unsafe_fn)]
#![stable(feature = "raw_ext", since = "1.1.0")]

use crate::os::raw::c_void;
Expand Down
1 change: 1 addition & 0 deletions library/std/src/sys/windows/ext/thread.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Extensions to `std::thread` for Windows.

#![deny(unsafe_op_in_unsafe_fn)]
#![stable(feature = "thread_extensions", since = "1.9.0")]

use crate::os::windows::io::{AsRawHandle, IntoRawHandle, RawHandle};
Expand Down
5 changes: 4 additions & 1 deletion library/std/src/sys/windows/fs.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![deny(unsafe_op_in_unsafe_fn)]

use crate::os::windows::prelude::*;

use crate::ffi::OsString;
Expand Down Expand Up @@ -862,7 +864,8 @@ pub fn copy(from: &Path, to: &Path) -> io::Result<u64> {
lpData: c::LPVOID,
) -> c::DWORD {
if dwStreamNumber == 1 {
*(lpData as *mut i64) = StreamBytesTransferred;
//SAFETY: We pass a *mut i64 as lpData to CopyFileExW, so we must get it back
unsafe { *(lpData as *mut i64) = StreamBytesTransferred };
}
c::PROGRESS_CONTINUE
}
Expand Down
6 changes: 5 additions & 1 deletion library/std/src/sys/windows/handle.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#![deny(unsafe_op_in_unsafe_fn)]
#![unstable(issue = "none", feature = "windows_handle")]

use crate::cmp;
Expand Down Expand Up @@ -120,7 +121,10 @@ impl RawHandle {
) -> io::Result<Option<usize>> {
let len = cmp::min(buf.len(), <c::DWORD>::MAX as usize) as c::DWORD;
let mut amt = 0;
let res = cvt(c::ReadFile(self.0, buf.as_ptr() as c::LPVOID, len, &mut amt, overlapped));
// SAFETY: the safety contract must be upheld by the caller
let res = unsafe {
cvt(c::ReadFile(self.0, buf.as_ptr() as c::LPVOID, len, &mut amt, overlapped))
};
match res {
Ok(_) => Ok(Some(amt as usize)),
Err(e) => {
Expand Down
2 changes: 2 additions & 0 deletions library/std/src/sys/windows/io.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![deny(unsafe_op_in_unsafe_fn)]

use crate::marker::PhantomData;
use crate::slice;
use crate::sys::c;
Expand Down
1 change: 1 addition & 0 deletions library/std/src/sys/windows/memchr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@
// Copyright 2015 Andrew Gallant, bluss and Nicolas Koch

// Fallback memchr is fastest on Windows.
#![deny(unsafe_op_in_unsafe_fn)]
pub use core::slice::memchr::{memchr, memrchr};
1 change: 1 addition & 0 deletions library/std/src/sys/windows/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#![deny(unsafe_op_in_unsafe_fn)]
#![allow(missing_docs, nonstandard_style)]

use crate::ffi::{OsStr, OsString};
Expand Down
Loading