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

Document mmap safety #674

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
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
4 changes: 3 additions & 1 deletion src/symbolize/gimli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,9 @@ impl<'data> Context<'data> {
fn mmap(path: &Path) -> Option<Mmap> {
let file = File::open(path).ok()?;
let len = file.metadata().ok()?.len().try_into().ok()?;
unsafe { Mmap::map(&file, len, 0) }
// SAFETY: All files we mmap are mmaped by the dynamic linker or the kernel already for the
// executable code of the process. Modifying them would cause crashes or UB anyways.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is true for some but not all uses of mmap in this crate. DWARF can be in a separate file that is not loaded by the dynamic linker or kernel (.dSYM on macOS, .debug/.dwz/.dwp on linux, or unpacked object files on both).

That doesn't mean we should use fs::read for those though. We don't need to read all of the DWARF, and doing so would affect performance and memory usage.

unsafe { Mmap::map(file, len, 0) }
}

cfg_if::cfg_if! {
Expand Down
4 changes: 2 additions & 2 deletions src/symbolize/gimli/elf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,9 @@ impl Mapping {
let len = file.metadata().ok()?.len();

// NOTE: we map the remainder of the entire archive instead of just the library so we don't have to determine its length
// NOTE: mmap will fail if `zip_offset` is not page-aligned
// SAFETY: See `super::mmap` function
let map = unsafe {
super::mmap::Mmap::map(&file, usize::try_from(len - zip_offset).ok()?, zip_offset)
super::mmap::Mmap::map(file, usize::try_from(len - zip_offset).ok()?, zip_offset)
}?;

Mapping::mk(map, |map, stash| {
Expand Down
6 changes: 5 additions & 1 deletion src/symbolize/gimli/mmap_fake.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ pub struct Mmap {
}

impl Mmap {
pub unsafe fn map(mut file: &File, len: usize, offset: u64) -> Option<Mmap> {
/// A fake `mmap` implementation for platforms with no native file mapping support.
///
/// # Safety
/// This function is always safe to call.
pub unsafe fn map(mut file: File, len: usize, offset: u64) -> Option<Mmap> {
let mut mmap = Mmap {
vec: Vec::with_capacity(len),
};
Expand Down
9 changes: 8 additions & 1 deletion src/symbolize/gimli/mmap_unix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,14 @@ pub struct Mmap {
}

impl Mmap {
pub unsafe fn map(file: &File, len: usize, offset: u64) -> Option<Mmap> {
/// Map a file into memory, returning `None` on failure. `offset` must be a multiple of the page
/// size, or mapping will fail[^1].
///
/// # Safety
/// - Mapped files must not be altered for the lifetime of the returned value.
///
/// [^1]: https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/functions/mmap.html
pub unsafe fn map(file: File, len: usize, offset: u64) -> Option<Mmap> {
let ptr = mmap64(
ptr::null_mut(),
len,
Expand Down
10 changes: 8 additions & 2 deletions src/symbolize/gimli/mmap_windows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,14 @@ pub struct Mmap {
}

impl Mmap {
pub unsafe fn map(file: &File, len: usize, offset: u64) -> Option<Mmap> {
let file = file.try_clone().ok()?;
/// Map a file into memory, returning `None` on failure. `offset` must be a multiple of
/// `dwAllocationGranularity` size, or mapping will fail[^1].
///
/// # Safety
/// - Mapped files must not be altered for the lifetime of the returned value.'
///
/// [^1]: https://learn.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-mapviewoffile
pub unsafe fn map(file: File, len: usize, offset: u64) -> Option<Mmap> {
let mapping = CreateFileMappingA(
file.as_raw_handle(),
ptr::null_mut(),
Expand Down
Loading