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.

Sign up for GitHub

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

sparse_mmap: remove C code and add SAFETY comments #117

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

sparse_mmap: remove C code and add SAFETY comments #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 0 additions & 1 deletion Cargo.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6254,7 +6254,6 @@ dependencies = [
name = "sparse_mmap"
version = "0.0.0"
dependencies = [
"cc",
"criterion",
"getrandom",
"libc",
Expand Down
3 changes: 0 additions & 3 deletions support/sparse_mmap/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@ name = "sparse_mmap"
edition.workspace = true
rust-version.workspace = true

[build-dependencies]
cc.workspace = true

[dependencies]
pal.workspace = true

Expand Down
47 changes: 0 additions & 47 deletions support/sparse_mmap/build.rs
View file
Open in desktop

This file was deleted.

22 changes: 22 additions & 0 deletions support/sparse_mmap/src/alloc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,22 @@ use unix as sys;
use windows as sys;

#[derive(Debug)]
/// A `Box`-like smart pointer for memory mappings.
/// Relies on the kernel interfaces rather than the
/// standard library API. That allows for the low-level
/// features this crate deals with.
pub struct Allocation {
ptr: *mut u8,
size: usize,
_dummy: std::marker::PhantomData<[u8]>,
}

// SAFETY: memory mappings are for created managing the guest memory
// and expected to live longer than the guest and any threads running
// the guest VPs. Essentially, these are constant when the guest runs.
// Thus the `ptr` field can be sent to other threads.
unsafe impl Send for Allocation {}
// SAFETY: Same as above for the sharing considerations.
unsafe impl Sync for Allocation {}

impl Allocation {
Expand All @@ -35,6 +44,7 @@ impl Allocation {

impl DerefMut for Allocation {
fn deref_mut(&mut self) -> &mut [u8] {
// SAFETY: the pointer is valid.
unsafe { slice::from_raw_parts_mut(self.ptr, self.size) }
}
}
Expand All @@ -43,12 +53,14 @@ impl Deref for Allocation {
type Target = [u8];

fn deref(&self) -> &[u8] {
// SAFETY: the pointer is valid.
unsafe { slice::from_raw_parts(self.ptr, self.size) }
}
}

impl Drop for Allocation {
fn drop(&mut self) {
// SAFETY: the pointer is valid.
unsafe {
sys::free(self.ptr, self.size);
}
Expand All @@ -70,6 +82,7 @@ impl Deref for SharedMem {
type Target = [AtomicU8];

fn deref(&self) -> &Self::Target {
// SAFETY: the pointer is valid.
unsafe { slice::from_raw_parts(self.alloc.ptr as *const AtomicU8, self.alloc.size) }
}
}
Expand All @@ -85,6 +98,8 @@ mod windows {
use windows_sys::Win32::System::Memory::PAGE_READWRITE;

pub fn alloc(size: usize) -> std::io::Result<*mut u8> {
// SAFETY: Calling the system API as required. The parameters have
// valid values.
let ptr = unsafe {
VirtualAlloc(
ptr::null_mut(),
Expand All @@ -100,6 +115,8 @@ mod windows {
}

pub unsafe fn free(ptr: *mut u8, _size: usize) {
// SAFETY: Calling the system API as required. The parameters have
// valid values.
let ret = unsafe { VirtualFree(ptr.cast(), 0, MEM_RELEASE) };
assert!(ret != 0);
}
Expand All @@ -110,6 +127,8 @@ mod unix {
use std::ptr;

pub fn alloc(size: usize) -> std::io::Result<*mut u8> {
// SAFETY: using the system API as required by the documentation.
// The values for the parameters are valid.
let ptr = unsafe {
libc::mmap(
ptr::null_mut(),
Expand All @@ -127,6 +146,8 @@ mod unix {
}

pub unsafe fn free(ptr: *mut u8, size: usize) {
// SAFETY: using the system API as required by the documentation.
// The values for the parameters are valid.
let ret = unsafe { libc::munmap(ptr.cast::<libc::c_void>(), size) };
assert!(ret == 0);
}
Expand All @@ -138,6 +159,7 @@ mod tests {

#[test]
fn test_alloc_free() -> Result<(), Box<dyn std::error::Error>> {
// SAFETY: memory is not shared between threads.
unsafe {
let x = sys::alloc(4096)?;
sys::free(x, 4096);
Expand Down
17 changes: 9 additions & 8 deletions support/sparse_mmap/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,11 @@
// Licensed under the MIT License.

//! Memory-related abstractions.

// UNSAFETY: Manual pointer manipulation, dealing with mmap, and a signal handler.
#![expect(unsafe_code)]
#![allow(clippy::undocumented_unsafe_blocks)]

pub mod alloc;
mod trycopy_unix;
mod trycopy_windows_arm64;
mod trycopy_windows_x64;
pub mod unix;
Expand Down Expand Up @@ -37,9 +36,8 @@ pub fn initialize_try_copy() {
#[cfg(unix)]
{
static INIT: std::sync::Once = std::sync::Once::new();
INIT.call_once(|| unsafe {
let err = install_signal_handlers();
if err != 0 {
INIT.call_once(|| {
if let Err(err) = trycopy_unix::install_signal_handlers() {
panic!(
"could not install signal handlers: {}",
std::io::Error::from_raw_os_error(err)
Expand All @@ -50,9 +48,6 @@ pub fn initialize_try_copy() {
}

unsafe extern "C" {
#[cfg(unix)]
fn install_signal_handlers() -> i32;

fn try_memmove(
dest: *mut u8,
src: *const u8,
Expand Down Expand Up @@ -634,6 +629,7 @@ mod tests {
};
let af_addr = &mut af as *mut _;

// SAFETY: no memory is shared between threads.
let res = unsafe {
match size {
Size::Bit8 => match primitive {
Expand Down Expand Up @@ -680,6 +676,7 @@ mod tests {
"Fault address must not be set for {primitive:?} and {size:?}"
);

// SAFETY: no memory is shared between threads.
let res = unsafe {
match size {
Size::Bit8 => match primitive {
Expand Down Expand Up @@ -745,13 +742,15 @@ mod tests {

let mapping = SparseMapping::new(range_size).unwrap();
mapping.alloc(page_size, page_size).unwrap();
// SAFETY: no memory is shared between threads.
let slice = unsafe {
std::slice::from_raw_parts_mut(mapping.as_ptr().add(page_size).cast::<u8>(), page_size)
};
slice.copy_from_slice(&BUF[..page_size]);
mapping.unmap(page_size, page_size).unwrap();

mapping.alloc(range_size - page_size, page_size).unwrap();
// SAFETY: no memory is shared between threads.
let slice = unsafe {
std::slice::from_raw_parts_mut(
mapping.as_ptr().add(range_size - page_size).cast::<u8>(),
Expand Down Expand Up @@ -780,6 +779,7 @@ mod tests {
let page_size = SparseMapping::page_size();
mapping.alloc(page_size, page_size).unwrap();
let base = mapping.as_ptr().cast::<u8>();
// SAFETY: no memory is shared between threads.
unsafe {
try_copy(BUF.as_ptr(), base, 100).unwrap_err();
try_copy(BUF.as_ptr(), base.add(page_size), 100).unwrap();
Expand All @@ -795,6 +795,7 @@ mod tests {
let mapping = SparseMapping::new(page_size * 2).unwrap();
mapping.alloc(0, page_size).unwrap();
let base = mapping.as_ptr().cast::<u8>();
// SAFETY: no memory is shared between threads.
unsafe {
assert_eq!(try_compare_exchange(base.add(8), 0, 1).unwrap().unwrap(), 1);
assert_eq!(
Expand Down
Loading
Loading