From 2fd0265803e868a69d28edf8543af20e46b2eda8 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sat, 21 Sep 2024 20:31:39 -0700 Subject: [PATCH 1/2] Add mmap documentation --- src/symbolize/gimli.rs | 2 ++ src/symbolize/gimli/elf.rs | 2 +- src/symbolize/gimli/mmap_fake.rs | 4 ++++ src/symbolize/gimli/mmap_unix.rs | 7 +++++++ src/symbolize/gimli/mmap_windows.rs | 7 +++++++ 5 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/symbolize/gimli.rs b/src/symbolize/gimli.rs index b9f6f3d6..580272f3 100644 --- a/src/symbolize/gimli.rs +++ b/src/symbolize/gimli.rs @@ -186,6 +186,8 @@ impl<'data> Context<'data> { fn mmap(path: &Path) -> Option { let file = File::open(path).ok()?; let len = file.metadata().ok()?.len().try_into().ok()?; + // 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. unsafe { Mmap::map(&file, len, 0) } } diff --git a/src/symbolize/gimli/elf.rs b/src/symbolize/gimli/elf.rs index b73f6aac..740d85f6 100644 --- a/src/symbolize/gimli/elf.rs +++ b/src/symbolize/gimli/elf.rs @@ -69,7 +69,7 @@ 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) }?; diff --git a/src/symbolize/gimli/mmap_fake.rs b/src/symbolize/gimli/mmap_fake.rs index 71697fc3..0ea05984 100644 --- a/src/symbolize/gimli/mmap_fake.rs +++ b/src/symbolize/gimli/mmap_fake.rs @@ -8,6 +8,10 @@ pub struct Mmap { } impl 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 { let mut mmap = Mmap { vec: Vec::with_capacity(len), diff --git a/src/symbolize/gimli/mmap_unix.rs b/src/symbolize/gimli/mmap_unix.rs index 0895ee5d..8034b209 100644 --- a/src/symbolize/gimli/mmap_unix.rs +++ b/src/symbolize/gimli/mmap_unix.rs @@ -15,6 +15,13 @@ pub struct Mmap { } impl 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 { let ptr = mmap64( ptr::null_mut(), diff --git a/src/symbolize/gimli/mmap_windows.rs b/src/symbolize/gimli/mmap_windows.rs index 1c8bc83c..eb034f7e 100644 --- a/src/symbolize/gimli/mmap_windows.rs +++ b/src/symbolize/gimli/mmap_windows.rs @@ -16,6 +16,13 @@ pub struct Mmap { } impl Mmap { + /// 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 { let file = file.try_clone().ok()?; let mapping = CreateFileMappingA( From 21836755aeb38c385acf8511cc631da98356c8d9 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sat, 21 Sep 2024 20:32:30 -0700 Subject: [PATCH 2/2] Have `Mmap` take ownership of `File` --- src/symbolize/gimli.rs | 2 +- src/symbolize/gimli/elf.rs | 2 +- src/symbolize/gimli/mmap_fake.rs | 2 +- src/symbolize/gimli/mmap_unix.rs | 2 +- src/symbolize/gimli/mmap_windows.rs | 3 +-- 5 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/symbolize/gimli.rs b/src/symbolize/gimli.rs index 580272f3..4be6634c 100644 --- a/src/symbolize/gimli.rs +++ b/src/symbolize/gimli.rs @@ -188,7 +188,7 @@ fn mmap(path: &Path) -> Option { let len = file.metadata().ok()?.len().try_into().ok()?; // 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. - unsafe { Mmap::map(&file, len, 0) } + unsafe { Mmap::map(file, len, 0) } } cfg_if::cfg_if! { diff --git a/src/symbolize/gimli/elf.rs b/src/symbolize/gimli/elf.rs index 740d85f6..441a2295 100644 --- a/src/symbolize/gimli/elf.rs +++ b/src/symbolize/gimli/elf.rs @@ -71,7 +71,7 @@ impl Mapping { // NOTE: we map the remainder of the entire archive instead of just the library so we don't have to determine its length // 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| { diff --git a/src/symbolize/gimli/mmap_fake.rs b/src/symbolize/gimli/mmap_fake.rs index 0ea05984..5659c16f 100644 --- a/src/symbolize/gimli/mmap_fake.rs +++ b/src/symbolize/gimli/mmap_fake.rs @@ -12,7 +12,7 @@ impl Mmap { /// /// # Safety /// This function is always safe to call. - pub unsafe fn map(mut file: &File, len: usize, offset: u64) -> Option { + pub unsafe fn map(mut file: File, len: usize, offset: u64) -> Option { let mut mmap = Mmap { vec: Vec::with_capacity(len), }; diff --git a/src/symbolize/gimli/mmap_unix.rs b/src/symbolize/gimli/mmap_unix.rs index 8034b209..0c35cb46 100644 --- a/src/symbolize/gimli/mmap_unix.rs +++ b/src/symbolize/gimli/mmap_unix.rs @@ -22,7 +22,7 @@ impl Mmap { /// - 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 { + pub unsafe fn map(file: File, len: usize, offset: u64) -> Option { let ptr = mmap64( ptr::null_mut(), len, diff --git a/src/symbolize/gimli/mmap_windows.rs b/src/symbolize/gimli/mmap_windows.rs index eb034f7e..fd5b1e74 100644 --- a/src/symbolize/gimli/mmap_windows.rs +++ b/src/symbolize/gimli/mmap_windows.rs @@ -23,8 +23,7 @@ impl Mmap { /// - 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 { - let file = file.try_clone().ok()?; + pub unsafe fn map(file: File, len: usize, offset: u64) -> Option { let mapping = CreateFileMappingA( file.as_raw_handle(), ptr::null_mut(),